Skip to content

fix: support zero-field struct group keys in aggregate emit - #25282

Open
fornwall wants to merge 1 commit into
apache:mainfrom
fornwall:panic-fix
Open

fornwall wants to merge 1 commit into
apache:mainfrom
fornwall:panic-fix

Conversation

@fornwall

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #25281.

Rationale for this change

GROUP BY and SELECT DISTINCT over a group key whose type contains a zero-field struct (Struct()) panicked when the aggregate emitted:

select arrow_cast(null, 'Struct()') as s, count(*) from (values (1),(2),(3)) group by 1;
-- thread 'tokio-rt-worker' panicked ... dictionary re-encode during emit:
-- InvalidArgumentError("use StructArray::try_new_with_length or StructArray::new_empty_fields ...")

The same panic hit SELECT DISTINCT and keys where the empty struct is nested inside another struct. A zero-field struct is a valid Arrow type and should group like any other key.

What changes are included in this PR?

encode_array_if_necessary in group_values/row.rs rebuilt every struct column with StructArray::try_new, which arrow rejects for zero fields since there is no child column to derive the length from. It now uses StructArray::try_new_with_length with the source array's length, so zero-field structs are rebuilt like any other struct. This function is shared by GroupValuesRows and the row-backed GroupColumn, so both paths are covered.

What is the testing strategy for this PR?

Three new sqllogictest cases at the end of struct.slt (whole-key, DISTINCT, and nested inside named_struct). All three panicked before the fix and pass after it.

Are there any user-facing changes?

No, other than the panic being fixed.

`GROUP BY` / `SELECT DISTINCT` over a key containing a zero-field struct
(`Struct()`) panicked when the aggregate emitted its groups. The row-format
decode path rebuilt each struct column with `StructArray::try_new`, which
arrow rejects for zero fields because it cannot infer the length from a
child column.

Use `StructArray::try_new_with_length`, passing the source array's length,
so zero-field structs are rebuilt like any other struct.

Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Sep 13, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.90%. Comparing base (15f32dd) to head (f2dd806).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25282      +/-   ##
==========================================
- Coverage   81.90%   81.90%   -0.01%     
==========================================
  Files        1134     1134              
  Lines      425200   425201       +1     
  Branches   425200   425201       +1     
==========================================
- Hits       348268   348251      -17     
- Misses      56291    56304      +13     
- Partials    20641    20646       +5     

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

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

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

panic: GROUP BY or SELECT DISTINCT over a zero-field (empty) struct key

2 participants