-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Change wildcard qualifier type from String to TableReference
#11073
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
Conversation
| string qualifier = 1; | ||
| string qualifier = 1 [deprecated = true]; | ||
| TableReference relation = 2; |
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.
Theoretically this should be a oneof, but I'm not sure if adding oneof will alter the pbjson serialization. Since we are deprecating the field anyway, I decided to handle the disambiguation in from_proto.rs and to_proto.rs instead.
Also, I'm not sure if we are allowed to make breaking changes to protobuf here. If we do, I can simply change the type of qualifier to make the code simpler.
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.
Also, I'm not sure if we are allowed to make breaking changes to protobuf here. If we do, I can simply change the type of qualifier to make the code simpler.
At the moment, we don't try to maintain any backwards compatibility, thus I think we should opt for simpler code
datafusion/datafusion/proto/src/lib.rs
Lines 37 to 42 in 8aad208
| //! # Version Compatibility | |
| //! | |
| //! The serialized form are not guaranteed to be compatible across | |
| //! DataFusion versions. A plan serialized with one version of DataFusion | |
| //! may not be able to deserialized with a different version. | |
| //! |
String to TableReference
alamb
left a comment
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 @linhr -- this change makes sense to me
I think the protobuf code could be made simpler as I mentioned, but otherwise this seems like a good cleanup to me. Thank you
| string qualifier = 1; | ||
| string qualifier = 1 [deprecated = true]; | ||
| TableReference relation = 2; |
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.
Also, I'm not sure if we are allowed to make breaking changes to protobuf here. If we do, I can simply change the type of qualifier to make the code simpler.
At the moment, we don't try to maintain any backwards compatibility, thus I think we should opt for simpler code
datafusion/datafusion/proto/src/lib.rs
Lines 37 to 42 in 8aad208
| //! # Version Compatibility | |
| //! | |
| //! The serialized form are not guaranteed to be compatible across | |
| //! DataFusion versions. A plan serialized with one version of DataFusion | |
| //! may not be able to deserialized with a different version. | |
| //! |
|
Thank you @alamb ! I've updated the protobuf code as suggested. |
alamb
left a comment
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 again @linhr
…che#11073) * Change wildcard qualifier type * Update protobuf * Minor update
Which issue does this PR close?
Closes #11072.
Rationale for this change
This PR changes the wildcard qualifier type to
TableReference. This avoids an implicit SQL parsing during logical planning, and makes the API more consistent.What changes are included in this PR?
The PR changes the wildcard qualifier type for
Expr::Wildcardand updates the.protofile.Are these changes tested?
The changes are covered by existing tests.
Are there any user-facing changes?
Yes, this PR contains breaking changes to the public API.