Add Aggregation fuzzer framework by Rachelint · Pull Request #12667 · apache/datafusion

Rachelint · 2024-09-28T19:38:29Z

Which issue does this PR close?

Rationale for this change

The aggregation fuzz tests maybe not enough, becasue there are so many optimizations in aggregation in datafusion.
For improving the testing, the first thing to do may be building a framework to make creating testcase easier.

This pr aimed to introduce such a framework.

What changes are included in this PR?

Design an AggregationFuzzer to make creating testcases for aggregation easier.
Two key components in AggregationFuzzer are DatasetGenerator and SessionContextGenerator.

In AggregationFuzzer(with a deinfed query represented by sql currently):

Multiple datasets will be generated by DatasetGenerator firstly.
And then for a specific dataset, multiple SessionContext with the random params about aggreagtion, and a baseline SessionContext will be generated.
Still for the specific dataset, each randomly generated SessionContext + basline result(generated from dataset + basline SessionContext) will be used to build AggregationFuzzTestTask.
In AggregationFuzzTestTask, we will run the query with dataset + related SessionContext, then we compare the result with basline result. If inconsistency found, report it and panic.
Finally, all AggregationFuzzTestTasks will be run in parallel in AggregationFuzzer

Are these changes tested?

Test by uts.

Are there any user-facing changes?

No,

alamb · 2024-09-29T11:56:17Z

😍 😍 😍 😍

Rachelint · 2024-10-03T18:00:16Z

Rest works:

Rewrite more cases through fuzzer
Refine the error handling
- Error of fuzzer self, just simple expect/unwrap
- Inconsistent results, println the task information, then panic

Rachelint · 2024-10-04T11:37:43Z

@alamb I found the new fuzz cases(based on aggregation fuzzer) failed after I rebase main.
I try to revert #12623 , and tests passed again, but I am still not sure what is wrong...

The failed new case: test_group_by_mixed_string_int64

I am more sure now, it may be due to the process of null, when I switch the field to non-nullabe, the test passed.

Updated
have found the reason, see detail in #12758
This pr may be ready now.

Rachelint · 2024-10-04T13:27:10Z

I found the reason and have submitted a pr to fix it #12758

alamb · 2024-10-04T20:02:15Z

I found the reason and have submitted a pr to fix it #12758

Testing for the win!

…erator.

Rachelint · 2024-10-08T15:21:28Z

The tests' run time after modifying data_gen_rounds and ctx_gen_rounds to both 16.

 Nextest run ID ef162b0e-5d2e-42dd-bf83-b6ccd08ff386 with nextest profile: default
Starting 40 tests across 1 binary
PASS [ 1.080s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_string_aggr_no_group
PASS [ 1.570s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_prim_aggr_no_group
PASS [ 1.512s] datafusion::fuzz fuzz_cases::aggregation_fuzzer::context_generator::test::test_generated_context
PASS [ 1.968s] datafusion::fuzz fuzz_cases::aggregation_fuzzer::data_generator::test::test_generated_datasets
PASS [ 3.496s] datafusion::fuzz fuzz_cases::distinct_count_string_fuzz::distinct_count_string_test
PASS [ 6.636s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_string_aggr_group_by_single_int64
PASS [ 11.075s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_prim_aggr_group_by_single_string
PASS [ 11.309s] datafusion::fuzz fuzz_cases::aggregate_fuzz::group_by_strings
PASS [ 11.404s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_string_aggr_group_by_single_string
PASS [ 11.426s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_prim_aggr_group_by_single_int64
PASS [ 9.577s] datafusion::fuzz fuzz_cases::join_fuzz::test_anti_join_1k
PASS [ 8.794s] datafusion::fuzz fuzz_cases::join_fuzz::test_anti_join_1k_filtered
PASS [ 16.672s] datafusion::fuzz fuzz_cases::aggregate_fuzz::streaming_aggregate_test
PASS [ 5.653s] datafusion::fuzz fuzz_cases::join_fuzz::test_inner_join_1k
PASS [ 11.194s] datafusion::fuzz fuzz_cases::join_fuzz::test_full_join_1k
PASS [ 18.353s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_prim_aggr_group_by_mixed_string_int64
PASS [ 7.364s] datafusion::fuzz fuzz_cases::join_fuzz::test_inner_join_1k_filtered
PASS [ 7.730s] datafusion::fuzz fuzz_cases::join_fuzz::test_left_join_1k
PASS [ 19.300s] datafusion::fuzz fuzz_cases::aggregate_fuzz::test_basic_string_aggr_group_by_mixed_string_int64
PASS [ 8.605s] datafusion::fuzz fuzz_cases::join_fuzz::test_full_join_1k_filtered
PASS [ 0.043s] datafusion::fuzz fuzz_cases::merge_fuzz::test_merge_2
PASS [ 0.069s] datafusion::fuzz fuzz_cases::merge_fuzz::test_merge_2_no_overlap
PASS [ 0.049s] datafusion::fuzz fuzz_cases::merge_fuzz::test_merge_3
PASS [ 0.037s] datafusion::fuzz fuzz_cases::merge_fuzz::test_merge_3_gaps
PASS [ 7.009s] datafusion::fuzz fuzz_cases::join_fuzz::test_left_join_1k_filtered
PASS [ 5.310s] datafusion::fuzz fuzz_cases::join_fuzz::test_right_join_1k
PASS [ 2.085s] datafusion::fuzz fuzz_cases::limit_fuzz::test_sort_topk_f64
PASS [ 1.723s] datafusion::fuzz fuzz_cases::limit_fuzz::test_sort_topk_i32
PASS [ 3.581s] datafusion::fuzz fuzz_cases::join_fuzz::test_semi_join_1k
PASS [ 0.164s] datafusion::fuzz fuzz_cases::sort_preserving_repartition_fuzz::sp_repartition_fuzz_tests::stream_merge_multi_order_preserve
PASS [ 3.120s] datafusion::fuzz fuzz_cases::join_fuzz::test_semi_join_1k_filtered
PASS [ 0.761s] datafusion::fuzz fuzz_cases::window_fuzz::bounded_window_causal_non_causal
PASS [ 5.374s] datafusion::fuzz fuzz_cases::join_fuzz::test_right_join_1k_filtered
PASS [ 3.533s] datafusion::fuzz fuzz_cases::limit_fuzz::test_sort_topk_str
PASS [ 3.914s] datafusion::fuzz fuzz_cases::limit_fuzz::test_sort_topk_i64str
PASS [ 6.426s] datafusion::fuzz fuzz_cases::sort_fuzz::test_sort_100k_mem
PASS [ 6.416s] datafusion::fuzz fuzz_cases::sort_preserving_repartition_fuzz::sp_repartition_fuzz_tests::sort_preserving_repartition_test
PASS [ 7.181s] datafusion::fuzz fuzz_cases::sort_fuzz::test_sort_unlimited_mem
PASS [ 7.971s] datafusion::fuzz fuzz_cases::sort_fuzz::test_sort_1k_mem
PASS [ 12.769s] datafusion::fuzz fuzz_cases::window_fuzz::window_bounded_window_random_comparison

Rachelint · 2024-10-08T18:54:05Z

@alamb I have added tests to cover more aggregations:

basic prim aggr(sum/sum distinct/max/min/count/avg)
basic string aggr(count/count distinct/min/max)

I make it through the simple implementation about randomly select sql memtioned in #12667 (comment)

it will randomly select a sql for each dataset
then run CTX_GEN_ROUNDS(now 16) randomly SessionConfig on specific dataset + selected query

Now the testcase may be like:

// Build fuzzerlet fuzzer = builder
.data_gen_config(data_gen_config).data_gen_rounds(16).add_sql("SELECT sum(a) FROM fuzz_table").add_sql("SELECT sum(distinct a) FROM fuzz_table").add_sql("SELECT max(a) FROM fuzz_table").add_sql("SELECT min(a) FROM fuzz_table").add_sql("SELECT count(a) FROM fuzz_table").add_sql("SELECT count(distinct a) FROM fuzz_table").add_sql("SELECT avg(a) FROM fuzz_table").table_name("fuzz_table").build();

And unforunately, seems a bug about min/max for string found... Maybe we should check and try to fix it in other pr.

alamb · 2024-10-08T20:59:09Z

And unforunately, seems a bug about min/max for string found... Maybe we should check and try to fix it in other pr.

Yes, let's handle that as a separate PR / issue.

alamb

Thank you again @Rachelint -- I think this is great and I'll plan to merge it tomorrow unless anyone else would like time to review

Then we can work on follow on items in other PRs

alamb · 2024-10-08T21:01:46Z

+}
+
+/// Fuzz test for group by `sting + int64`
+#[tokio::test(flavor = "multi_thread")]


I meant like
#[tokio::test] async fn test_basic_string_aggr_group_by_mixed_string_int64_1() { ... } #[tokio::test] async fn test_basic_string_aggr_group_by_mixed_string_int64_2() { ... }
Rather than a single test that was multi-threaded

alamb · 2024-10-09T14:07:12Z

Thanks again @Rachelint -- this is so great

alamb · 2024-10-09T14:09:28Z

I am working on filing the error that this found for min/max

alamb · 2024-10-09T14:25:39Z

I also have some ideas on how to improve the error reporting here

Rachelint · 2024-10-09T14:27:39Z

I also have some ideas on how to improve the error reporting here

Nice, the error reporting is indeed not so readable currently

Rachelint · 2024-10-09T14:28:12Z

I also have some ideas on how to improve the error reporting here

Maybe #5131 will help? The error seems similar.

alamb · 2024-10-09T14:49:14Z

#12832 has a proposal for improved error reporting

alamb · 2024-10-09T15:49:09Z

+ let fuzzer = builder
+ .data_gen_config(data_gen_config)
+ .data_gen_rounds(8)
+ // FIXME: Encounter error in min/max


Proposed fix in #12834

alamb · 2024-10-10T12:01:15Z

BTW this found another bug in #12792

See https://github.com/apache/datafusion/actions/runs/11272685143/job/31348663515?pr=12792

❤️

github-actionsBot added the core Core DataFusion crate label Sep 28, 2024

Rachelint force-pushed the aggr-fuzzer branch 2 times, most recently from d1f5aea to 6ad27f5Compare October 3, 2024 14:59

Rachelint force-pushed the aggr-fuzzer branch 3 times, most recently from b059373 to cf82eb7Compare October 4, 2024 10:37

Rachelint changed the title ~~[WIP] Aggregation fuzzer~~Aggregation fuzzerOct 4, 2024

Rachelint mentioned this pull request Oct 4, 2024
fix equal_to in PrimitiveGroupValueBuilder#12758
Merged

Rachelint marked this pull request as ready for review October 4, 2024 13:26

Rachelint added 16 commits October 5, 2024 23:14

impl primitive arrays generator.
6514cd2

sort out the test record batch generating codes.
1a11133

draft for DataSetsGenerator.
e0ea349

tmp
c952bdf

improve the data generator, and start to impl the session context gen…
214d67f
…erator.

impl context generator.
04b4246

tmp
6b2af7f

define the AggregationFuzzer.
77d2268

add ut for data generator.
4bef192

improve comments for SessionContextGenerator.
e7fbf47

define GeneratedSessionContextBuilder to reduce repeated codes.
984f6aa

extract the check equality logic for reusing.
12e3f37

add ut for SessionContextGenerator.
ca4a40c

tmp
a4639de

finish the main logic of AggregationFuzzer.
0cfd035

try to rewrite some test using the fuzzer.
8271079

Rachelint added 7 commits October 8, 2024 23:27

improve comments for rounds constants.
d5ff6ec

small improvements.
4b18d53

select sql from some candidates ranther than fixed one.
fbf3a6e

make data_gen_rounds able to set again, and add more tests.
79b0734

add no group cases.
ca36a88

add fuzz test for basic string aggr.
ea5e80b

make data_gen_rounds smaller.
7f08f2b

Rachelint force-pushed the aggr-fuzzer branch from 12e3bcc to 7f08f2bCompare October 8, 2024 18:40

add comments.
9b0005b

Rachelint added 2 commits October 9, 2024 02:57

fix typo.
8040dc3

fix comment.
5c90a6b

alamb approved these changes Oct 8, 2024
View reviewed changes

alamb merged commit 3353c06 into apache:mainOct 9, 2024

Rachelint deleted the aggr-fuzzer branch October 9, 2024 14:26

alamb mentioned this pull request Oct 9, 2024
Improve AggregationFuzzer error reporting #12832
Merged

alamb mentioned this pull request Oct 9, 2024
Error in min/max queries: InvalidArgumentError("number of columns(1) must match number of fields(2) in schema" #12833
Closed

alamb reviewed Oct 9, 2024
View reviewed changes

This was referenced Oct 9, 2024
Aggregation fuzz testing #12114
Closed
Implement special min/max accumulator for Strings and Binary (10% faster for Clickbench Q28) #12792
Merged

jayzhan211 mentioned this pull request Oct 13, 2024
Make AggregationFuzzer reproducible #12903
Open

Uh oh!

Conversation

Rachelint commented Sep 28, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Sep 29, 2024

Uh oh!

Rachelint commented Oct 3, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rachelint commented Oct 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rachelint commented Oct 4, 2024

Uh oh!

alamb commented Oct 4, 2024

Uh oh!

Rachelint commented Oct 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rachelint commented Oct 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Oct 8, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alambOct 8, 2024

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 9, 2024

Uh oh!

alamb commented Oct 9, 2024

Uh oh!

alamb commented Oct 9, 2024

Uh oh!

Rachelint commented Oct 9, 2024

Uh oh!

Rachelint commented Oct 9, 2024

Uh oh!

alamb commented Oct 9, 2024

Uh oh!

alambOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rachelint commented Sep 28, 2024•
edited
Loading

Rachelint commented Oct 3, 2024•
edited
Loading

Rachelint commented Oct 4, 2024•
edited
Loading

Rachelint commented Oct 8, 2024•
edited
Loading

Rachelint commented Oct 8, 2024•
edited
Loading