grundprinzip commented on code in PR #39090:
URL: https://github.com/apache/spark/pull/39090#discussion_r1051537978


##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -37,6 +37,26 @@ message Expression {
     Alias alias = 6;
     Cast cast = 7;
     UnresolvedRegex unresolved_regex = 8;
+    SortOrder sort_order = 9;
+  }
+
+  message SortOrder {

Review Comment:
   doc please



##########
connector/connect/common/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -246,29 +246,11 @@ message Sort {
   // (Required) Input relation for a Sort.
   Relation input = 1;
 
-  // (Required) Sort fields.
-  repeated SortField sort_fields = 2;
+  // (Required) The ordering expressions

Review Comment:
   ```suggestion
     // (Required) The ordering expressions.
   ```



##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -37,6 +37,26 @@ message Expression {
     Alias alias = 6;
     Cast cast = 7;
     UnresolvedRegex unresolved_regex = 8;
+    SortOrder sort_order = 9;

Review Comment:
   Do you mind adding a bit of documentation that would explain what are valid 
uses of `SortOrder` in expressions? This is just to make sure it's understood 
that you can't use this in a projection etc.
   
   If the only use-case for SortOrder is to be used in `Sort` relation and 
`Window` then maybe it's better to pull it out from the expressions message.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to