Copilot commented on code in PR #6968: URL: https://github.com/apache/datafusion-comet/pull/6968#discussion_r4268963649
########## docs/source/user-guide/latest/tuning/operators.md: ########## @@ -187,12 +187,15 @@ nested in arrays and structs, so `ORDER BY`, window ordering and range partition even with `spark.comet.exec.strictFloatingPoint=true`. Only the comparison key is normalized; returned values keep their original NaN representation and zero sign. -A key that nests values in an array or struct whose type can hold a null element or field falls back in every mode under -`ASC NULLS LAST` or `DESC NULLS FIRST`, and in a `RANGE` window frame that has to find a row's peers, because Spark orders -such a null below every other value and the native sort and window frames would not -([#6476](https://github.com/apache/datafusion-comet/issues/6476), -[#6477](https://github.com/apache/datafusion-comet/issues/6477)). The default null orders run natively, and -`spark.comet.expression.SortOrder.allowIncompatible=true` forces the other null orders onto the native path. +A key that nests values in an array or struct whose type can hold a null element or field stays native under +`ASC NULLS LAST` and `DESC NULLS FIRST` too: Comet separates the key's root null placement from its nested ordering +([#6476](https://github.com/apache/datafusion-comet/issues/6476)). A `RANGE` window frame that has to find a row's +peers over such a key still falls back to Spark +([#6477](https://github.com/apache/datafusion-comet/issues/6477)). `sort_array` sorts array elements rather than ordering rows. It follows Spark's floating-point ordering as well, so it also stays native with `spark.comet.exec.strictFloatingPoint=true`. + +The default `array_sort` comparator also runs natively. It puts null array elements last while comparing nulls +inside nested arrays and structs below non-null values. A custom `array_sort` comparator continues through the JVM +codegen dispatcher. Review Comment: The PR behavior (and tests) indicate arrays with non-default string collations also stay on the Spark/JVM path, but this section only mentions custom comparators. Please document the non-default collation limitation here as well so users understand when `array_sort` will not run natively. ########## spark/src/main/scala/org/apache/comet/serde/CometSortOrder.scala: ########## @@ -46,26 +46,18 @@ import org.apache.comet.serde.QueryPlanSerde.exprToProtoInternal */ object CometSortOrder extends CometExpressionSerde[SortOrder] { - /** - * Subjects of both the runtime fallback reasons and the generated compatibility docs. Shared so - * the two cannot describe the policy differently. - */ - private val nestedNullOrderSort = - "Sorting with `ASC NULLS LAST` or `DESC NULLS FIRST` on an array or struct that can hold a " + - "null element or field" + private val nondeterministicNestedKeyReason = + "A nested sort key with nondefault root null placement must be deterministic" Review Comment: This fallback/unsupported reason doesn’t explain *why* determinism is required. Since the implementation wraps the key and (at least conceptually) evaluates it twice (for `isnull(key)` and `key`), the message should mention duplicate evaluation and that nondeterministic expressions could yield inconsistent results. ########## spark/src/main/scala/org/apache/comet/serde/CometSortOrder.scala: ########## @@ -86,7 +78,13 @@ object CometSortOrder extends CometExpressionSerde[SortOrder] { expr: SortOrder, inputs: Seq[Attribute], binding: Boolean): Option[ExprOuterClass.Expr] = { - val childExpr = exprToProtoInternal(expr.child, inputs, binding) + val wrapped = if (needsRootNullWrapper(expr)) { + CreateNamedStruct( + Seq(Literal("root_null"), IsNull(expr.child), Literal("value"), expr.child)) + } else { + expr.child + } Review Comment: Wrapping the key as `struct(isnull(child), child)` duplicates the child expression, which can force computing the sort key twice per row in the native expression pipeline. Consider introducing a dedicated wrapper node (or equivalent) in the native/proto representation that computes the child once and reuses it for both fields, or add native common-subexpression elimination for identical subtrees in this sort-key path. -- 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]
