-
Notifications
You must be signed in to change notification settings - Fork 28.6k
[SPARK-48639][CONNECT][PYTHON] Add Origin to RelationCommon #47115
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
….RelationCommon"" This reverts commit 2e31572.
// (Required) Shared relation metadata. | ||
string source_info = 1; | ||
string source_info = 1 [deprecated=true]; |
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.
Just switched to deprecation. It breaks the protobuf compat check, and I should backport this change all down to branch-3.4, which isn't worthwhile
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.
Can we try annotate it with a buf comment?
Something like
// buf:lint:ignore FIELD_NO_DELETE
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.
So the main reason for doing it like this is that we compare to 3.5 branch correct? And setting to reserved breaks it?
Yup |
Merged to master. |
### What changes were proposed in this pull request? This PR proposes to add `Origin` (from apache#46789) to `Relation.RelationCommon` This is a revert of the revert. ### Why are the changes needed? To have the common protobuf message to keep the source code info. ```diff - // TODO(SPARK-48639): Add origin like Expression.ExpressionCommon - - // (Required) Shared relation metadata. - string source_info = 1; + // (Optional) Shared relation metadata. + reserved 1; ``` This is considered as a breaking change, and we should fix up all other branches down to avoid, which isn't really worthwhile. ### Does this PR introduce _any_ user-facing change? No. This is not used. ### How was this patch tested? CI should validate protobuf definition, and exiting tests should pass. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47115 from HyukjinKwon/SPARK-48639-followup. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
What changes were proposed in this pull request?
This PR proposes to add
Origin
(from #46789) toRelation.RelationCommon
This is a revert of the revert.
Why are the changes needed?
To have the common protobuf message to keep the source code info.
This is considered as a breaking change, and we should fix up all other branches down to avoid, which isn't really worthwhile.
Does this PR introduce any user-facing change?
No. This is not used.
How was this patch tested?
CI should validate protobuf definition, and exiting tests should pass.
Was this patch authored or co-authored using generative AI tooling?
No.