Resolve errors with time to convert bins (#5283)

* time to convert test

* rworkaround for time to convert bins

* address comment

Author: neilkakkar

ee/clickhouse/queries/funnels/funnel_time_to_convert.py

@@ -60,10 +60,19 @@ def get_query(self) -> str:
         ]
         steps_average_conversion_time_expression_sum = " + ".join(steps_average_conversion_time_identifiers)
 
+        steps_average_conditional_for_invalid_values = [
+            f"{identifier} >= 0" for identifier in steps_average_conversion_time_identifiers
+        ]
+        # :HACK: Protect against CH bug https://github.com/ClickHouse/ClickHouse/issues/26580
+        #   once the issue is resolved, stop skipping the test: test_auto_bin_count_single_step_duplicate_events
+        #   and remove this comment
+
         query = f"""
             WITH
                 step_runs AS (
-                    {steps_per_person_query}
+                    SELECT * FROM (
+                        {steps_per_person_query}
+                    ) WHERE {" AND ".join(steps_average_conditional_for_invalid_values)}
                 ),
                 histogram_params AS (
                     -- Binning ensures that each sample belongs to a bin in results
@@ -98,8 +107,6 @@ def get_query(self) -> str:
                     histogram_from_seconds + floor(({steps_average_conversion_time_expression_sum} - histogram_from_seconds) / bin_width_seconds) * bin_width_seconds AS bin_from_seconds,
                     count() AS person_count
                 FROM step_runs
-                -- We only need to check step to_step here, because it depends on all the other ones being NOT NULL too
-                WHERE step_{to_step}_average_conversion_time_inner IS NOT NULL
                 GROUP BY bin_from_seconds
             ) results
             FULL OUTER JOIN (

ee/clickhouse/queries/funnels/test/test_funnel_time_to_convert.py

@@ -1,3 +1,4 @@
+import unittest
 from uuid import uuid4
 
 from ee.clickhouse.models.event import create_event
@@ -78,6 +79,62 @@ def test_auto_bin_count_single_step(self):
             },
         )
 
+    @unittest.skip("Wait for bug to be resolved")
+    def test_auto_bin_count_single_step_duplicate_events(self):
+        # demonstrates existing CH bug. Current patch is to remove negative times from consideration
+        # Reference on what happens: https://github.com/ClickHouse/ClickHouse/issues/26580
+
+        _create_person(distinct_ids=["user a"], team=self.team)
+        _create_person(distinct_ids=["user b"], team=self.team)
+        _create_person(distinct_ids=["user c"], team=self.team)
+
+        _create_event(event="step one", distinct_id="user a", team=self.team, timestamp="2021-06-08 18:00:00")
+        _create_event(event="step one", distinct_id="user a", team=self.team, timestamp="2021-06-08 19:00:00")
+        # Converted from 0 to 1 in 3600 s
+        _create_event(event="step one", distinct_id="user a", team=self.team, timestamp="2021-06-08 21:00:00")
+
+        _create_event(event="step one", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:00:00")
+        _create_event(event="step one", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:37:00")
+        # Converted from 0 to 1 in 2200 s
+
+        _create_event(event="step one", distinct_id="user c", team=self.team, timestamp="2021-06-11 07:00:00")
+        _create_event(event="step one", distinct_id="user c", team=self.team, timestamp="2021-06-12 06:00:00")
+        # Converted from 0 to 1 in 82_800 s
+
+        filter = Filter(
+            data={
+                "insight": INSIGHT_FUNNELS,
+                "interval": "day",
+                "date_from": "2021-06-07 00:00:00",
+                "date_to": "2021-06-13 23:59:59",
+                "funnel_from_step": 0,
+                "funnel_to_step": 1,
+                "funnel_window_days": 7,
+                "events": [
+                    {"id": "step one", "order": 0},
+                    {"id": "step one", "order": 1},
+                    {"id": "step one", "order": 2},
+                ],
+            }
+        )
+
+        funnel_trends = ClickhouseFunnelTimeToConvert(filter, self.team, ClickhouseFunnel)
+        results = funnel_trends.run()
+
+        # Autobinned using the minimum time to convert, maximum time to convert, and sample count
+        self.assertEqual(
+            results,
+            {
+                "bins": [
+                    (2220.0, 2),  # Reached step 1 from step 0 in at least 2200 s but less than 29_080 s - users A and B
+                    (29080.0, 0),  # Analogous to above, just an interval (in this case 26_880 s) up - no users
+                    (55940.0, 0),  # Same as above
+                    (82800.0, 1),  # Reached step 1 from step 0 in at least 82_800 s but less than 109_680 s - user C
+                ],
+                "average_conversion_time": 29_540,
+            },
+        )
+
     def test_custom_bin_count_single_step(self):
         _create_person(distinct_ids=["user a"], team=self.team)
         _create_person(distinct_ids=["user b"], team=self.team)