dwsmith1983 opened a new pull request, #6986: URL: https://github.com/apache/datafusion-comet/pull/6986
## Which issue does this PR close? Closes #6972. ## Rationale for this change `canShortCircuitNulls` keeps every argument of an expression eager when a later argument holds a subexpression the operator's expressions share, so an error Spark raises from the hoisted subexpression is raised in Comet too. It treated them as shared whatever `spark.sql.subexpressionElimination.enabled` says. With the setting off, Spark's projection hoists nothing, and its own null short-circuit skips the later argument on a row whose first argument is NULL, so Comet raised where Spark returns rows. `SELECT m1[1 div i], m2[1 div i]` with ANSI on, the setting off, NULL maps and `i = 0` is one example. Honoring the setting everywhere would be wrong the other way. Spark's whole-stage codegen aggregates share the subexpressions of their update expressions whatever the setting says (`AggregateCodegenSupport.doConsumeWithoutKeys` and the keyed paths in `HashAggregateExec` call `subexpressionEliminationForWholeStageCodegen` without checking it, on 3.5 through 4.1), while `ProjectExec`'s codegen and the projections behind shuffle partition keys and `TakeOrderedAndProject` only do so when it is on. ## What changes are included in this PR? - `withSharedSubexpressions` takes an `alwaysShared` flag, default false. With the setting off the operator shares nothing unless it asked for `alwaysShared`. `canShortCircuitNulls` itself is unchanged. - `CometExecRule` passes `alwaysShared` for aggregates (`BaseAggregateExec`). The native shuffle writer and `TakeOrderedAndProject` keep the default, matching Spark's `UnsafeProjection.create`. - With the setting on nothing changes. An aggregate that Spark runs without whole-stage codegen (object hash aggregate, sort aggregate without codegen, or codegen switched off) does not share with the setting off, but Comet still treats it as sharing, so it stays eager there as before: at worst it raises where Spark would not, never a different result. #5867 changes the same part of `QueryPlanSerde`, so whichever lands second needs a small merge. Its decline for dispatched expressions reads the same shared set, so after both land a projection with the setting off stays dispatched again (Spark shares nothing there) while aggregates still decline; the three projection cases in its fixtures that run with the setting off then expect dispatch instead of a fallback. ## How are these changes tested? - `QueryPlanSerdeSuite` checks `canShortCircuitNulls` with the setting off and on, with and without `alwaysShared`, and outside any operator. - New `array_null_short_circuit_cse_off.sql` and `map_null_short_circuit_cse_off.sql` run with ANSI on and the setting off. Projection, `DISTRIBUTE BY`, `ORDER BY` and `TakeOrderedAndProject` pairs that share a raising cast or division over NULL inputs run natively and return Spark's rows; each raised in Comet before this change, which also shows the shuffle cases run through the native shuffle writer. Ungrouped and grouped aggregate pairs that share the raising piece across two functions still raise like Spark, with the error class checked so a fallback to Spark cannot pass them, and an aggregate that shares the whole call (which skips the raising piece on its own) stays native. Both files pin whole-stage codegen, which the aggregate cases depend on. - Dropping the setting check, leaving aggregates out of `alwaysShared`, or defaulting the flag to true each fails the cases it should. Run on Spark 4.1 (the array and map SQL fixtures, `QueryPlanSerdeSuite`, `CometMapExpressionSuite`, `CometArrayExpressionSuite`, `CometNativeShuffleSuite`, `CometTopKSuite`), Spark 3.5 and 4.0 (the null short-circuit fixtures and `QueryPlanSerdeSuite`) and the strict-warnings compile. -- 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]
