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

Update Sqlglot and fix regression #26

Merged
merged 4 commits into from
Aug 5, 2024
Merged

Update Sqlglot and fix regression #26

merged 4 commits into from
Aug 5, 2024

Conversation

wongjingping
Copy link
Contributor

@wongjingping wongjingping commented Aug 5, 2024

pinned sqlglot version in requirements.txt
fixed featurization of TO_DATE when dialect="postgres"
linted repo
add additional test using column names with spaces
add vscode settings to facilitate easier testing

add additional test using column names with spaces
Copy link
Member

@rishsriv rishsriv left a comment

Choose a reason for hiding this comment

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

Thank you!

@rishsriv
Copy link
Member

rishsriv commented Aug 5, 2024

Hmm I'm not quite sure why that test is failing though 🤷🏽‍♂️

@wongjingping
Copy link
Contributor Author

wongjingping commented Aug 5, 2024

Turns out that after bumping sqlglot to the latest version 25.8.1, TO_DATE when parsed with dialect="postgres" is now classified as exp.StrToDate and no longer an anonymous expression. Updated parsing to reflect that. Test results are the same i.e. we didn't change featurization behavior / logic, and only updated it to reflect the same expected behavior with the bumped sqlglot version.

p.s. sqlglot parsing for TO_DATE in postgres was updated in the original repo here ~ 2 weeks ago

@@ -429,6 +429,8 @@ def get_sql_features(
features.date_sub = True
elif isinstance(node, exp.DateTrunc) or isinstance(node, exp.TimestampTrunc):
features.date_trunc = True
elif isinstance(node, exp.StrToDate):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the key regression fix after updating sqlglot

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for documenting and for the fix!

@wongjingping wongjingping changed the title Improve testing of metadata serialization Update Sqlglot and fix regression Aug 5, 2024
@wongjingping wongjingping merged commit ae36c54 into main Aug 5, 2024
1 check passed
@wongjingping wongjingping deleted the jp/test branch August 5, 2024 02:18
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.

2 participants