Skip to content
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

Decouple AggregationFuzzer from DuckDB #6701

Closed

Conversation

mbasmanova
Copy link
Contributor

@mbasmanova mbasmanova commented Sep 22, 2023

Extract ReferenceQueryRunner interface to allow using different reference
databases for results verification in AggregationFuzzer. For example, we would
want to verify Presto functions against Presto and Spark functions against
Spark.

Part of #6595

@mbasmanova mbasmanova marked this pull request as ready for review September 22, 2023 19:18
@netlify
Copy link

netlify bot commented Sep 22, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9bb7b8c
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65138b8518a1f30008fd223d

@facebook-github-bot 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 Sep 22, 2023
@facebook-github-bot
Copy link
Contributor

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

*/
#pragma once

#include "velox/exec/tests/utils/QueryAssertions.h"
Copy link
Collaborator

@majetideepak majetideepak Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this include to the DuckQueryRunner.cpp file.
You must forward-declare or use only the required std headers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that when you build the PrestoQueryRunner later, this include will enforce a dependency on DuckDB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@majetideepak These are good points. Thanks. Updated.

@mbasmanova mbasmanova force-pushed the reference-query-runner branch from e77fba3 to cf857c7 Compare September 23, 2023 00:30
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@mbasmanova mbasmanova force-pushed the reference-query-runner branch from f9d7e3e to 50ee13a Compare September 26, 2023 15:52
@facebook-github-bot
Copy link
Contributor

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

@mbasmanova mbasmanova force-pushed the reference-query-runner branch from 50ee13a to efe0994 Compare September 26, 2023 18:03
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbasmanova Looks great. Thanks!

return true;
}

std::unordered_set<std::string> getAggregateFunctions() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Summary:
Extract ReferenceQueryRunner interface to allow using different reference
databases for results verification in AggregationFuzzer. For example, we would
want to verify Presto functions against Presto and Spark functions against
Spark.

Part of facebookincubator#6595


Reviewed By: xiaoxmeng

Differential Revision: D49553797

Pulled By: mbasmanova
@mbasmanova mbasmanova force-pushed the reference-query-runner branch from efe0994 to 9bb7b8c Compare September 27, 2023 01:55
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49553797

@facebook-github-bot
Copy link
Contributor

@mbasmanova merged this pull request in cdfcd93.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit cdfcd938.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

ericyuliu pushed a commit to ericyuliu/velox that referenced this pull request Oct 12, 2023
Summary:
Extract ReferenceQueryRunner interface to allow using different reference
databases for results verification in AggregationFuzzer. For example, we would
want to verify Presto functions against Presto and Spark functions against
Spark.

Part of facebookincubator#6595

Pull Request resolved: facebookincubator#6701

Reviewed By: xiaoxmeng

Differential Revision: D49553797

Pulled By: mbasmanova

fbshipit-source-id: 489cd41ce4276c2d1d5230780f76439a39e70456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants