Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Conditionally allow to keep partition_by columns when using PARTITIONED BY enhancement #11107
feat: Conditionally allow to keep partition_by columns when using PARTITIONED BY enhancement #11107
Changes from 7 commits
a038d6f
44be074
43b931c
1578273
20197a1
206b274
89adc88
e91397a
e352203
4132827
23d95c0
f4e2158
5f02278
e76d811
fdbca78
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this special case is unfortunate, but I don't have a great idea of how to make it better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After further consideration, IMO we should restrict this handling to format options. For other configurations, users must specify the prefix. Otherwise, this list will continue to grow longer, and using those prefixes would lose their meaning (which is why all these refactors were done to have a structured configuration).
Additionally, instead of using "hive," we need a more general term. Perhaps the "execution" prefix would be a better alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on what you mean by "we should restrict this handling to format options"? I am unsure if you are suggesting using a different struct for these non-
format.
options or continue using the HashMap with a mix of options and then extract out the logic to filter out the non-format.
options and do not pass those down throughtable_options.alter_with_string_hash_map
.I agree on changing the prefix to
execution.
which also aligns with theExecutionOptions
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave a try to this in e91397a . Let me know if that's what you had in mind. Thanks for feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what I had in my mind. Thank you for the collaboration. I just would like to mention two more points:
execution
prefix to the example indatafusion/sql/src/parser.rs
.If the user provides anything other than "true," it is interpreted as "false." It might be wiser to give an error if the value is neither "true" nor "false."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I just fixed that test case and also slightly modified the handling of the value to account for what we discussed:
ExecutionOptions
value, false by default.Hopefully e352203 is finally ready. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is a good first step towards adding settings other than format. Thanks for your effort, @hveiga. I think the PR is ready to be merged once conflicts are resolved.