-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Miscellaneous cleanups #17189
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
Miscellaneous cleanups #17189
Conversation
It became redundant when `input_expr_types: &[DataType]` was replaced with `input_expr_fields: &[Field]`, i.e. in 577c424 commit. The benefit of passing in a "field" is that it combines data type and nullable attribute.
7c383fc
to
dbf832a
Compare
In `UserDefinedLogicalNode::check_invariants`, the actual plan to check for invariants is `self`. The `plan` is always `LogicalPlan::Extension` and provides no further information. It's confusing.
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.
These all look like very nice cleanups. Thank you for the PR!
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.
Thanks @findepi -- the only thing I am concerned about with this PR is the change to check_invariants
. Otherwise this entire PR looks like a very nice improvement
field_qualifiers: vec![None; field_count], | ||
functional_dependencies: FunctionalDependencies::empty(), | ||
}; | ||
dfschema.check_names()?; |
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.
Context is that checking names here makes this consistent with the other (fallible) ways to create a DFSchema from a Schema
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.
FWIW it seems this has caused a regression for some users in 50.0.0 (there are some code paths that violate an invariant). See #17706
This reverts commit c31ab2e.
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.
Thanks @findepi and @timsaucer
See individual commits