Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(redshift)!: Normalize time units in their full singular form #3652

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

VaggelisD
Copy link
Collaborator

Fixes #3651

Normalize Redshift's units from plural/abbreviation to their full singular form during parse time. This is already happening at Snowflake, so the existing infrastructure is moved out to dialect.py as most of these mappings are similar between dialects.

tests/dialects/test_redshift.py Outdated Show resolved Hide resolved
sqlglot/dialects/dialect.py Outdated Show resolved Hide resolved
@VaggelisD
Copy link
Collaborator Author

VaggelisD commented Jun 14, 2024

I thought again and have concluded to the following regarding this PR:

  • I moved DATE_PART_MAPPING to Dialect, thus any dialect that wants to change it's mappings can do so and pass its self in map_date_part().
  • There was a dispute between Redshift EPOCH & Snowflake EPOCH_SECOND but Snowflake also supports EPOCH, EPOCH_SECONDS as abbreviations for EPOCH_SECOND; Thus, I followed the convention that Snowflake will turn EPOCH_SECOND -> EPOCH, and with this change we won't need to override the mapping in either dialect

Regarding the general refactor:

  • We can probably swap exp.TimeUnit::UNABBREVIATED_UNIT_NAME with Dialect::DATE_PART_MAPPING, which should normalize most (if not all) dialects and remove the need for map_date_part() everywhere.
  • If any dialect has different mappings, we can go the manual map_date_part(unit, dialect) route. This is a necessary evil because exp.TimeUnit won't have access to the dialect specific mapping, but we can rethink this later on.

Any thoughts?

Redshift date parts | Snowflake date parts

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We can probably swap exp.TimeUnit::UNABBREVIATED_UNIT_NAME with Dialect::DATE_PART_MAPPING, which should normalize most (if not all) dialects and remove the need for map_date_part() everywhere.

Sounds good. If there are no inconsistencies between dialects then that would make things simpler. The tricky part begins when you have to override certain date parts because there are discrepancies.

If any dialect has different mappings, we can go the manual map_date_part(unit, dialect) route. This is a necessary evil because exp.TimeUnit won't have access to the dialect specific mapping, but we can rethink this later on.

Right, so we'll be able to have an escape hatch. Sounds good to me 👍

@VaggelisD VaggelisD force-pushed the vaggelisd/redshift_units branch from 4008d06 to b6cb790 Compare June 14, 2024 12:23
@georgesittas georgesittas merged commit d331e56 into main Jun 14, 2024
6 checks passed
@georgesittas georgesittas deleted the vaggelisd/redshift_units branch June 14, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issue in transpile
2 participants