Skip to content

Add dicts to aggregation fuzz testing - #16232

Merged
blaginin merged 4 commits into
apache:mainfrom
blaginin:fuzzy-dicts
Jun 4, 2025
Merged

Add dicts to aggregation fuzz testing#16232
blaginin merged 4 commits into
apache:mainfrom
blaginin:fuzzy-dicts

Conversation

@blaginin

Copy link
Copy Markdown
Member

Which issue does this PR close?

Related to #15871 (comment)

Rationale for this change

Adds dicts support for fuzzy testing when doing aggregation (to test prs like Related to #15871)

Are these changes tested?

Are there any user-facing changes?

No

@blagininblaginin self-assigned this Jun 2, 2025
@github-actionsgithub-actionsBot added the core Core DataFusion crate label Jun 2, 2025
@blaginin
blaginin marked this pull request as ready for review June 2, 2025 16:36
@alambalamb changed the title Add dicts to aggergation fuzzy testingAdd dicts to aggregation fuzz testingJun 4, 2025

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

Thank you @blaginin 🙏

Will this test fail intermittently due to ?

If not, does that mean the fuzz test doesn't cover that case?

UInt32Type,
values
),
DataType::UInt64 => generate_dict!(

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.

I think technically since only Dictionary(UInt64, Utf8) is made, we can avoid all the other cases in this match statement and save on codegen

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair, fixed in e0bdf97

@blaginin

Copy link
Copy Markdown
MemberAuthor

Thanks for the review!

Will this test fail intermittently?

It won't fail because of 0 here

https://github.com/apache/datafusion/pull/16232/files#diff-08d7a1f4d6a968c393a2a0f2a2f54118f38d6a29009ce31b261f3ca27a2d3396R733

Once #16228 is fixed, we'll be able to generate null values in both keys and values (will extend the ticket once this PR is merged)

Comment threaddatafusion/core/tests/fuzz_cases/record_batch_generator.rs
@alamb

alamb commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

Once #16228 is fixed, we'll be able to generate null values in both keys and values (will extend the ticket once this PR is merged)

Make sense -- that would be great context to add as a comment. I left a suggestion

blagininand others added 3 commits June 4, 2025 13:22
@blaginin
blaginin merged commit f513e2c into apache:mainJun 4, 2025
kosiew pushed a commit to kosiew/datafusion that referenced this pull request Jun 10, 2025
* Add dicts to fuzzy testing
* Add a note on `0.0`
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
* Remove unused key types
---------
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@blaginin@alamb