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-41164][CONNECT] Update relations.proto to follow Connect proto development guide #38678
[SPARK-41164][CONNECT] Update relations.proto to follow Connect proto development guide #38678
Conversation
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.
lgtm
be53f73
to
eef3333
Compare
Merged to master. |
|
||
// (Optional) The join condition. Could be unset when `using_columns` is utilized. | ||
// | ||
// This field does not co-exist with using_columns. | ||
Expression join_condition = 3; |
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.
why is this not optional?
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 is Expression
which is a message. By default it has has_field_name
.
I think per current proto sytle guide we do not need optional
for messages.
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.
In fact, not sure if we need to improve the proto style:
For a optional Message field, we also use optional
to mark to keep consistency?
… development guide ### What changes were proposed in this pull request? As we have a guidance for Connect proto ([adding proto messages](https://github.com/apache/spark/blob/master/connector/connect/docs/adding-proto-messages.md)), this PR updates `relations.proto` to follow the development guide. This PR also adds some missing documentation for the proto. ### Why are the changes needed? 1. Follow development guide. 2. Improve proto Documentation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? Existing UT Closes apache#38678 from amaliujia/improve_relation_proto_to_follow_proto_rules. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… development guide ### What changes were proposed in this pull request? As we have a guidance for Connect proto ([adding proto messages](https://github.com/apache/spark/blob/master/connector/connect/docs/adding-proto-messages.md)), this PR updates `relations.proto` to follow the development guide. This PR also adds some missing documentation for the proto. ### Why are the changes needed? 1. Follow development guide. 2. Improve proto Documentation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? Existing UT Closes apache#38678 from amaliujia/improve_relation_proto_to_follow_proto_rules. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… development guide ### What changes were proposed in this pull request? As we have a guidance for Connect proto ([adding proto messages](https://github.com/apache/spark/blob/master/connector/connect/docs/adding-proto-messages.md)), this PR updates `relations.proto` to follow the development guide. This PR also adds some missing documentation for the proto. ### Why are the changes needed? 1. Follow development guide. 2. Improve proto Documentation. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? Existing UT Closes apache#38678 from amaliujia/improve_relation_proto_to_follow_proto_rules. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
What changes were proposed in this pull request?
As we have a guidance for Connect proto (adding proto messages), this PR updates
relations.proto
to follow the development guide.This PR also adds some missing documentation for the proto.
Why are the changes needed?
Does this PR introduce any user-facing change?
NO
How was this patch tested?
Existing UT