-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add aggregations over distinct inputs to AggregationFuzzer #8328
Conversation
✅ Deploy Preview for meta-velox canceled.
|
8f23e52
to
27839ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duanmeng Meng, thank you for working on this extension to the Fuzzer. Some initial questions. Have you found any bugs using this extension?
@mbasmanova Hi Masha, I've run for 3600s using this PR's version and changed the ratio of distinct aggregation testing to 90% ( I20240111 00:39:32.758095 907797 AggregationFuzzer.cpp:456] ==============================> Done with iteration 326
I20240111 00:39:32.758114 907797 AggregationFuzzer.cpp:1148] Total functions tested: 7
I20240111 00:39:32.758121 907797 AggregationFuzzer.cpp:1149] Total masked aggregations: 5 (1.53%)
I20240111 00:39:32.758127 907797 AggregationFuzzer.cpp:1151] Total global aggregations: 2 (0.61%)
I20240111 00:39:32.758131 907797 AggregationFuzzer.cpp:1153] Total group-by aggregations: 31 (9.48%)
I20240111 00:39:32.758136 907797 AggregationFuzzer.cpp:1155] Total distinct aggregations: 292 (89.30%)
I20240111 00:39:32.758141 907797 AggregationFuzzer.cpp:1157] Total aggregations over sorted inputs: 7 (2.14%)
I20240111 00:39:32.758144 907797 AggregationFuzzer.cpp:1159] Total window expressions: 2 (0.61%)
I20240111 00:39:32.758148 907797 AggregationFuzzer.cpp:1161] Total aggregations verified against reference DB: 132 (40.37%)
I20240111 00:39:32.758152 907797 AggregationFuzzer.cpp:1163] Total aggregations not verified (non-deterministic function / not supported by reference DB / reference DB failed): 318 (97.25%) / 407 (124.46%) / 52 (15.90%)
I20240111 00:39:32.758157 907797 AggregationFuzzer.cpp:1168] Total failed aggregations: 3 (0.92%)
[==========] Running 0 tests from 0 test suites.
[==========] 0 tests from 0 test suites ran. (0 ms total)
[ PASSED ] 0 tests. |
@mbasmanova Yes, I found some issues during the implementation and testing.
-- Aggregation[SINGLE [g0, g1] a0 := count(ROW["c0"]) distinct] -> g0:ARRAY<BIGINT>, g1:MAP<VARBINARY,SMALLINT>, a0:BIGINT
-- TableScan[table: hive_table] -> g0:ARRAY<BIGINT>, g1:MAP<VARBINARY,SMALLINT>, c0:INTEGER, c1:INTEGER
I20240110 17:03:27.754366 698605 Task.cpp:1082] All drivers (2) finished for task test_cursor 22 after running for 44 ms.
I20240110 17:03:27.754391 698605 Task.cpp:1766] Terminating task test_cursor 22 with state Finished after running for 44 ms.
/Users/macduan/work/native/velox-8/velox/exec/tests/utils/QueryAssertions.cpp:1104: Failure
Failed
Expected 897, got 898
2 extra rows, 1 missing rows
2 of extra rows:
[] | [] | 2
[] | [] | 2
1 of missing rows:
[] | [] | 4
|
@duanmeng Thank you for sharing additional context. Would be nice to include that in the PR description. |
Interestingly CI failure is in the fuzzer:
|
Do these need to be orderable or would it be sufficient to have them comparable? Sounds like DistinctAggregations are missing a check for types of distinct inputs. We should get a clear failure when applying distinct to non-comparable types. |
6947a0c
to
1b67f38
Compare
@mbasmanova I've updated the PR, and rerun the fuzzer for a while and the count distinct issue is not reproduced.
|
Distinct like Sorted aggregations must run single-threaded. Is this what you observed? The PR description as written sounds like you don't know why the fuzzer fails, so you just change number of drivers to 1. |
@mbasmanova Looks like it does not merge the two drivers' results? I will keep resolving this first. |
@mbasmanova Hi Masha, I think I know the root cause. In the fuzzer if we only use |
bd0537a
to
bb36e24
Compare
@duanmeng Like I mentioned earlier, similar to sorted aggregations, distinct aggregation must run single-threaded or data must be partitioned on group-by keys among threads. You can achieve that by either setting maxDrivers = 1 or by adding local partition on grouping keys before the aggregation. |
Distinct aggregations do not support spilling yet. See #7791 |
@mbasmanova Hi Masha, thanks for your guidance, I've used the |
auto call = makeFunctionCall(signature.name, argNames, sortedInputs); | ||
// Exclude approx_xxx, merge aggregations since it conflicts with | ||
// distinct semantics. | ||
// TODO: Add a exclude list for aggregations that cannot be supported by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
@@ -684,34 +730,100 @@ void resetCustomVerifiers( | |||
} | |||
} | |||
|
|||
void initVerifier( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming: initializeVerifiers
Is this a pure refactoring? Maybe extract refactoring of existing code into a separate PR to make reviewing easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I refactored this part along the the checkResults
. I will restore this only left the checkResults
.
} | ||
} // namespace | ||
|
||
bool AggregationFuzzer::checkResult( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming: checkResult name suggests that this function checks some results, however, it runs plans and compares the results. Can we think of a more descriptive name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compareEquivalentPlanResults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function actually executes the plans, not only compares the results. Maybe, assertEquivalentPlanResults?
1f8b276
to
f581f34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duanmeng This is shaping up nicely. Would you share stats from running the modified fuzzer? These are printed at the end of the run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @duanmeng, thank you for extending AggregationFuzzer to cover distinct aggregations! I left a few comments.
42088f8
to
d44912f
Compare
@mbasmanova Hi Masha, I've resolved your comments with the latest codes, and ran the fuzzer locally for 360s (I will run it nightly for 3600s) the stats are as follows. Could you please help to take a look? Thanks. I20240118 23:33:37.139185 1331215 AggregationFuzzer.cpp:475] ==============================> Done with iteration 60
I20240118 23:33:37.139217 1331215 AggregationFuzzer.cpp:1023] Total functions tested: 22
I20240118 23:33:37.139222 1331215 AggregationFuzzer.cpp:1024] Total masked aggregations: 9 (14.75%)
I20240118 23:33:37.139232 1331215 AggregationFuzzer.cpp:1026] Total global aggregations: 5 (8.20%)
I20240118 23:33:37.139236 1331215 AggregationFuzzer.cpp:1028] Total group-by aggregations: 43 (70.49%)
I20240118 23:33:37.139241 1331215 AggregationFuzzer.cpp:1030] Total distinct: 8 (13.11%)
I20240118 23:33:37.139245 1331215 AggregationFuzzer.cpp:1032] Total distinct aggregations: 4 (6.56%)
I20240118 23:33:37.139250 1331215 AggregationFuzzer.cpp:1034] Total aggregations over sorted inputs: 4 (6.56%)
I20240118 23:33:37.139253 1331215 AggregationFuzzer.cpp:1036] Total window expressions: 5 (8.20%)
I20240118 23:33:37.139257 1331215 AggregationFuzzer.cpp:1038] Total aggregations verified against reference DB: 7 (11.48%)
I20240118 23:33:37.151767 1331215 AggregationFuzzer.cpp:1040] Total aggregations not verified (non-deterministic function / not supported by reference DB / reference DB failed): 33 (54.10%) / 17 (27.87%) / 0 (0.00%)
I20240118 23:33:37.151778 1331215 AggregationFuzzer.cpp:1045] Total failed aggregations: 6 (9.84%)
[==========] Running 0 tests from 0 test suites.
[==========] 0 tests from 0 test suites ran. (0 ms total)
[ PASSED ] 0 tests. |
d44912f
to
4e7e431
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duanmeng Thank you for working on this. Some comments and questions.
size_t numDistinct{0}; | ||
|
||
// Number of iterations using distinct aggregation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using aggregations over distinct inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// Returns true if specified aggregate function can be applied to distinct | ||
// inputs. | ||
bool supportsDistinctInputs(const CallableSignature& signature) { | ||
if (signature.args.size() != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
signature.args.empty()
We should allow 2+ argument aggregate functions? Why limit to single-arg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
4e7e431
to
8edbab8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbasmanova Hi Masha, I've updated the PR according to your comments and guidance. Could you please help to take another look? Thanks.
size_t numDistinct{0}; | ||
|
||
// Number of iterations using distinct aggregation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// Returns true if specified aggregate function can be applied to distinct | ||
// inputs. | ||
bool supportsDistinctInputs(const CallableSignature& signature) { | ||
if (signature.args.size() != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
8edbab8
to
ff47dbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great % a couple tiny comments. Thank you for adding this functionality to help improve fuzzer coverage.
ff47dbc
to
6991d90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@duanmeng Ci is red. Would you take a look? |
Update: CI passed after rebase. @mbasmanova Hi Masha, there are two ci failures in logsvelox_s3file_test287: E0119 17:41:19.555821 96311 Exceptions.h:68] Line: ../../velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp:93, Function:initialize, Expression: Failed to get metadata for S3 object due to: 'Network connection'. Path:'s3://dummy/foo.txt', SDK Error Type:99, HTTP Status Code:-1, S3 Service:'Unknown', Message:'curlCode: 7, Couldn't connect to server', RequestID:'', Source: RUNTIME, ErrorCode: INVALID_STATE
287: ../../velox/connectors/hive/storage_adapters/s3fs/tests/S3FileSystemTest.cpp:134: Failure
287: Value of: e.message().find("Failed to get metadata for S3 object due to: 'Access denied'. Path:'s3://dummy/foo.txt', SDK Error Type:15, HTTP Status Code:403, S3 Service:'MinIO', Message:'No response body.'") != std::string::npos
287: Actual: false
287: Expected: true
287: Expected error message to contain 'Failed to get metadata for S3 object due to: 'Access denied'. Path:'s3://dummy/foo.txt', SDK Error Type:15, HTTP Status Code:403, S3 Service:'MinIO', Message:'No response body.'', but received 'Failed to get metadata for S3 object due to: 'Network connection'. Path:'s3://dummy/foo.txt', SDK Error Type:99, HTTP Status Code:-1, S3 Service:'Unknown', Message:'curlCode: 7, Couldn't connect to server', RequestID:'''.
287: I0119 17:41:19.557137 96311 TempDirectoryPath.cpp:31] TempDirectoryPath:: removing all files from/tmp/velox_test_S1xysT
287: [ FAILED ] S3FileSystemTest.invalidAccessKey (27566 ms) JoinFuzzerI20240119 18:41:39.149451 245782 JoinFuzzer.cpp:290] Executing query plan:
-- HashJoin[RIGHT SEMI (PROJECT) u0=t0 AND u3=t3 AND u2=t2 AND u1=t1] -> t2:INTERVAL DAY TO SECOND, match:BOOLEAN
-- TableScan[table: hive_table] -> u0:INTERVAL DAY TO SECOND, u1:INTERVAL DAY TO SECOND, u2:INTERVAL DAY TO SECOND, u3:VARBINARY, bp4:DATE
-- TableScan[table: hive_table] -> t0:INTERVAL DAY TO SECOND, t1:INTERVAL DAY TO SECOND, t2:INTERVAL DAY TO SECOND, t3:VARBINARY, tp4:VARCHAR, tp5:ROW<f0:BOOLEAN,f1:INTEGER>
I20240119 18:41:39.169649 249406 Task.cpp:1082] All drivers (4) finished for task test_cursor 694 after running for 18 ms.
I20240119 18:41:39.169705 249406 Task.cpp:1766] Terminating task test_cursor 694 with state Finished after running for 18 ms.
I20240119 18:41:39.170841 245782 JoinFuzzer.cpp:312] Results: [ROW ROW<t2:INTERVAL DAY TO SECOND,match:BOOLEAN>: 500 elements, no nulls]
../../velox/exec/tests/utils/QueryAssertions.cpp:1128: Failure
Failed
Expected 500, got 500
26 extra rows, 26 missing rows
10 of extra rows:
83070177176245426 | false
1266269431950912509 | false
1421626354945872417 | false
1753879513182740235 | false
2666551404982973180 | false
2768977648360222480 | false
3106077618523166233 | false
3157969022991353850 | false
3233218116822410557 | false
3346020967571024721 | false
10 of missing rows:
83070177176245426 | true
1266269431950912509 | true
1421626354945872417 | true
1753879513182740235 | true
2666551404982973180 | true
2768977648360222480 | true
3106077618523166233 | true
3157969022991353850 | true
3233218116822410557 | true
3346020967571024721 | true
Unexpected results
E20240119 18:41:39.179034 245782 Exceptions.h:68] Line: ../../velox/exec/tests/JoinFuzzer.cpp:849, Function:verify, Expression: assertEqualResults({expected}, {actual}) Logically equivalent plans produced different results, Source: RUNTIME, ErrorCode: INVALID_STATE
terminate called after throwing an instance of 'facebook::velox::VeloxRuntimeError'
what(): Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: Logically equivalent plans produced different results
Retriable: False
Expression: assertEqualResults({expected}, {actual})
Function: verify
File: ../../velox/exec/tests/JoinFuzzer.cpp
Line: 849
Stack trace:
# 0 _ZN8facebook5velox7process10StackTraceC1Ei
# 1 _ZN8facebook5velox14VeloxException5State4makeIZNS1_C4EPKcmS5_St17basic_string_viewIcSt11char_traitsIcEES9_S9_S9_bNS1_4TypeES9_EUlRT_E_EESt10shared_ptrIKS2_ESA_SB_
# 2 _ZN8facebook5velox14VeloxExceptionC1EPKcmS3_St17basic_string_viewIcSt11char_traitsIcEES7_S7_S7_bNS1_4TypeES7_
# 3 _ZN8facebook5velox17VeloxRuntimeErrorC2EPKcmS3_St17basic_string_viewIcSt11char_traitsIcEES7_S7_S7_bS7_
# 4 _ZN8facebook5velox6detail14veloxCheckFailINS0_17VeloxRuntimeErrorEPKcEEvRKNS1_18VeloxCheckFailArgsET0_
# 5 _ZN8facebook5velox4exec4test12_GLOBAL__N_110JoinFuzzer6verifyENS0_4core8JoinTypeE
# 6 _ZN8facebook5velox4exec4test12_GLOBAL__N_110JoinFuzzer2goEv
# 7 _ZN8facebook5velox4exec4test10joinFuzzerEm
# 8 _ZN16JoinFuzzerRunner3runEm
# 9 main
# 10 __libc_start_main
# 11 _start |
6991d90
to
f205c2e
Compare
@duanmeng Meng, thank you for looking into CI failures. Would you create 2 separate GitHub issues for these 2 problems (assuming there are no existing issues)? @majetideepak Deepak, would you help look into S3FileSystemTest.invalidAccessKey failure? |
@mbasmanova merged this pull request in e38603a. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Enhance AggregationFuzzer to add query plans that run aggregations over
unique inputs, i.e. SELECT k1, k2, count(distinct x) FROM t GROUP BY 1, 2.
The distinct column must be a comparable type for distinct aggregation.
Like Sorted aggregations, Distinct aggregations are tested using 2 logically
equivalent plans: Aggregation over Values and TableScan.
Distinct aggregations cannot be split into partial and final, hence, such
plans are not tested.
Exclude approx_xxx aggregations since their verifiers may not be able
to verify the results. The approx_percentile verifier would discard
the distinct property when calculating the expected result, say the
the expected result of the verifier would be
approx_percentile(x)
, whichmaybe different from the actual result of
approx_percentile(distinct x)
,when the x has lots of duplicated items.
Part of #7666