Add support in AggregationFuzzer to compare against reference DB for non deterministic functions on unsorted inputs #9009

kgpai · 2024-03-08T19:10:51Z

Why
Currently for aggregate functions with custom verifiers, we do not verify plans against the reference db. This is because for these functions the reference db results differ and require us to use custom verifiers - typically the ResultVerifier::compare api. The compare api requires VeloxVectors and previously the results of the reference query runner were in materialized row format and thus couldnt be called. Recently support was added to return the reference db results as velox vectors (#8880). With this we can now validate functions which require custom verifiers.

What
We add support in the AggregationFuzzer to use the new 'executeVector' api if supported by the refrence query runner, and use that to validate non deterministic functions that require a custom verifier.

Results
Currently with this change we have increased the number of plans being verified against reference db from 40 % to 80+%.

 Total masked aggregations: 78 (14.58%)
Total global aggregations: 41 (7.66%)
Total group-by aggregations: 392 (73.27%)
Total distinct aggregations: 49 (9.16%)
 Total aggregations over distinct inputs: 40 (7.48%)
Total window expressions: 53 (9.91%)
 Total functions tested: 38
 Total iterations requiring sorted inputs: 43 (8.04%)
Total iterations verified against reference DB: 448 (83.74%)
 Total functions not verified (verification skipped / not supported by reference DB / reference DB failed): 53 (9.91%) / 6 (1.12%) / 3 (0.56%)
 Total failed functions: 27 (5.05%)

Experimental runs :
https://github.com/facebookincubator/velox/actions/runs/8199583642/job/22443872635
https://github.com/facebookincubator/velox/actions/runs/8199583642/job/22424917125

Notes
Certain functions have been skiplisted for following reasons:

map_union_sum : This requires us to use the lates presto due to a bug on Presto end in version .284 not supporting reals.
approx_set: Signatures supported by Velox are more than supported by Presto
min_by, max_by: Requires custom verifiers (order by not supported by Presto)
any_value: alias for arbitrary but this alias isnt present in Presto.

netlify · 2024-03-08T19:11:09Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`9dee0c6`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65f36efaec71c400089409d9

duanmeng

@kgpai Looks great % some nits.

duanmeng · 2024-03-11T02:30:19Z

velox/exec/fuzzer/AggregationFuzzerBase.cpp

@@ -77,6 +77,20 @@ DEFINE_bool(

 namespace facebook::velox::exec::test {

+bool AggregationFuzzerBase::isSupportedType(const TypePtr& type) const {


Any chance to add a comment for it?

duanmeng · 2024-03-11T02:31:33Z

velox/exec/fuzzer/AggregationFuzzerBase.cpp

+          referenceQueryRunner_->executeVector(
+              sql.value(), input, plan->outputType()),
+          ReferenceQueryErrorCode::kSuccess);
+    } catch (std::exception& e) {


const std::exception& ?

Doesnt look like we use it, I can just remove it .

duanmeng · 2024-03-11T02:34:29Z

velox/exec/fuzzer/AggregationFuzzerBase.cpp

+              sql.value(), input, plan->outputType()),
+          ReferenceQueryErrorCode::kSuccess);
+    } catch (std::exception& e) {
+      // ++stats_.numReferenceQueryFailed;


Should we remove this?

kagamiori

Hi @kgpai, thank you for adding this support! I left a few comments.

kagamiori · 2024-03-11T20:34:23Z

velox/exec/fuzzer/AggregationFuzzer.cpp

-            VELOX_CHECK(
-                verifier->verify(resultOrError.result),
-                "Aggregation results failed custom verification");
+        if (expectedResult && resultOrError.result) {


nit: resultOrError.result != nullptr is already checked at line 1031, so no need to check it again here.

kagamiori · 2024-03-11T20:34:55Z

velox/exec/fuzzer/AggregationFuzzer.cpp

    if (resultOrError.result != nullptr) {
      if (!customVerification) {
+        std::optional<MaterializedRowMultiset> expectedResult;


nit: simply auto expectedResult = referenceResult.first below would be easier?

kagamiori · 2024-03-11T20:50:40Z

velox/exec/fuzzer/AggregationFuzzer.cpp

+              "Velox and reference DB results don't match");
+          LOG(INFO) << "Verified results against reference DB";
+        }
+      } else if (referenceQueryRunner_->supportsVeloxVectorResults()) {


Should we check the verifier supports compare here too? And if not, we still call verify()?

I considered only cases where compare is supported, but its still helpful to run against reference even where the verifier only supports verify, for e.g to ensure that the signatures are supported by Presto (if velox supports more signatures). I dont think we lose anything by having it run against Presto.

kagamiori · 2024-03-11T20:53:03Z

velox/exec/fuzzer/AggregationFuzzer.cpp

+            // Merge all the results into one.
+            auto results = referenceResult.first.value();
+            auto totalCount = 0;
+            for (const auto& result : results) {
+              totalCount += result->size();
+            }
+            auto copy = BaseVector::create<RowVector>(
+                results[0]->type(), totalCount, pool_.get());
+            auto copyCount = 0;
+            for (const auto& result : results) {
+              copy->copy(result.get(), copyCount, 0, result->size());
+              copyCount += result->size();
+            }
+
+            expected.result = copy;


Could we extract this into a helper method?

kagamiori · 2024-03-11T20:54:26Z

velox/exec/fuzzer/AggregationFuzzerBase.cpp

@@ -77,6 +77,20 @@ DEFINE_bool(

 namespace facebook::velox::exec::test {

+bool AggregationFuzzerBase::isSupportedType(const TypePtr& type) const {
+  if (type->isDate() || type->isIntervalDayTime() || type->isUnKnown()) {


It would be nice to explain why these types are not supported.

I am wondering whether I should make this a property of referenceQueryRunner.

Are they supported in DuckQueryRunner? If so, we cannot make them a property of referenceQueryRunner.

I think ddb doesnt support Timestamp (because of nanosecond precision), and these types arent supported in PQR; so I still feel at some level this should be properties of respective query runner. Let me know what you think.

kagamiori · 2024-03-11T21:02:02Z

velox/exec/fuzzer/AggregationFuzzerBase.cpp

+    LOG(INFO) << "Query not supported by the reference DB";
+  }
+
+  return std::make_pair(


nit: Move this into the else branch to be with the log statement together?

kagamiori

LGTM. Thanks!

kagamiori · 2024-03-13T00:28:15Z

velox/exec/fuzzer/AggregationFuzzerBase.cpp

@@ -77,6 +77,20 @@ DEFINE_bool(

 namespace facebook::velox::exec::test {

+bool AggregationFuzzerBase::isSupportedType(const TypePtr& type) const {
+  if (type->isDate() || type->isIntervalDayTime() || type->isUnKnown()) {


Are they supported in DuckQueryRunner? If so, we cannot make them a property of referenceQueryRunner.

kagamiori · 2024-03-13T00:32:25Z

velox/exec/fuzzer/AggregationFuzzerBase.h

+
+  /// Returns false if the type or its children are unsupported.
+  /// Currently returns false if type is Date,IntervalDayTime or Unknown.
+  /// \param type


nit: Why do we use /// instead of // here? Should this be "@param" and "@return"?

This was auto generated , I will change it to @param and @return , the convention is for /// if we want to make a docstring .

facebook-github-bot · 2024-03-14T18:26:05Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-03-14T21:42:28Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-03-15T00:07:47Z

@kgpai merged this pull request in 03d017f.

…non deterministic functions on unsorted inputs (facebookincubator#9009) Summary: **Why** Currently for aggregate functions with custom verifiers, we do not verify plans against the reference db. This is because for these functions the reference db results differ and require us to use custom verifiers - typically the ResultVerifier::compare api. The compare api requires VeloxVectors and previously the results of the reference query runner were in materialized row format and thus couldnt be called. Recently support was added to return the reference db results as velox vectors (facebookincubator#8880). With this we can now validate functions which require custom verifiers. **What** We add support in the AggregationFuzzer to use the new 'executeVector' api if supported by the refrence query runner, and use that to validate non deterministic functions that require a custom verifier. **Results** Currently with this change we have increased the number of plans being verified against reference db from 40 % to 80+%. ``` Total masked aggregations: 78 (14.58%) Total global aggregations: 41 (7.66%) Total group-by aggregations: 392 (73.27%) Total distinct aggregations: 49 (9.16%) Total aggregations over distinct inputs: 40 (7.48%) Total window expressions: 53 (9.91%) Total functions tested: 38 Total iterations requiring sorted inputs: 43 (8.04%) Total iterations verified against reference DB: 448 (83.74%) Total functions not verified (verification skipped / not supported by reference DB / reference DB failed): 53 (9.91%) / 6 (1.12%) / 3 (0.56%) Total failed functions: 27 (5.05%) ``` Experimental runs : https://github.com/facebookincubator/velox/actions/runs/8199583642/job/22443872635 https://github.com/facebookincubator/velox/actions/runs/8199583642/job/22424917125 **Notes** Certain functions have been skiplisted for following reasons: 1. map_union_sum : This requires us to use the lates presto due to a bug on Presto end in version .284 not supporting reals. 2. approx_set: Signatures supported by Velox are more than supported by Presto 3. min_by, max_by: Requires custom verifiers (order by not supported by Presto) 4. any_value: alias for arbitrary but this alias isnt present in Presto. Pull Request resolved: facebookincubator#9009 Reviewed By: kagamiori Differential Revision: D54911135 Pulled By: kgpai fbshipit-source-id: e2fc1fc91a367eb1a3538aa87e755e31ae1018a6

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 8, 2024

kgpai requested review from kagamiori, duanmeng and mbasmanova March 8, 2024 19:11

duanmeng reviewed Mar 11, 2024

View reviewed changes

kagamiori reviewed Mar 11, 2024

View reviewed changes

kgpai force-pushed the agg_fuzzer_unsorted branch 2 times, most recently from 74e06d1 to d702f45 Compare March 12, 2024 21:30

kagamiori approved these changes Mar 13, 2024

View reviewed changes

Rebase to main

a0af100

kgpai force-pushed the agg_fuzzer_unsorted branch from d702f45 to f39362e Compare March 14, 2024 18:08

Removing arbitrary from skipList.

9dee0c6

kgpai force-pushed the agg_fuzzer_unsorted branch from f39362e to 9dee0c6 Compare March 14, 2024 21:41

facebook-github-bot closed this in 03d017f Mar 15, 2024

facebook-github-bot added the Merged label Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support in AggregationFuzzer to compare against reference DB for non deterministic functions on unsorted inputs #9009

Add support in AggregationFuzzer to compare against reference DB for non deterministic functions on unsorted inputs #9009

kgpai commented Mar 8, 2024

netlify bot commented Mar 8, 2024 •

edited

Loading

duanmeng left a comment

duanmeng Mar 11, 2024

duanmeng Mar 11, 2024

kgpai Mar 11, 2024

duanmeng Mar 11, 2024

kagamiori left a comment

kagamiori Mar 11, 2024

kagamiori Mar 11, 2024

kagamiori Mar 11, 2024

kgpai Mar 12, 2024

kagamiori Mar 11, 2024

kagamiori Mar 11, 2024

kgpai Mar 12, 2024

kagamiori Mar 13, 2024

kgpai Mar 13, 2024

kagamiori Mar 11, 2024

kagamiori left a comment

kagamiori Mar 13, 2024

kagamiori Mar 13, 2024

kgpai Mar 13, 2024

facebook-github-bot commented Mar 14, 2024

facebook-github-bot commented Mar 14, 2024

facebook-github-bot commented Mar 15, 2024

		@@ -77,6 +77,20 @@ DEFINE_bool(

		namespace facebook::velox::exec::test {

		bool AggregationFuzzerBase::isSupportedType(const TypePtr& type) const {

Add support in AggregationFuzzer to compare against reference DB for non deterministic functions on unsorted inputs #9009

Add support in AggregationFuzzer to compare against reference DB for non deterministic functions on unsorted inputs #9009

Conversation

kgpai commented Mar 8, 2024

netlify bot commented Mar 8, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

duanmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kagamiori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kagamiori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 14, 2024

facebook-github-bot commented Mar 14, 2024

facebook-github-bot commented Mar 15, 2024

netlify bot commented Mar 8, 2024 •

edited

Loading