Repository: opensafely-core/cohort-extractor (language: python)

Issue:
Treat null dates as 1900-01-01 in EMIS
#### Problem
Currently, clinical events with no known "applicable" date are NULL in EMIS (this is the date we use in cohortextractor which represents the date the information applies to, rather than when it was entered). In TPP unknown dates default to `1900-01-01`. This means that these events are treated differently in cohort-extractor: (a) events with null dates are excluded when an `on-or-before` date is entered, and (b) if no date limits are entered, a result is returned for all `returning` options other than `date`, which returns a null, not indicating that the patient had actually matched the given criteria.  

Further discussion in [this](https://github.com/opensafely/emis-qa/issues/4) issue. 

#### Solution
Convert null dates to `1900-01-01` (or make sure cohortextractor treats them as such). 


Discussion / hints:
We should consider implementing this as a `FLOOR_DATE` and (`CEILING_DATE`?) "constant" in code to make its intent clear. We will also need to be sure we add this in the right places in documentation.

Fix patch:
diff --git a/cohortextractor/emis_backend.py b/cohortextractor/emis_backend.py
index 68aa3703..a9d5203c 100644
--- a/cohortextractor/emis_backend.py
+++ b/cohortextractor/emis_backend.py
@@ -28,6 +28,14 @@
 ICNARC_TABLE = "icnarc_view"
 ONS_TABLE = "ons_view"
 CPNS_TABLE = "cpns_view"
+# Sometimes a clinician will want to record that an event has happened at some
+# point in the past without knowing the specific date. In TPP such events are
+# given a date of 1900-01-01 and in EMIS they are given a date of NULL.
+# However, we already use NULL as a value to indicate that a patient had no
+# matching event at all, so we need another value to indicate "they had a
+# matching event but at an indeterminate time in the past". For consistency's
+# sake we use the same date as in TPP.
+EARLIEST_DATE = "1900-01-01"
 
 
 class EMISBackend:
@@ -855,8 +863,12 @@ def _patients_with_events(
         ignore_missing_values=False,
     ):
         codelist_table, codelist_queries = self.create_codelist_table(codelist)
+        # Using COALESCE like this has the potential to produce very
+        # inefficient queries by scuppering use of indices. However, my
+        # understanding is there are no indicies anyway in the current Presto
+        # setup so this shouldn't actually make much difference
         date_condition, date_joins = self.get_date_condition(
-            from_table, "effective_date", between
+            from_table, f"COALESCE(effective_date, {quote(EARLIEST_DATE)})", between
         )
         ignored_day_condition, extra_queries = self._these_codes_occur_on_same_day(
             from_table, ignore_days_where_these_codes_occur
@@ -869,10 +881,10 @@ def _patients_with_events(
 
         # Result ordering
         if find_first_match_in_period:
-            ordering = "ASC"
+            ordering = "ASC NULLS FIRST"
             date_aggregate = "MIN"
         else:
-            ordering = "DESC"
+            ordering = "DESC NULLS LAST"
             date_aggregate = "MAX"
 
         query_column = None
@@ -911,7 +923,7 @@ def _patients_with_events(
               registration_id,
               hashed_organisation,
               {column_definition} AS {column_name},
-              DATE(effective_date) AS date
+              COALESCE(DATE(effective_date), {quote(EARLIEST_DATE)}) AS date
             FROM (
               SELECT
                 {from_table}.registration_id,
@@ -938,7 +950,7 @@ def _patients_with_events(
               {from_table}.registration_id,
               {from_table}.hashed_organisation,
               {column_definition} AS {column_name},
-              {date_aggregate}(DATE(effective_date)) AS date
+              {date_aggregate}(COALESCE(DATE(effective_date), {quote(EARLIEST_DATE)})) AS date
             FROM {from_table}{additional_join}
             INNER JOIN {codelist_table}
             ON {code_column} = {codelist_table}.code


Test patch:
diff --git a/tests/test_emis_backend.py b/tests/test_emis_backend.py
index 3de1fdd2..b6bd2cc7 100644
--- a/tests/test_emis_backend.py
+++ b/tests/test_emis_backend.py
@@ -2086,3 +2086,90 @@ def test_truncate_patient_id():
 
     large_id_hex = hex(987654321 + 2 ** 100)[2:]
     assert truncate_patient_id(large_id_hex) == 576460752303423488
+
+
+def test_null_observation_dates_handled_correctly():
+    session = make_session()
+    session.add_all(
+        [
+            Patient(
+                observations=[
+                    Observation(snomed_concept_id="10000001", effective_date=None),
+                    Observation(
+                        snomed_concept_id="10000002", effective_date="2020-02-05"
+                    ),
+                ]
+            ),
+            Patient(
+                observations=[
+                    Observation(
+                        snomed_concept_id="10000001", effective_date="2020-06-12"
+                    ),
+                    Observation(
+                        snomed_concept_id="10000002", effective_date="2021-07-02"
+                    ),
+                ]
+            ),
+        ]
+    )
+    session.commit()
+    codes = codelist(["10000001", "10000002"], "snomedct")
+    study = StudyDefinition(
+        population=patients.all(),
+        first_event_date=patients.with_these_clinical_events(
+            codes,
+            returning="date",
+            find_first_match_in_period=True,
+            date_format="YYYY-MM-DD",
+        ),
+        first_event_code=patients.with_these_clinical_events(
+            codes,
+            returning="code",
+            find_first_match_in_period=True,
+            date_format="YYYY-MM-DD",
+        ),
+        # This should be the same value as `event_date` but doing it this way
+        # makes it use a parition query so we can test that branch
+        date_of_first_code=patients.date_of(
+            "first_event_code", date_format="YYYY-MM-DD"
+        ),
+        last_event_date=patients.with_these_clinical_events(
+            codes,
+            returning="date",
+            find_last_match_in_period=True,
+            date_format="YYYY-MM-DD",
+        ),
+        last_event_code=patients.with_these_clinical_events(
+            codes,
+            returning="code",
+            find_last_match_in_period=True,
+            date_format="YYYY-MM-DD",
+        ),
+        date_of_last_code=patients.date_of("last_event_code", date_format="YYYY-MM-DD"),
+        pre2020_event_date=patients.with_these_clinical_events(
+            codes,
+            on_or_before="2020-01-01",
+            returning="date",
+            find_last_match_in_period=True,
+            date_format="YYYY-MM-DD",
+        ),
+        pre2020_event_code=patients.with_these_clinical_events(
+            codes,
+            on_or_before="2020-01-01",
+            returning="code",
+            find_last_match_in_period=True,
+            date_format="YYYY-MM-DD",
+        ),
+        date_of_pre2020_code=patients.date_of(
+            "pre2020_event_code", date_format="YYYY-MM-DD"
+        ),
+    )
+    assert_results(
+        study.to_dicts(),
+        first_event_date=["1900-01-01", "2020-06-12"],
+        date_of_first_code=["1900-01-01", "2020-06-12"],
+        last_event_date=["2020-02-05", "2021-07-02"],
+        date_of_last_code=["2020-02-05", "2021-07-02"],
+        pre2020_event_date=["1900-01-01", ""],
+        date_of_pre2020_code=["1900-01-01", ""],
+    )