Conversation
peterxcli
left a comment
There was a problem hiding this comment.
could you add sql test and drop the new scala test?
0da3cd2 to
4e31347
Compare
|
@peterxcli is it ok? |
Co-authored-by: Peter Lee <peterxcli@gmail.com>
|
@andygrove could you check, please |
sunchao
left a comment
There was a problem hiding this comment.
Correctness
This removes the type-admission fallback for BinaryType equi-join keys. The existing serializer already maps binary to Arrow Binary, so the change lets the existing native sort-merge join handle those keys. Join-type selection, residual-filter gates, key serialization, ascending sort options and null equality are unchanged.
I traced the comparator through the pinned DataFusion 55.1.0 and Arrow 59.3.0 sources. Both native join implementations use JoinKeyComparator, which delegates binary values to Arrow's unsigned byte-slice comparison. This agrees with ByteArray.compareBinary in the maintained Spark 3.5/4.0 sources, including high-bit bytes, empty values and shorter prefixes. Comet continues to pass NullEqualsNothing, so null keys do not match each other. I found no P1/P2 issue in this path.
The SQL fixture addresses the request to replace the Scala test. It contains eleven answer-and-native-operator queries covering all six supported join types, a composite key, binary boundary values, nulls and duplicate groups larger than the configured batch size. Broadcast thresholds are disabled and sort-merge joins are preferred. The dictionary setting is tested in both configurations, although the actual array encoding reaching the join has not been observed. The resolved inline suggestion is also reflected in this head.
Validation limits
Reviewed head 2f4213c7 against base 0ea7bdb0. The current merge preview has parents [base, head]. Its production code, dependency lock and relevant test harness match the head. Its eight additional changes are unrelated SQL fixtures.
CI, CodeQL and the Delta gate are action_required, each with zero jobs. Only the labeling check passed. I have not run Spark/native tests or a benchmark locally. Canonical Spark 3.4/4.1/4.2 branches were unavailable, so no source or runtime coverage is claimed for those versions.
Performance
The added type match runs during planning. It introduces no per-row work, new conversion stage or separate binary implementation. The existing native comparator is cached per batch pair, and binary values use the existing byte comparison.
Enabling the native path may avoid fallback overhead, but the PR supplies no measured Spark-versus-Comet result. The SQL fixture establishes intended correctness coverage, not a speedup. I found no new avoidable hot-path work in this change. Could you add a focused Spark-versus-Comet microbenchmark with fixed-width binary keys and a duplicate-heavy case to quantify the benefit and check for regressions before making a performance claim?
Design
Extending the existing admission list is a direct fit for a type already supported by the serialization and native execution layers. It preserves Spark's join choice and the existing restrictions on unsupported key types and residual filters. A separate binary join operator or comparator would duplicate functionality already provided by Arrow.
Abstraction & complexity
The production change is limited to an import and one supported-type case. The SQL fixture uses the existing test harness and configuration matrix without introducing helpers or framework changes. No additional abstraction or required simplification emerged from the review.
BinaryType already supported on datafusion side, but still not enabled in comet