Skip to content

Comments

Add dicts to aggregation fuzz testing#16232

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

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

Conversation

@blaginin
Copy link
Collaborator

@blaginin blaginin commented Jun 2, 2025

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

@blaginin blaginin self-assigned this Jun 2, 2025
@github-actions github-actions bot 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
@alamb alamb changed the title Add dicts to aggergation fuzzy testing Add dicts to aggregation fuzz testing Jun 4, 2025
Copy link
Contributor

@alamb alamb left a comment

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
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
Collaborator Author

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
Collaborator Author

blaginin commented Jun 4, 2025

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)

@alamb
Copy link
Contributor

alamb commented Jun 4, 2025

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

@blaginin blaginin merged commit f513e2c into apache:main Jun 4, 2025
27 checks passed
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 <[email protected]>

* Remove unused key types

---------

Co-authored-by: Andrew Lamb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants