You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "blaginin (via GitHub)" <gi...@apache.org> on 2025/06/02 15:05:41 UTC
[PR] Add dicts to aggergation fuzzy testing [datafusion]
blaginin opened a new pull request, #16232:
URL: https://github.com/apache/datafusion/pull/16232
## Which issue does this PR close?
Related to https://github.com/apache/datafusion/pull/15871#discussion_r2112273596
## Rationale for this change
Adds dicts support for fuzzy testing when doing aggregation (to test prs like Related to https://github.com/apache/datafusion/pull/15871)
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add dicts to aggregation fuzz testing [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16232:
URL: https://github.com/apache/datafusion/pull/16232#discussion_r2126344160
##########
datafusion/core/tests/fuzz_cases/record_batch_generator.rs:
##########
@@ -656,11 +713,110 @@ impl RecordBatchGenerator {
self,
num_rows,
max_num_distinct,
+ null_pct,
batch_gen_rng,
array_gen_rng,
BooleanType
}
}
+ DataType::Dictionary(ref key_type, ref value_type)
+ if key_type.is_dictionary_key_type() =>
+ {
+ // We generate just num_distinct values because they will be reused by different keys
+ let mut array_gen_rng = array_gen_rng;
+
+ let values = Self::generate_array_of_type_inner(
+ &ColumnDescr::new("values", *value_type.clone()),
+ num_distinct,
+ batch_gen_rng,
+ array_gen_rng.clone(),
+ 0.0, // null values are generated on the key level
+ );
+
+ match key_type.as_ref() {
+ DataType::Int8 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int8Type,
+ values
+ ),
+ DataType::Int16 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int16Type,
+ values
+ ),
+ DataType::Int32 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int32Type,
+ values
+ ),
+ DataType::Int64 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int64Type,
+ values
+ ),
+ DataType::UInt8 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ UInt8Type,
+ values
+ ),
+ DataType::UInt16 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ UInt16Type,
+ values
+ ),
+ DataType::UInt32 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ UInt32Type,
+ values
+ ),
+ DataType::UInt64 => generate_dict!(
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add dicts to aggregation fuzz testing [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #16232:
URL: https://github.com/apache/datafusion/pull/16232#discussion_r2126542789
##########
datafusion/core/tests/fuzz_cases/record_batch_generator.rs:
##########
@@ -656,11 +713,110 @@ impl RecordBatchGenerator {
self,
num_rows,
max_num_distinct,
+ null_pct,
batch_gen_rng,
array_gen_rng,
BooleanType
}
}
+ DataType::Dictionary(ref key_type, ref value_type)
+ if key_type.is_dictionary_key_type() =>
+ {
+ // We generate just num_distinct values because they will be reused by different keys
+ let mut array_gen_rng = array_gen_rng;
+
+ let values = Self::generate_array_of_type_inner(
+ &ColumnDescr::new("values", *value_type.clone()),
+ num_distinct,
+ batch_gen_rng,
+ array_gen_rng.clone(),
+ 0.0, // null values are generated on the key level
+ );
+
+ match key_type.as_ref() {
+ DataType::Int8 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int8Type,
+ values
+ ),
+ DataType::Int16 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int16Type,
+ values
+ ),
+ DataType::Int32 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int32Type,
+ values
+ ),
+ DataType::Int64 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ Int64Type,
+ values
+ ),
+ DataType::UInt8 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ UInt8Type,
+ values
+ ),
+ DataType::UInt16 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ UInt16Type,
+ values
+ ),
+ DataType::UInt32 => generate_dict!(
+ self,
+ num_rows,
+ num_distinct,
+ null_pct,
+ batch_gen_rng,
+ array_gen_rng,
+ UInt32Type,
+ values
+ ),
+ DataType::UInt64 => generate_dict!(
Review Comment:
Fair, fixed in https://github.com/apache/datafusion/pull/16232/commits/e0bdf974374e57d5e860a5a337efa8ff2ccfbd52
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add dicts to aggregation fuzz testing [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16232:
URL: https://github.com/apache/datafusion/pull/16232#discussion_r2126366811
##########
datafusion/core/tests/fuzz_cases/record_batch_generator.rs:
##########
@@ -656,11 +713,110 @@ impl RecordBatchGenerator {
self,
num_rows,
max_num_distinct,
+ null_pct,
batch_gen_rng,
array_gen_rng,
BooleanType
}
}
+ DataType::Dictionary(ref key_type, ref value_type)
+ if key_type.is_dictionary_key_type() =>
+ {
+ // We generate just num_distinct values because they will be reused by different keys
+ let mut array_gen_rng = array_gen_rng;
+
+ let values = Self::generate_array_of_type_inner(
+ &ColumnDescr::new("values", *value_type.clone()),
+ num_distinct,
+ batch_gen_rng,
+ array_gen_rng.clone(),
+ 0.0, // null values are generated on the key level
Review Comment:
```suggestion
// Once https://github.com/apache/datafusion/issues/16228 is fixed
// we can also generate nulls in values
0.0, // null values are generated on the key level
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add dicts to aggregation fuzz testing [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16232:
URL: https://github.com/apache/datafusion/pull/16232#issuecomment-2939665729
> Once https://github.com/apache/datafusion/issues/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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add dicts to aggregation fuzz testing [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin merged PR #16232:
URL: https://github.com/apache/datafusion/pull/16232
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add dicts to aggregation fuzz testing [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on PR #16232:
URL: https://github.com/apache/datafusion/pull/16232#issuecomment-2939655137
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 https://github.com/apache/datafusion/issues/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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org