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

refactor: make SqlToRel::new derive the parser options from the context provider #14822

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

niebayes
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Previously, the SqlToRel::new method uses a default ParserOptions.
Now, its parser options derive the SqlParserOptions from the context provider.

Are these changes tested?

Yes, covered by existing test cases.

Are there any user-facing changes?

Previously, users need to set the parser options once when creating a session config, and again when creating a SqlToRel.
Now, users can set the parser options only once when creating a session config, and expect each SqlToRel's created by SqlToRel::new derives the parser options in the session config.

@github-actions github-actions bot added the sql SQL Planner label Feb 22, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @niebayes -- this makes sense to me

Could we please add some sort of test to amke sure we don't accidentally break this in the future? Like maybe a query that requires enable_ident_normalization ?

@niebayes
Copy link
Contributor Author

@alamb Sure, I'll add some tests soon.

@alamb
Copy link
Contributor

alamb commented Feb 26, 2025

@alamb Sure, I'll add some tests soon.

Let's do it in a follow on PR to reduce the review queue

@alamb alamb merged commit 3d64de4 into apache:main Feb 26, 2025
24 checks passed
@niebayes niebayes deleted the refactor/sql_to_rel_derive_options branch February 26, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make SqlToRel respect parser options from ContextProvider
2 participants