Skip to content

fix(common): support empty struct in ScalarValue::compact and new_default - #24582

Merged
Jefffrey merged 2 commits into
apache:mainfrom
unikdahal:fix-scalar-compact-empty-struct
Aug 30, 2026
Merged

fix(common): support empty struct in ScalarValue::compact and new_default#24582
Jefffrey merged 2 commits into
apache:mainfrom
unikdahal:fix-scalar-compact-empty-struct

Conversation

@unikdahal

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#24581.

Rationale for this change

StructArray::new panics for structs with zero fields because it cannot infer array length from an empty column list. This hit two code paths in ScalarValue: new_default when building a default value for a zero-field struct type, and compact_view_buffers (used by ScalarValue::compact) when compacting a struct array that has zero fields but a nonzero row count.

What changes are included in this PR?

  • ScalarValue::new_default and compact_view_buffers now special-case zero-field structs, using StructArray::new_empty_fields (which takes an explicit length/nulls instead of inferring it from columns) rather than StructArray::new.
  • Added test_compact_empty_struct covering the panic case.

Are these changes tested?

Yes, new unit test added.

Are there any user-facing changes?

No behavior change other than fixing a panic on empty-field structs.

@github-actionsgithub-actionsBot added the common Related to common crate label Aug 22, 2026

@JefffreyJefffrey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CI looks to be failing

Comment threaddatafusion/common/src/scalar/mod.rs Outdated
Comment on lines +5045 to +5048
Arc::new(StructArray::new_empty_fields(
s.len(),
s.nulls().cloned(),
))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could probably clone the input instead of reconstructing?

Comment threaddatafusion/common/src/scalar/mod.rs Outdated
Comment on lines +11626 to +11631
let nulls = NullBuffer::from(vec![true, false, true]);
let empty_struct = Arc::new(StructArray::new_empty_fields(3, Some(nulls)));
let mut scalar = ScalarValue::Struct(empty_struct);

// Before fix: panics inside compact_view_buffers calling StructArray::new on 0 fields
scalar.compact();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems a bit odd that a scalar would have 3 rows

@codecov-commenter

codecov-commenter commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.59259% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.42%. Comparing base (5134a1a) to head (6cd684f).
⚠️ Report is 59 commits behind head on main.

Files with missing linesPatch %Lines
datafusion/common/src/scalar/mod.rs92.59%4 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #24582 +/- ##
==========================================
+ Coverage 81.38% 81.42% +0.04% 
==========================================
Files 1116 1121 +5 Lines 397960 402202 +4242 Branches 397960 402202 +4242 ==========================================
+ Hits 323880 327502 +3622 - Misses 55120 55498 +378 - Partials 18960 19202 +242 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@unikdahal

unikdahal commented Aug 27, 2026

Copy link
Copy Markdown
ContributorAuthor

Thanks @Jefffrey!

Empty struct handling: Since zero-field structs don't have child buffers to compact, compact_view_buffers now just returns the input array directly.

Test update: Switched test_compact_empty_struct from the 3-row scalar to a single-row StructArray (kept a null row so we still verify null buffer handling).

CI: Fixed the cargo fmt issue in the new nested test.

@Jefffrey
Jefffrey added this pull request to the merge queueAug 30, 2026
@Jefffrey

Copy link
Copy Markdown
Contributor

thanks @unikdahal

Merged via the queue into apache:main with commit 74f4dd6Aug 30, 2026
38 checks passed
timsaucer added a commit that referenced this pull request Sep 2, 2026
… and new_default (#24582) (#24876)
This is a back port of #24582 into `branch-55` for inclusion in 55.1.0.
Please see the original PR for details.
Co-authored-by: Unik Dahal <61407386+unikdahal@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commonRelated to common crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

panic: 'StructArray::new' on zero-field (empty) structs in ScalarValue::compact and ScalarValue::new_default

3 participants

@unikdahal@codecov-commenter@Jefffrey