Skip to content

feat: add BinaryType support for SortMergeJoin - #5928

Open
xhumanoid wants to merge 2 commits into
apache:mainfrom
xhumanoid:fix-sortmergejoin-binary
Open

xhumanoid wants to merge 2 commits into
apache:mainfrom
xhumanoid:fix-sortmergejoin-binary

Conversation

@xhumanoid

Copy link
Copy Markdown
Contributor

BinaryType already supported on datafusion side, but still not enabled in comet

@github-actions github-actions Bot added the enhancement New feature or request label Sep 14, 2026

@peterxcli peterxcli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add sql test and drop the new scala test?

@xhumanoid
xhumanoid force-pushed the fix-sortmergejoin-binary branch from 0da3cd2 to 4e31347 Compare September 14, 2026 17:23
@xhumanoid

Copy link
Copy Markdown
Contributor Author

@peterxcli is it ok?

@peterxcli peterxcli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other lgtm

Comment thread spark/src/main/scala/org/apache/spark/sql/comet/operators.scala Outdated
Co-authored-by: Peter Lee <peterxcli@gmail.com>
@xhumanoid

Copy link
Copy Markdown
Contributor Author

@andygrove could you check, please

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants