HeartSaVioR commented on a change in pull request #31580:
URL: https://github.com/apache/spark/pull/31580#discussion_r585262907
##########
File path:
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/SortDirection.java
##########
@@ -19,14 +19,33 @@
import org.apache.spark.annotation.Experimental;
+import static
org.apache.spark.sql.connector.expressions.NullOrdering.NULLS_FIRST;
+import static
org.apache.spark.sql.connector.expressions.NullOrdering.NULLS_LAST;
+
/**
* A sort direction used in sorting expressions.
+ * <p>
+ * Each direction has a default null ordering that is implied if no null
ordering is specified
+ * explicitly.
*
* @since 3.2.0
*/
@Experimental
public enum SortDirection {
- ASCENDING, DESCENDING;
+ ASCENDING(NULLS_FIRST), DESCENDING(NULLS_LAST);
+
+ private final NullOrdering defaultNullOrdering;
+
+ SortDirection(NullOrdering defaultNullOrdering) {
+ this.defaultNullOrdering = defaultNullOrdering;
+ }
+
+ /**
+ * Returns the default null ordering to use if no null ordering is specified
explicitly.
+ */
+ public NullOrdering defaultNullOrdering() {
Review comment:
So the current implementation of SortOrder is following:
```
/**
* Represents a sort order in the public expression API.
*
* @since 3.2.0
*/
@Experimental
public interface SortOrder extends Expression {
/**
* Returns the sort expression.
*/
Expression expression();
/**
* Returns the sort direction.
*/
SortDirection direction();
/**
* Returns the null ordering.
*/
NullOrdering nullOrdering();
}
```
which we force to implement `nullOrdering()` which destroys the purpose of
having default null ordering in SortDirection.
Given SortDirection will have default null ordering, we can provide "default
implementation" of `nullOrdering()` which would be simply `return
direction().defaultNullOrdering();` and add some explanation to javadoc to
override this method if they don't want to follow the default null ordering of
SortDirection.
(I actually thought about Optional and I found by myself not prefer it.
Given we're able to leverage default method in interface, I don't see the
reason not to use it.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
