Add PrestoQueryRunner #7628

duanmeng · 2023-11-17T07:52:53Z

No description provided.

netlify · 2023-11-17T07:52:59Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`083114b`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/655cbb839ac1bd0008cccbbd

mbasmanova

@duanmeng Thank you for bringing in PrestoQueryRunner to Velox. Looks good % a question about type parsing.

CC: @majetideepak @pedroerp @kgpai

mbasmanova · 2023-11-17T13:08:34Z

velox/exec/tests/utils/PrestoQueryRunner.cpp

+    std::vector<TypePtr> types;
+    for (const auto& column : response_["columns"]) {
+      names.push_back(column["name"].asString());
+      // TODO：types.push_back(parseTypeSignature(column["type"].asString()));


Can we maybe parse the JSON and use logic similar to Type::create to create Type from it?

"typeSignature": { "rawType": "row", "typeArguments": [ { "rawType": "array", "typeArguments": [ { "rawType": "map", "typeArguments": [ { "rawType": "varchar", "typeArguments": [],

The type parser #7568 from @majetideepak is about to land, I will add parseType(column["type"].asString()) here to get the Velox type once it is merged.

mbasmanova · 2023-11-17T13:41:46Z

velox/exec/tests/utils/PrestoQueryRunner.cpp

+  VELOX_CHECK_EQ(
+      response.status_code,
+      200,
+      "Post to {} failed: {}",


Post -> POST

mbasmanova · 2023-11-17T13:41:53Z

velox/exec/tests/utils/PrestoQueryRunner.cpp

+  VELOX_CHECK_EQ(
+      response.status_code,
+      200,
+      "Get from {} failed: {}",


duanmeng · 2023-11-21T03:42:38Z

@mbasmanova Hi Masha, I've resolved the comments and used the typeParse API. I started a PrestoHiveRunner instance, enabled the PrestoRunnerTest, and ran, all passed locally. Could you please help to review PR? Thanks.

mbasmanova

@duanmeng Thank you for working on this. Looks good % a question below. Let's add an .md file to explain how to use PrestoQueryRunner, e.g. provide instructions for someone who would want to try running the test.

CC: @kgpai

mbasmanova · 2023-11-21T08:57:56Z

velox/exec/tests/utils/PrestoQueryRunner.h

+  PrestoQueryRunner(
+      std::string coordinatorUri,
+      std::string user,
+      const int32_t timeout = 10'000);


Drop const.

Document this parameter.

Perhaps, add units to the name for clarity: timeoutMillis or use an std::chrono type.

Why 10'000? Is this 10 seconds?

Why does the test need to override this to 20'000?

I've removed the const, documented this parameter, and used std::chrono type.

Why 10'000? Is this 10 seconds?

I followed the default value used in PrestoDB. I just changed it to 1000ms.

Why does the test need to override this to 20'000?

Removed.

@mbasmanova Could we add the document file in the fellow-up PR? I found that I need to make code changes to show how to use PrestoQueryRunner in Velox. I want to add a follow-up PR to make the runner configurable through the gflags along with the document on how to use PrestoQueryRunner. What do you think?

Could we add the document file in the fellow-up PR?

Sure.

I want to add a follow-up PR to make the runner configurable through the gflags

Wondering why would we want to do that.

Wondering why would we want to do that.

I found that I need to make code changes to show how to use PrestoQueryRunner in Velox, say velox_aggregation_fuzzer_test, when I am trying to write the document of how to use PrestoQueryRunner, something like the following. With the follow-up, we could control the creation of the query runner instance to switch runner only through program parameters.

diff --git a/velox/exec/tests/AggregationFuzzerTest.cpp b/velox/exec/tests/AggregationFuzzerTest.cpp --- a/velox/exec/tests/AggregationFuzzerTest.cpp +++ b/velox/exec/tests/AggregationFuzzerTest.cpp +#include "velox/exec/tests/utils/PrestoQueryRunner.h" - auto duckQueryRunner = + auto queryRunner = + std::make_unique<facebook::velox::exec::test::PrestoQueryRunner>("http://127.0.0.1:8080", "hive"); @@ -343,5 +337,5 @@ int main(int argc, char** argv) { facebook::velox::exec::test::getCustomInputGenerators(); options.timestampPrecision = facebook::velox::VectorFuzzer::Options::TimestampPrecision::kMilliSeconds; - return Runner::run(initialSeed, std::move(duckQueryRunner), options); + return Runner::run(initialSeed, std::move(queryRunner), options);

I don't think we want to allow for switching between Duck and Presto query runners in the Fuzzer. Once Presto query runner works, we want to always use that and remove Duck query runner.

Once Presto query runner works, we want to always use that and remove Duck query runner.

Got, it. I will highlight this in the follow-up.

duanmeng · 2023-11-21T09:19:33Z

@duanmeng Thank you for working on this. Looks good % a question below. Let's add an .md file to explain how to use PrestoQueryRunner, e.g. provide instructions for someone who would want to try running the test.

@mbasmanova Hi Masha, where should we add this .md file?

mbasmanova · 2023-11-21T09:21:50Z

where should we add this .md file?

Perhaps, next to PrestoQueryRunner.h, i.e. PrestoQueryRunner.md.

mbasmanova · 2023-11-21T09:24:23Z

velox/exec/tests/CMakeLists.txt

@@ -288,3 +288,11 @@ add_test(
  COMMAND cpr_http_client_test
  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
 target_link_libraries(cpr_http_client_test cpr::cpr gtest gtest_main)
+
+add_executable(presto_query_runner_test PrestoQueryRunnerTest.cpp)


Why do we need a separate executable for his test? We used to have a problem with CI ran out of disk space because all these executables are very large. CC: @kgpai

@mbasmanova Removed this executable, and linked the runner test into velox_exec_infra_test.

mbasmanova

@kgpai Krishna, would you help merge this PR?

kgpai

Thanks for this work @duanmeng

kgpai · 2023-11-21T13:09:43Z

velox/exec/tests/PrestoQueryRunnerTest.cpp

+  }
+};
+
+TEST_F(PrestoQueryRunnerTest, DISABLED_basic) {


nit: can you add a comment here that this is disabled because it requires a presto co-ordinator running at localhost etc ?

Added comments.

kgpai · 2023-11-21T13:44:23Z

velox/exec/tests/utils/PrestoQueryRunner.h

+  /// method.
+  /// @param resultType Expected type of the results.
+  /// @return Data received from Presto.
+  std::multiset<std::vector<velox::variant>> execute(


nit: maybe in a future pr we can add in the option to pass in table name, so we can potentially have multiple queries running at same time. This can only run single threaded atm.

OK, got it.

@kgpai How about generating a random table name when constructing the PrestoQueryRunner, say, tmp_$randonStringName_$threadId?

Let's keep things simple and first change the CI to allow running PrestoQueryRunnerTest, then replace Duck runner in the fuzzer.

facebook-github-bot · 2023-11-21T13:45:31Z

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

facebook-github-bot · 2023-11-21T18:59:57Z

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

facebook-github-bot · 2023-11-21T21:01:34Z

@kgpai merged this pull request in 19b29f7.

conbench-facebook · 2023-11-21T21:23:22Z

Conbench analyzed the 1 benchmark run on commit 19b29f7b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…tal jobs. (#8314) Summary: **What ?** Recently support for [PrestoQueryRunner ](#7628 added to AggregationFuzzer, so that it can use Presto as a source of truth. This PR sets up an experimental job that uses the Presto Java container to run Aggregation Fuzzer with Presto as source of truth. **Why ?** Running the aggregation fuzzer against Presto gives us greater confidence on the correctness of our aggregate functions. **How ?** 1. [Previously we created a docker image with support for java and Presto](#7920) 2. This still requires some changes to get Presto to work with AggregationFuzzer. These changes are listed below. 3. Adds support for Hive metastore - we do this via a file based metastore by adding the hive.properties file to $PRESTO_HOME/etc/catalog (see #8111) 4. We also set the pid file on the launcher to ensure there is only one run of the Presto server 5. Another thing to note is that we set the timezone to be ['Americas/Los_Angeles' due to details described here ](#8127). **Testing:** There is no automated way to test this. Here are manual steps I follow: 1. Create presto-java-container by running `docker-compose build presto-java` 6. Ssh into the presto-java-container 7. Copy Velox sources into the presto-java-container and build Velox 8. Launch Presto service: /opt/start-prestojava.sh 9. Run Aggregation fuzzer with --presto_url=http://127.0.0.1:8080 flag Example Run: Here is an example run https://github.com/facebookincubator/velox/actions/runs/7508139395/job/20443035814 Pull Request resolved: #8314 Reviewed By: kagamiori Differential Revision: D52701237 Pulled By: kgpai fbshipit-source-id: e4ba284aed469bc69cc95c90f708a0c92a040205

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 Nov 17, 2023

duanmeng requested a review from mbasmanova November 17, 2023 07:53

duanmeng force-pushed the runner branch from 1bf923f to ed6f0ff Compare November 17, 2023 07:55

duanmeng marked this pull request as draft November 17, 2023 07:57

duanmeng marked this pull request as ready for review November 17, 2023 08:47

mbasmanova reviewed Nov 17, 2023

View reviewed changes

mbasmanova requested review from kgpai and laithsakka November 17, 2023 13:09

mbasmanova reviewed Nov 17, 2023

View reviewed changes

duanmeng added 2 commits November 21, 2023 11:25

Add PrestoQueryRunner

ac75e96

use type parser

c2ea0d6

duanmeng force-pushed the runner branch from ed6f0ff to c2ea0d6 Compare November 21, 2023 03:39

mbasmanova reviewed Nov 21, 2023

View reviewed changes

resolve comments

62b9f5d

mbasmanova reviewed Nov 21, 2023

View reviewed changes

duanmeng added 2 commits November 21, 2023 18:44

remove executable

998adf4

fix cmake issue

90e5b66

mbasmanova approved these changes Nov 21, 2023

View reviewed changes

kgpai approved these changes Nov 21, 2023

View reviewed changes

resolve comments

083114b

facebook-github-bot closed this in 19b29f7 Nov 21, 2023

facebook-github-bot added the Merged label Nov 21, 2023

mbasmanova mentioned this pull request Nov 22, 2023

Allow custom result verifiers in AggregationFuzzer #7674

Closed

duanmeng mentioned this pull request Nov 29, 2023

Add PrestoQueryRunner Document #7712

Closed

kgpai mentioned this pull request Jan 12, 2024

Setup AggregationFuzzer to use Presto as source of truth in experimental jobs. #8314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PrestoQueryRunner #7628

Add PrestoQueryRunner #7628

duanmeng commented Nov 17, 2023

netlify bot commented Nov 17, 2023 •

edited

Loading

mbasmanova left a comment

mbasmanova Nov 17, 2023

duanmeng Nov 17, 2023

mbasmanova Nov 17, 2023

mbasmanova Nov 17, 2023

duanmeng commented Nov 21, 2023

mbasmanova left a comment

mbasmanova Nov 21, 2023

duanmeng Nov 21, 2023

mbasmanova Nov 21, 2023

duanmeng Nov 21, 2023

mbasmanova Nov 21, 2023

duanmeng Nov 21, 2023

mbasmanova Nov 21, 2023

duanmeng Nov 21, 2023

duanmeng commented Nov 21, 2023

mbasmanova commented Nov 21, 2023

mbasmanova Nov 21, 2023

duanmeng Nov 21, 2023

mbasmanova left a comment

kgpai left a comment

kgpai Nov 21, 2023

duanmeng Nov 21, 2023

kgpai Nov 21, 2023

duanmeng Nov 21, 2023

duanmeng Nov 21, 2023

mbasmanova Nov 21, 2023

facebook-github-bot commented Nov 21, 2023

facebook-github-bot commented Nov 21, 2023

facebook-github-bot commented Nov 21, 2023

conbench-facebook bot commented Nov 21, 2023

Add PrestoQueryRunner #7628

Add PrestoQueryRunner #7628

Conversation

duanmeng commented Nov 17, 2023

netlify bot commented Nov 17, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

mbasmanova 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

duanmeng commented Nov 21, 2023

mbasmanova 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

duanmeng commented Nov 21, 2023

mbasmanova commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

kgpai 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

facebook-github-bot commented Nov 21, 2023

facebook-github-bot commented Nov 21, 2023

facebook-github-bot commented Nov 21, 2023

conbench-facebook bot commented Nov 21, 2023

netlify bot commented Nov 17, 2023 •

edited

Loading