Repository navigation
feat(spark): add equal_null - #24827
Conversation
Rewrites to IS NOT DISTINCT FROM in simplify(), mirroring Spark's own RuntimeReplaceable alias of EqualNullSafe. Fills the porting-script stub, restoring the two queries that lost an argument to the typeof() dedup.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main apache/datafusion#24827 +/- ##
==========================================
+ Coverage 82.33% 82.36% +0.02%
==========================================
Files 1137 1138 +1
Lines 432498 433029 +531
Branches 432498 433029 +531
==========================================
+ Hits 356116 356654 +538
Misses 54843 54843
+ Partials 21539 21532 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on adding equal_null support. The overall approach looks good, especially reusing IS NOT DISTINCT FROM rather than introducing a separate comparison implementation.
I found two correctness issues that I think should be addressed before merging. One is around the nullability reported by the UDF when simplification is disabled, and the other is an inconsistency in signed-zero handling for nested values. I left inline comments with reproduction cases and suggested fixes.
|
Both are fixed. For nullability I overrode For signed zero this was not specific to |
|
@advitrocks9 |
@kosiew All green now |
kosiew
left a comment
There was a problem hiding this comment.
@advitrocks9, thanks for the updates here. This looks good to me.
The nullability concern is addressed by return_field_from_args returning a non-nullable Boolean field, and the optimizer-disabled EXPLAIN test confirms that behavior.
The nested signed-zero handling also looks good. Normalizing at the shared nested-comparison boundary covers both regular nested comparisons and the equal_null fallback path, with coverage for scalar and optimizer-disabled array cases.
I also took another look at the grouping coverage. My earlier suggestion to add a nested GROUP BY SQLLogicTest was not necessary. The existing nested_float_edge_cases_match_rows_fallback unit test directly exercises GroupValuesColumn::intern with nested +0.0 and -0.0 values and verifies that its grouping matches the established GroupValuesRows fallback. Since that test predates this change, it provides regression coverage for the path being modified here. An end-to-end SQLLogicTest could still be useful additional coverage, but I don't think it is required for this PR.
Thanks for addressing the feedback!
|
🚀 |
## Which issue does this PR close? No issue filed. Part of #15914, which tracks the Spark function library. ## Rationale for this change `equal_null` is missing, and the `misc` module it belongs in had no functions in it at all. In Spark it is an alias: `EqualNull(l, r)` is rewritten to `EqualNullSafe`, the `<=>` operator, so it is null-safe equality. Both NULL is true, and the result is never NULL: ```sql SELECT equal_null(NULL, NULL); -- true SELECT equal_null(NULL::int, 1::int); -- false ``` DataFusion already has that operator as `IS NOT DISTINCT FROM`, so this mirrors Spark's own structure instead of writing a second comparison kernel. ## What changes are included in this PR? `misc/equal_null.rs` and its registration. `simplify()` rewrites to the operator, which is Spark's `replacement` field, and `invoke_with_args` calls `apply_cmp` with the same operator so the function also works with the logical optimizer disabled, as the crate README requires for Comet. Two of the stub's commented-out queries were malformed, the porting script wrote one cast per distinct `typeof()` key, so a call with two identical literals lost an argument. The same artifact affects 10 more pairs in 6 other spark files, left alone here. Two divergences from Spark are left alone because they belong to the operator, not to this function. Spark treats `-NaN` and `NaN` as equal and DataFusion does not, which is true of `IS NOT DISTINCT FROM` generally. Spark also rejects maps at analysis since `MapType` is not orderable, while `comparison_coercion` here accepts them. `return_field_from_args` is overridden so the field is not nullable. `normalize_float_zero` now recurses into nested children. `compare_op_for_nested` and `GroupValuesColumn` both normalize through it, so the scalar kernels and the two group-by paths agree on nested float keys. ## Are these changes tested? Yes, `spark/misc/equal_null.slt` goes from a skipped stub to 27 assertions covering the truth table, float ordering, columns, arrays, structs, decimals and the arity errors. Reverting `simplify()` to plain `Eq` fails 9 of them, so the file is not passing on constant folding. Putting `invoke_with_args` back to a stub fails the two queries that run with the optimizer off, and nothing else. ## Are there any user-facing changes? Yes. `equal_null` is new in `datafusion-spark`, and `-0.0` now matches `+0.0` inside nested values, in comparisons and in `GROUP BY`, matching the scalar kernels.
Which issue does this PR close?
No issue filed. Part of apache/datafusion-spark#5, which tracks the Spark function library.
Rationale for this change
equal_nullis missing, and themiscmodule it belongs in had no functions in it at all. InSpark it is an alias:
EqualNull(l, r)is rewritten toEqualNullSafe, the<=>operator, soit is null-safe equality. Both NULL is true, and the result is never NULL:
DataFusion already has that operator as
IS NOT DISTINCT FROM, so this mirrors Spark's ownstructure instead of writing a second comparison kernel.
What changes are included in this PR?
misc/equal_null.rsand its registration.simplify()rewrites to the operator, which isSpark's
replacementfield, andinvoke_with_argscallsapply_cmpwith the same operator sothe function also works with the logical optimizer disabled, as the crate README requires for
Comet.
Two of the stub's commented-out queries were malformed, the porting script wrote one cast per
distinct
typeof()key, so a call with two identical literals lost an argument. The sameartifact affects 10 more pairs in 6 other spark files, left alone here.
Two divergences from Spark are left alone because they belong to the operator, not to this
function. Spark treats
-NaNandNaNas equal and DataFusion does not, which is true ofIS NOT DISTINCT FROMgenerally. Spark also rejects maps at analysis sinceMapTypeis notorderable, while
comparison_coercionhere accepts them.return_field_from_argsis overridden so the field is not nullable.normalize_float_zeronowrecurses into nested children.
compare_op_for_nestedandGroupValuesColumnboth normalizethrough it, so the scalar kernels and the two group-by paths agree on nested float keys.
Are these changes tested?
Yes,
spark/misc/equal_null.sltgoes from a skipped stub to 27 assertions covering the truthtable, float ordering, columns, arrays, structs, decimals and the arity errors.
Reverting
simplify()to plainEqfails 9 of them, so the file is not passing on constantfolding. Putting
invoke_with_argsback to a stub fails the two queries that run with theoptimizer off, and nothing else.
Are there any user-facing changes?
Yes.
equal_nullis new indatafusion-spark, and-0.0now matches+0.0inside nestedvalues, in comparisons and in
GROUP BY, matching the scalar kernels.