Skip to content

GH-50113: [C++][Python] Fix count for sliced union arrays#50114

Merged
pitrou merged 1 commit into
apache:mainfrom
fenfeng9:issue-50113-union-count-null-offsets
Jun 10, 2026
Merged

GH-50113: [C++][Python] Fix count for sliced union arrays#50114
pitrou merged 1 commit into
apache:mainfrom
fenfeng9:issue-50113-union-count-null-offsets

Conversation

@fenfeng9

@fenfeng9 fenfeng9 commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Sliced union arrays could report incorrect results from count.

For sliced union arrays, span.GetValues<int8_t>(1) and span.GetValues<int32_t>(2) already return offset-adjusted pointers. The logical null count path then indexed those pointers with span.offset + i, effectively applying the parent offset twice.
The sparse union path also checked child nullness with i instead of span.offset + i.

What changes are included in this PR?

  • Fixes logical null counting for sliced sparse and dense union arrays.
  • Add C++ and Python regression tests.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@fenfeng9 fenfeng9 requested review from AlenkaF, raulcd and rok as code owners June 6, 2026 16:01
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50113 has been automatically assigned in GitHub to PR creator.

@pitrou pitrou force-pushed the issue-50113-union-count-null-offsets branch from 1495b90 to 2d37d2b Compare June 10, 2026 14:57

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

+1, thanks a lot for this fix @fenfeng9

@pitrou

pitrou commented Jun 10, 2026

Copy link
Copy Markdown
Member

I've rebased from git main so as to incorporate #50108

@fenfeng9

Copy link
Copy Markdown
Contributor Author

Some tests failed, but they look unrelated to this PR.

@fenfeng9

Copy link
Copy Markdown
Contributor Author

And thanks for the review.

@pitrou

pitrou commented Jun 10, 2026

Copy link
Copy Markdown
Member

Some tests failed, but they look unrelated to this PR.

Indeed they do!

@pitrou pitrou merged commit fea7897 into apache:main Jun 10, 2026
51 of 55 checks passed
@pitrou pitrou removed the awaiting review Awaiting review label Jun 10, 2026
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit fea7897.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants