Skip to content

Add FixedSizeBinary support for MultiGroupBy#23646

Open
maxburke wants to merge 1 commit into
apache:mainfrom
urbanlogiq:multi-group-by-fsb
Open

Add FixedSizeBinary support for MultiGroupBy#23646
maxburke wants to merge 1 commit into
apache:mainfrom
urbanlogiq:multi-group-by-fsb

Conversation

@maxburke

@maxburke maxburke commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Closes #23645

Rationale for this change

Multi-Group-By has cases for regular Binary/LargeBinary types, but not FixedSizeBinary

Are these changes tested?

Yes.

Are there any user-facing changes?

No

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jul 16, 2026
@codecov-commenter

codecov-commenter commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.92761% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.67%. Comparing base (4957f5d) to head (5eb8979).

Files with missing lines Patch % Lines
...s/group_values/multi_group_by/fixed_size_binary.rs 98.71% 3 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #23646      +/-   ##
==========================================
+ Coverage   80.66%   80.67%   +0.01%     
==========================================
  Files        1087     1088       +1     
  Lines      367409   367782     +373     
  Branches   367409   367782     +373     
==========================================
+ Hits       296367   296726     +359     
- Misses      53390    53400      +10     
- Partials    17652    17656       +4

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

alamb
alamb reviewed Jul 16, 2026
View reviewed changes

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

Thanks @maxburke -- what is the use case for this query? Is it that you are grouping on multiple columns and if that also includes a fixed size binary the performance drops off?

@alamb

alamb commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Related discussion

@alamb

alamb commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

There is also this PR

However I don't think that will handle FixedSizeBinary

@alamb alamb mentioned this pull request Jul 16, 2026
Merged
Rich-T-kid
Rich-T-kid reviewed Jul 16, 2026
View reviewed changes

@Rich-T-kid Rich-T-kid 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.

This PR looks very reasonable. I noticed that this is very similar to the ByteGroupValueBuilder implementation.
I think it would be worth it to try and see if we can modify ByteGroupValueBuilder to support similar optimizations such as batched writes. besides that vectorized/equal_to and the other trait methods are 1-1.
If ByteGroupValueBuilder cant be expanded on to include fixed size binary without extensive branching that hurts readability thats also fine.

Comment thread datafusion/physical-plan/src/aggregates/group_values/multi_group_by/fixed_size_binary.rs Outdated

impl GroupColumn for FixedSizeBinaryGroupValueBuilder {
fn equal_to(&self, lhs_row: usize, array: &ArrayRef, rhs_row: usize) -> bool {
debug_assert!(matches!(array.data_type(), DataType::FixedSizeBinary(_)));

@Rich-T-kid Rich-T-kid Jul 16, 2026

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.

the array input is guaranteed to be the correct type. you can remove the assertion here

@maxburke maxburke Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Comment thread datafusion/physical-plan/src/aggregates/group_values/multi_group_by/fixed_size_binary.rs Outdated
rhs_rows: &[usize],
equal_to_results: &mut BooleanBufferBuilder,
) {
debug_assert!(matches!(array.data_type(), DataType::FixedSizeBinary(_)));

@Rich-T-kid Rich-T-kid Jul 16, 2026

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.

similar point to above

@maxburke maxburke Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Comment thread datafusion/physical-plan/src/aggregates/group_values/multi_group_by/fixed_size_binary.rs
@alamb

alamb commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

This PR looks very reasonable. I noticed that this is very similar to the ByteGroupValueBuilder implementation. I think it would be worth it to try and see if we can modify ByteGroupValueBuilder to support similar optimizations such as batched writes. besides that vectorized/equal_to and the other trait methods are 1-1. If ByteGroupValueBuilder cant be expanded on to include fixed size binary without extensive branching that hurts readability thats also fine.

This sounds like a good thing to explore in a follow on / parallel PR

@alamb

alamb commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Since I think the point of doing this PR is to improve performance it would be nice to have some benchmark results

I created a PR with some benchmarks here

Hopefully we can merge that and then use it to validate that this PR improves things

@maxburke

maxburke commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @maxburke -- what is the use case for this query? Is it that you are grouping on multiple columns and if that also includes a fixed size binary the performance drops off?

More so that if we're grouping by a FixedSizeBinary column we run out of memory in the GroupValuesRows interning and crash :)

@maxburke
maxburke force-pushed the multi-group-by-fsb branch from e843517 to f8ebf73 Compare July 16, 2026 20:49
timsaucer pushed a commit to saadtajwar/datafusion that referenced this pull request Jul 17, 2026
@alamb @claude
bench: add FixedSizeBinary coverage to multi_group_by benchmark (apac…
4957f5d 
…he#23650)

## Which issue does this PR close?

- Related to apache#23646 
- Related to apache#23645.

## Rationale for this change

The point of a specialized FixedSizeBinary group values is performance
but we have no performance benchmark for it.

## What changes are included in this PR?

Adds a `fixed_size_binary` experiment to
`datafusion/physical-plan/benches/multi_group_by.rs` 
Run with:

```bash
cargo bench -p datafusion-physical-plan --bench multi_group_by --features test_utils -- fixed_size_binary
```
## Are these changes tested?

This is benchmark-only. The benchmark compiles on `main` and runs
end-to-end
on top of apache#23646. No product code changes.

## Are there any user-facing changes?

No.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alamb

alamb commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Can you please merge this PR up from main (so I can run the benchmarks using the bot?) I can't do it myself b/c I don't have write permissions int he urbanlogiq repo

@maxburke

maxburke commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Can you please merge this PR up from main (so I can run the benchmarks using the bot?) I can't do it myself b/c I don't have write permissions int he urbanlogiq repo

done!

@maxburke
maxburke force-pushed the multi-group-by-fsb branch from 3970070 to 5eb8979 Compare July 17, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb left review comments
+1 more reviewer
@Rich-T-kid Rich-T-kid Rich-T-kid left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multi-Group-By does not support FixedSizeBinary types

4 participants

@maxburke @codecov-commenter @alamb @Rich-T-kid