Fix array_except nullability mismatch - #4237
Conversation
|
@comphead This PR should be ready for review. |
andygrove
left a comment
There was a problem hiding this comment.
Thanks for tackling this one. The approach of a delegating UDF wrapper that normalizes inner list-field nullability at runtime makes sense, since the serde alone cannot fix a check that inspects the actual Arrow array's field nullability at execution time. Normalizing containsNull from false to true only touches type metadata and does not change the set-difference result, so this looks safe from a Spark compatibility standpoint. The test coverage is also good, especially exercising column-sourced arrays with mixed nullability through checkSparkAnswerAndOperator so we know native execution is actually verified.
A few things to look at:
CI: clippy is red (blocking). ubuntu-latest/rust-test fails on clone_on_ref_ptr at three sites in comet_scalar_funcs.rs (lines 262, 279, 296), all the list_arr.values().clone() calls. spark-expr/src/lib.rs has #![deny(clippy::clone_on_ref_ptr)], so these are hard errors. Switching them to Arc::clone(list_arr.values()) should fix it.
CI: spark-sql-auto-sql_core-1 (Spark 4.0). I read through this log and could not find a failure tied to array_except. It is dominated by unrelated streaming and view warnings, and the native build failed in the same pipeline, so this looks flaky or environmental rather than something this PR introduced. Worth a re-run once clippy is fixed to confirm it goes green.
Duplication with an existing helper. arrays.scala already has normalizeContainerNullability in CometCreateArray (line 551) that recursively forces container nullability across ArrayType, MapType, and StructType. The new normalizeArrayContainsNull is a narrower version that only walks ArrayType. Since CometArrayExcept.isTypeSupported restricts elements to primitives and nested arrays, the narrower version is functionally fine today, but it might be worth extracting the existing helper to a shared spot and reusing it rather than keeping two.
Speculative LargeList / FixedSizeList branches. Spark arrays arrive over FFI as regular List, so those arms of normalize_list_inner_nullability and normalize_list_scalar may be unreachable on this path, and they account for a good chunk of the new code. Could they be trimmed to just List (keeping the nested-List recursion, which is genuinely needed)? If they are intentionally defensive, a short comment would help.
Return type: which path wins? The serde already sends an explicit normalized return type via scalarFunctionExprToProtoWithReturnType, and the native NormalizingArrayExcept::return_type also normalizes independently. Which one actually drives the planned schema? If the serde one wins, the native override and the stored data_type field (only used in PartialEq/Hash/Debug) look redundant and could be dropped.
Why not the sibling functions? array_union and array_intersect still use plain scalarFunctionExprToProto and seem to work. A one-line note on why array_except's check_datatypes is stricter would help future readers understand the asymmetry.
Minor test comment. In CometArrayExpressionSuite, the comment mentions "one with WHERE clause filtering, one without", but the query SELECT array_except(array(_2, _3), array(_4)) from t1 has no WHERE clause. Might be worth tidying so it matches.
|
Sorry for the late review @yuboxx. Feel free to ping me if you continue working on this |
Which issue does this PR close?
Closes #3646.
Rationale for this change
Spark can produce equivalent arrays with different
containsNullvalues. DataFusion'sarray_exceptchecks Arrow list types strictly, including child field nullability, so Comet could reject compatible Spark arrays withList(Int32)vsList(non-null Int32).What changes are included in this PR?
This PR wraps DataFusion's
array_exceptUDF to normalize list child nullability before delegation, and serializes an explicit normalized return type so Comet's planned schema matches the runtime Arrow result.It also re-enables the SQL regression case and adds coverage for nested arrays and mixed-nullability inputs.
How are these changes tested?
cargo test -p datafusion-comet-spark-expr normalizes_ --lib./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite array_except" -Dtest=none