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

[SPARK-48639][CONNECT][PYTHON] Add Origin to RelationCommon #47115

Closed
wants to merge 2 commits into from

Conversation

HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR proposes to add Origin (from #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.

-  // 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.

// (Required) Shared relation metadata.
string source_info = 1;
string source_info = 1 [deprecated=true];
Copy link
Member Author

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

Copy link
Contributor

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

@HyukjinKwon
Copy link
Member Author

cc @grundprinzip

Copy link
Contributor

@grundprinzip grundprinzip left a 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?

@HyukjinKwon
Copy link
Member Author

Yup

@HyukjinKwon
Copy link
Member Author

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants