feat: handle temporal columns in group bys

[betodealmeida]

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Query before was invalid:

SELECT FLOOR("__time" TO DAY) AS "__time",
       COUNT(*) AS "count"
FROM "druid"."wikipedia"
GROUP BY FLOOR("__time" TO DAY)
ORDER BY "count" DESC
LIMIT 100;

SELECT FLOOR("__time" TO DAY) AS "__timestamp",
       FLOOR("__time" TO DAY) AS "__time",
       COUNT(*) AS "count"
FROM "druid"."wikipedia"
WHERE FLOOR("__time" TO DAY) = '2016-06-27T00:00:00.000Z' -- THIS FAILS
GROUP BY FLOOR("__time" TO DAY),
         FLOOR("__time" TO DAY)
ORDER BY "count" DESC
LIMIT 10000;

Query after:

SELECT FLOOR("__time" TO DAY) AS "__time",
       COUNT(*) AS "count"
FROM "druid"."wikipedia"
GROUP BY FLOOR("__time" TO DAY)
ORDER BY "count" DESC
LIMIT 100;

SELECT FLOOR("__time" TO DAY) AS "__timestamp",
       FLOOR("__time" TO DAY) AS "__time",
       COUNT(*) AS "count"
FROM "druid"."wikipedia"
WHERE FLOOR("__time" TO DAY) = TIME_PARSE('2016-06-27T00:00:00+00:00') -- THIS IS CORRECT
GROUP BY FLOOR("__time" TO DAY),
         FLOOR("__time" TO DAY)
ORDER BY "count" DESC
LIMIT 10000;

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[villebro]

LGTM - just needs a rebase

[villebro]

Oh wow, I didn't know these weren't defined yet for Druid!

[betodealmeida]

Yeah, I implemented epoch_to_dttm while working on a solution, and decided to leave it there even if I wasn't using it, and add epoch_ms_to_dttm for completeness.

[john-bodley]

@betodealmeida and @villebro I don't believe this logic works for all cases. You could have a temporal field which is encoded as a string per the Python date format which shouldn't be converted to a TIMESTAMP.

Isn't the correct logic to use the type of the column rather than assuming it's a TIMESTAMP? Furthermore self.db_engine_spec.convert_dttm("TIMESTAMP", dttm) could return None (even for the TIMESTAMP target type). I was thinking the logic should be of the form,

if column_map[dimension].is_temporal and and isinstance(value, str):
    if result := self.db_engine_spec.convert_dttm(
        column_map[dimension].type, 
        dateutil.parser.parse(value),
    ):
        value = text(result)

[john-bodley]

@betodealmeida I have proposed a fix in #17312.