Skip to content

fix: copy rows retained by identity broadcasts - #7260

Merged
sunchao merged 1 commit into
apache:mainfrom
pingzh:pingzh-copy-identity-broadcast-rows
Oct 27, 2026
Merged

sunchao merged 1 commit into
apache:mainfrom
pingzh:pingzh-copy-identity-broadcast-rows

Conversation

@pingzh

@pingzh pingzh commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #6741. First of four smaller changes replacing #7117. The expression safeguards are tracked separately in #7261; both PRs target main independently.

Rationale for this change

An identity broadcast retains every input row, but the JVM columnar-to-row converters reuse their row object. Materializing that iterator can therefore repeat the last row of each batch instead of preserving the build rows.

What changes are included in this PR?

Copy each row when adapting a Comet broadcast to IdentityBroadcastMode. Hashed broadcast modes already copy row data into their own storage. This fix is independent of the make_date evaluation changes.

How are these changes tested?

A focused regression directly converts an identity broadcast containing four distinct rows across two partitions. It verifies retained values and cardinality with the direct JVM converter enabled and disabled.

Full-reactor JVM and test-source compilation passes on Spark 4.1 / JDK 17, including formatting and style checks. Runtime execution is blocked because the configured native dependency registry does not contain the locked aws-config 1.13.0 package.

@github-actions github-actions Bot added bug Something isn't working area:ffi Arrow FFI / JNI boundary labels Oct 27, 2026

@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.

LGTM. Reviewed 52c6067 against 01fc37a with five independent reviewers, including Spark semantics, row ownership, planner paths, tests, and an adversarial pass. No actionable issue found.

The copy happens before either converter reuses the UnsafeRow, preserving the complete row storage for identity broadcasts. Hashed broadcasts remain unchanged. Release branch-1.2 has the same original bug, so this is an intended correctness fix there too.

The new regression passed with both converters in the Spark 4.1 execution job, alongside 1,780 passing tests. That job tested merge ff9f7c3, whose tree matches the reviewed head. A separate Spark 4.1.3 ownership probe reproduced repeated rows without copying and distinct rows with copying, including strings, nested structs, arrays, and nulls. Broader Spark SQL jobs were skipped; no full local Comet runtime suite was run.

@sunchao
sunchao added this pull request to the merge queue Oct 27, 2026
Merged via the queue into apache:main with commit 84944e9 Oct 27, 2026
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ffi Arrow FFI / JNI boundary backport-1.2 bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants