fix(redshift): roundtrip for HH12 time mapping#7626
Conversation
SQLGlot Integration Test ResultsComparing:
By Dialect
Overallmain: 113210 total, 112112 passed (pass rate: 99.0%), sqlglot version: sqlglot:fix/redshift-to_char_cast: 101037 total, 101037 passed (pass rate: 100.0%), sqlglot version: Transitions: Dialect pair changes: 0 previous results not found, 3 current results not found ✅ 60 test(s) passed |
| # ref: https://docs.aws.amazon.com/redshift/latest/dg/r_FORMAT_strings.html | ||
| TIME_FORMAT = "'YYYY-MM-DD HH24:MI:SS'" | ||
| TIME_MAPPING = {**Postgres.TIME_MAPPING, "MON": "%b", "HH24": "%H", "HH": "%I"} | ||
| INVERSE_TIME_MAPPING = {**Postgres.INVERSE_TIME_MAPPING, "%I": "HH12", "%H": "HH24"} |
There was a problem hiding this comment.
Should this live in Postgres' TIME_MAPPING? Why is it Redshift-specific?
There was a problem hiding this comment.
Good point, I'll test it all in Postgres and move it over if needs be.
There was a problem hiding this comment.
@georgesittas Postgres didn't have the problem, it looks like the "HH": "%I" mapping for Redshift was added accidentally about a year ago. The "HH24": "%H" mapping isn't needed either, it's inherited from Postgres. INVERSE_TIME_MAPPING isn't needed.
There was a problem hiding this comment.
Fair enough about HH24, but don't we still need to map HH to %I for transpilation purposes, though? From their docs, it appears to be equivalent to HH12:
HH or HH12 | Hour (12-hour clock, 01–12)
There was a problem hiding this comment.
That one is the same in Postgres, so it should probably be handled there. Right now, everything roundtrips OK in Redshift (and Postgres). Do you want me to update that part in Postgres for this PR?
94f98f4 to
67d6899
Compare
|
@fivetran-kwoodbeck I think there may be another issue with time formatting in Redshift and other derivatives of Postgres: certain datetime functions call "TO_CHAR": build_formatted_time(exp.TimeToStr, "postgres"),This means that overrides in Redshift such as the ones relevant to this PR won't be accounted for. This issue may exist in other dialect hierarchies, too, so it's worth investigating further. |
67d6899 to
eca672f
Compare
Yes @georgesittas it was an issue, I added overrides for "TO_CHAR" and a few others which fixes it. Was that what you were thinking? |
eca672f to
dbe1c83
Compare
| TIME_FORMAT = "'YYYY-MM-DD HH24:MI:SS'" | ||
| TIME_MAPPING = {**Postgres.TIME_MAPPING, "MON": "%b", "HH24": "%H", "HH": "%I"} | ||
| TIME_MAPPING = {**Postgres.TIME_MAPPING, "MON": "%b", "MONTH": "%B"} | ||
| INVERSE_TIME_MAPPING = {**Postgres.INVERSE_TIME_MAPPING, "%b": "MON", "%B": "MONTH"} |
There was a problem hiding this comment.
This shouldn't be necessary. If it is, something is wrong with the population of INVERSE_TIME_MAPPING.
| "TO_CHAR": build_formatted_time(exp.TimeToStr, "redshift"), | ||
| "TO_DATE": build_formatted_time(exp.StrToDate, "redshift"), | ||
| "TO_TIMESTAMP": lambda args: ( | ||
| exp.UnixToTime.from_arg_list(args) | ||
| if len(args) == 1 | ||
| else build_formatted_time(exp.StrToTime, "redshift")(args) | ||
| ), |
There was a problem hiding this comment.
I think there's a better refactor we can do, I'll get this in and do it as a follow-up.
2 participants
TO_CHARformat strings withHH12round-tripped back asHH. Root cause:TIME_MAPPINGhad both"HH12": "%I"(from Postgres) and"HH": "%I"(Redshift override), and since HH appeared later in the dict, the auto-generatedINVERSE_TIME_MAPPINGmapped%I → HHinstead of%I → HH12.Fix: Added
INVERSE_TIME_MAPPINGoverride indialects/redshift.py