Fixing Pinot queries for time granularities: WEEKS/MONTHS/QUARTERS/YE…

…ARS (#12536)
apache · Jan 15, 2021 · 5f2de1d · 5f2de1d
1 parent 603ab75
commit 5f2de1d
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 7 deletions.
diff --git a/superset/db_engine_specs/pinot.py b/superset/db_engine_specs/pinot.py
@@ -35,10 +35,10 @@ class PinotEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
         "PT1M": "1:MINUTES",
         "PT1H": "1:HOURS",
         "P1D": "1:DAYS",
-        "P1W": "1:WEEKS",
-        "P1M": "1:MONTHS",
-        "P0.25Y": "3:MONTHS",
-        "P1Y": "1:YEARS",
+        "P1W": "week",
+        "P1M": "month",
+        "P0.25Y": "quarter",
+        "P1Y": "year",
     }
 
     _python_to_java_time_patterns: Dict[str, str] = {
@@ -50,6 +50,17 @@ class PinotEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
         "%S": "ss",
     }
 
+    _use_date_trunc_function: Dict[str, bool] = {
+        "PT1S": False,
+        "PT1M": False,
+        "PT1H": False,
+        "P1D": False,
+        "P1W": True,
+        "P1M": True,
+        "P0.25Y": True,
+        "P1Y": True,
+    }
+
     @classmethod
     def get_timestamp_expr(
         cls,
@@ -86,8 +97,13 @@ def get_timestamp_expr(
                 raise NotImplementedError("No pinot grain spec for " + str(time_grain))
         else:
             return TimestampExpression("{{col}}", col)
+
         # In pinot the output is a string since there is no timestamp column like pg
-        time_expr = f"DATETIMECONVERT({{col}}, '{tf}', '{tf}', '{granularity}')"
+        if cls._use_date_trunc_function.get(time_grain):
+            time_expr = f"DATETRUNC('{granularity}', {{col}}, '{seconds_or_ms}')"
+        else:
+            time_expr = f"DATETIMECONVERT({{col}}, '{tf}', '{tf}', '{granularity}')"
+
         return TimestampExpression(time_expr, col)
 
     @classmethod

diff --git a/tests/db_engine_specs/pinot_tests.py b/tests/db_engine_specs/pinot_tests.py
@@ -23,11 +23,19 @@
 class TestPinotDbEngineSpec(TestDbEngineSpec):
     """ Tests pertaining to our Pinot database support """
 
+    def test_pinot_time_expression_sec_one_1d_grain(self):
+        col = column("tstamp")
+        expr = PinotEngineSpec.get_timestamp_expr(col, "epoch_s", "P1D")
+        result = str(expr.compile())
+        self.assertEqual(
+            result,
+            "DATETIMECONVERT(tstamp, '1:SECONDS:EPOCH', '1:SECONDS:EPOCH', '1:DAYS')",
+        )  # noqa
+
     def test_pinot_time_expression_sec_one_1m_grain(self):
         col = column("tstamp")
         expr = PinotEngineSpec.get_timestamp_expr(col, "epoch_s", "P1M")
         result = str(expr.compile())
         self.assertEqual(
-            result,
-            "DATETIMECONVERT(tstamp, '1:SECONDS:EPOCH', '1:SECONDS:EPOCH', '1:MONTHS')",
+            result, "DATETRUNC('month', tstamp, 'SECONDS')",
         )  # noqa