feat(fuzzer): Support multiple joins in the join node "toSql" methods for reference query runners #11801

DanielHunte · 2024-12-09T20:10:47Z

Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query.

Differential Revision: D66977480

netlify · 2024-12-09T20:11:03Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`2d9c75b`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67733cdebc5f9e0008dbf81d

facebook-github-bot · 2024-12-09T20:11:14Z

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

facebook-github-bot · 2024-12-09T20:11:46Z

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

…in Presto QR (facebookincubator#11801) Summary: Currently, the hash join "toSql" method for PrestoQueryRunner only supports a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-09T22:10:56Z

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

…for reference query runners (facebookincubator#11801) Summary: Currently, the hash join "toSql" method for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-10T18:20:38Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-10T20:26:00Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-14T01:17:50Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-15T07:09:00Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-16T04:41:45Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-16T06:01:21Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

pedroerp

Thanks for looking into this @DanielHunte . Left a few comments

velox/core/PlanNode.h

velox/exec/fuzzer/DuckQueryRunner.cpp

pedroerp · 2024-12-27T18:12:02Z

velox/exec/fuzzer/DuckQueryRunner.cpp

+    probeTableName = fmt::format("({})", *probeSubQuery);
+    buildTableName = fmt::format("({})", *buildSubQuery);
+  } else {
+    return std::nullopt;


should this throw an exception, or this is a legit valid case?

velox/exec/fuzzer/PrestoQueryRunner.cpp

pedroerp · 2024-12-27T18:16:47Z

velox/exec/fuzzer/ReferenceQueryRunner.h

@@ -66,6 +66,13 @@ class ReferenceQueryRunner {
    return true;
  }

+  /// Executes SQL query returned by the 'toSql' method based on the plan.
+  virtual std::multiset<std::vector<velox::variant>> execute(


why not making this pure virtual? ( = 0;)

SparkQueryRunner does not currently need an implementation of this method.

pedroerp · 2024-12-27T18:16:54Z

velox/exec/fuzzer/ReferenceQueryRunner.h

@@ -88,6 +95,13 @@ class ReferenceQueryRunner {
    return false;
  }

+  /// Similar to 'execute' but returns results in RowVector format.
+  virtual std::vector<RowVectorPtr> executeVector(


SparkQueryRunner does not currently need an implementation of this method.

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-30T20:13:54Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-30T20:20:25Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-30T20:48:53Z

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

… for reference query runners (facebookincubator#11801) Summary: Currently, the hash join and nested loop join "toSql" methods for all reference query runners only support a single join. This change extends it to support multiple joins, only needing the join node of the last join in the tree. It traverses up the tree and recursively builds the sql query. Differential Revision: D66977480

facebook-github-bot · 2024-12-31T00:38:12Z

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

kevinwilfong · 2025-01-03T23:59:56Z

velox/exec/fuzzer/ReferenceQueryRunner.h

+  virtual std::vector<RowVectorPtr> executeAndReturnVector(
+      const std::string& sql,
+      const core::PlanNodePtr& plan) {
+    VELOX_UNSUPPORTED();
+  }


Why does executeAndReturnVector need to be in ReferenceQueryRunner/virtual? It looks like it's only called from PrestoQueryRunner and could just be a private helper method.

kevinwilfong · 2025-01-04T00:02:14Z

velox/exec/fuzzer/DuckQueryRunner.cpp

+  if (const auto joinNode =
+          std::dynamic_pointer_cast<const core::ValuesNode>(plan)) {
+    return toSql(joinNode);
+  }


Looks like you copied this from above, valuesNode would be a more appropriate variable name here.

kevinwilfong · 2025-01-04T00:11:21Z

velox/exec/fuzzer/DuckQueryRunner.h

@@ -60,6 +65,8 @@ class DuckQueryRunner : public ReferenceQueryRunner {
      const RowTypePtr& resultType) override;


Do we still use this function anywhere, or can we delete this flavor of execute?

kevinwilfong · 2025-01-04T00:17:29Z

velox/exec/fuzzer/DuckQueryRunner.h

+      const std::string& sql,
+      const core::PlanNodePtr& plan) override;


I saw the discussion that caused you to add sql as parameter this function, but it feels a little unnecessarily dangerous to me to pass in plan and sql (which is wholly derived from plan) and rely on them to be in sync without being able to check.

What do you think about passing in the plan and only converting the plan to SQL in execute?

The code in JoinFuzzer is

if (auto sql = referenceQueryRunner_->toSql(plan)) { return referenceQueryRunner_->execute(*sql, plan); } LOG(INFO) << "Query not supported by the reference DB"; return std::nullopt;

if we do the conversion in execute and return std::optional<std::multiset<std::vectorvelox::variant>> then all that code in JoinFuzzer could just be

return referenceQueryRunner_->execute(plan);

which still addresses the duplication that Wei had concerns about.

(as a small bonus the log message could be a little more explicit "Query not supported by DuckDB" and "Query not supported by Presto" in their respective implementations)

kevinwilfong · 2025-01-04T00:25:08Z

velox/exec/fuzzer/ReferenceQueryRunner.h

+  /// Returns the name of the values node table in the form t_<id>.
+  std::string getTableName(const core::ValuesNodePtr& valuesNode) {
+    return fmt::format("t_{}", valuesNode->id());
+  }
+  // Traverses all nodes in the plan and returns all tables and their names.
+  std::unordered_map<std::string, std::vector<velox::RowVectorPtr>>
+  getAllTables(const core::PlanNodePtr& plan) {
+    std::unordered_map<std::string, std::vector<velox::RowVectorPtr>> result;
+    if (const auto valuesNode =
+            std::dynamic_pointer_cast<const core::ValuesNode>(plan)) {
+      result.insert({getTableName(valuesNode), valuesNode->values()});
+    } else {
+      for (const auto& source : plan->sources()) {
+        auto tablesAndNames = getAllTables(source);
+        result.insert(tablesAndNames.begin(), tablesAndNames.end());
+      }
+    }
+    return result;
+  }
+
+  bool isSupportedDwrfType(const TypePtr& type) {
+    if (type->isDate() || type->isIntervalDayTime() || type->isUnKnown()) {
+      return false;
+    }
+
+    for (auto i = 0; i < type->size(); ++i) {
+      if (!isSupportedDwrfType(type->childAt(i))) {
+        return false;
+      }
+    }
+
+    return true;
+  }


These could be protected right? (It looks like getTableName and isSupportedDwrfType could even be private)

kevinwilfong · 2025-01-04T00:26:55Z

velox/exec/fuzzer/ReferenceQueryRunner.h

+      out << filterToSql(joinNode.filter());
+    }
+    return out.str();
+  };


kevinwilfong · 2025-01-04T00:29:49Z

velox/exec/fuzzer/ReferenceQueryRunner.h

+  }
+
+  /// Same as the above toSql but for hash join nodes.
+  virtual std::optional<std::string> toSql(


Before this change ReferenceQueryRunner was pretty trivial. With these more complicated function definitions it's probably worth adding a ReferenceQueryRunner.cpp and moving the implementation of these toSql methods there (just like they were split in PrestoQueryRunner).

If you do that, the static methods declared above could probably be moved outside the class into functions declared in an anonymous namespace in the cpp file.

kevinwilfong · 2025-01-04T00:44:16Z

velox/exec/fuzzer/ReferenceQueryRunner.h

+      probeTableName = probeSubQuery->find(" ") != std::string::npos
+          ? fmt::format("({})", *probeSubQuery)
+          : *probeSubQuery;
+      buildTableName = buildSubQuery->find(" ") != std::string::npos
+          ? fmt::format("({})", *buildSubQuery)
+          : *buildSubQuery;


minor: it should still work if you just always wrap the sub queries in parentheses right? that seems simpler

kevinwilfong · 2025-01-04T00:49:15Z

velox/exec/fuzzer/ReferenceQueryRunner.h

+    std::string probeTableName;
+    std::string buildTableName;
+    const std::optional<std::string> probeSubQuery =
+        toSql(joinNode->sources()[0]);
+    const std::optional<std::string> buildSubQuery =
+        toSql(joinNode->sources()[1]);
+    if (probeSubQuery && buildSubQuery) {
+      probeTableName = probeSubQuery->find(" ") != std::string::npos
+          ? fmt::format("({})", *probeSubQuery)
+          : *probeSubQuery;
+      buildTableName = buildSubQuery->find(" ") != std::string::npos
+          ? fmt::format("({})", *buildSubQuery)
+          : *buildSubQuery;
+    } else {
+      return std::nullopt;
+    }


It looks like this code is duplicated, could you make a helper function, e.g. something like

std::optional<std::string> joinSourceToSql(const PlanNodePtr& node) { const std::optional<std::string> subQuery = toSql(node); if (!subQuery) { return std::nullopt; } return fmt::format("({})", *subQuery); }

Then in both join toSql functions you'd have

auto probeTableName = joinSourceToSql(joinNode->sources()[0]); auto buildTableName = joinSourceToSql(joinNode->sources()[1]); if (!probeTableName || !buildTableName) { return std::nullopt; }

kevinwilfong · 2025-01-04T00:51:06Z

velox/exec/fuzzer/ReferenceQueryRunner.h

+    VELOX_CHECK(
+        joinNode->joinCondition() == nullptr,


nit: you could make this VELOX_CHECK_NULL(joinNode->joinCondition(), ....

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 Dec 9, 2024

facebook-github-bot added the fb-exported label Dec 9, 2024

DanielHunte force-pushed the export-D66977480 branch from e573440 to f24cd12 Compare December 9, 2024 20:11

DanielHunte force-pushed the export-D66977480 branch from f24cd12 to 6e9702c Compare December 9, 2024 22:10

DanielHunte force-pushed the export-D66977480 branch from 6e9702c to 3cbd9e0 Compare December 10, 2024 18:20

DanielHunte changed the title ~~feat(fuzzer): Support multiple joins in the hash node "toSql" method in Presto QR~~ feat(fuzzer): Support multiple joins in the hash node "toSql" method for reference query runners Dec 10, 2024

DanielHunte changed the title ~~feat(fuzzer): Support multiple joins in the hash node "toSql" method for reference query runners~~ feat(fuzzer): Support multiple joins in the join node "toSql" methods for reference query runners Dec 10, 2024

DanielHunte force-pushed the export-D66977480 branch from 3cbd9e0 to 28f2244 Compare December 10, 2024 20:25

DanielHunte force-pushed the export-D66977480 branch from 28f2244 to 180fe64 Compare December 14, 2024 01:17

DanielHunte force-pushed the export-D66977480 branch from 180fe64 to 731d39f Compare December 15, 2024 07:08

DanielHunte force-pushed the export-D66977480 branch from 731d39f to 211a0b3 Compare December 16, 2024 04:41

DanielHunte force-pushed the export-D66977480 branch from 211a0b3 to 3befde2 Compare December 16, 2024 06:01

DanielHunte force-pushed the export-D66977480 branch from 3befde2 to 1db434c Compare December 16, 2024 20:20

pedroerp reviewed Dec 27, 2024

View reviewed changes

DanielHunte force-pushed the export-D66977480 branch from 3cbb0cf to f19fcc8 Compare December 30, 2024 20:13

DanielHunte force-pushed the export-D66977480 branch from f19fcc8 to a872daa Compare December 30, 2024 20:19

DanielHunte force-pushed the export-D66977480 branch from a872daa to 4d95f9d Compare December 30, 2024 20:48

DanielHunte force-pushed the export-D66977480 branch from 4d95f9d to 2d9c75b Compare December 31, 2024 00:37

kevinwilfong reviewed Jan 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fuzzer): Support multiple joins in the join node "toSql" methods for reference query runners #11801

feat(fuzzer): Support multiple joins in the join node "toSql" methods for reference query runners #11801

DanielHunte commented Dec 9, 2024 •

edited

Loading

netlify bot commented Dec 9, 2024 •

edited

Loading

facebook-github-bot commented Dec 9, 2024

facebook-github-bot commented Dec 9, 2024

facebook-github-bot commented Dec 9, 2024

facebook-github-bot commented Dec 10, 2024

facebook-github-bot commented Dec 10, 2024

facebook-github-bot commented Dec 14, 2024

facebook-github-bot commented Dec 15, 2024

facebook-github-bot commented Dec 16, 2024

facebook-github-bot commented Dec 16, 2024

pedroerp left a comment

pedroerp Dec 27, 2024

pedroerp Dec 27, 2024

DanielHunte Dec 30, 2024

pedroerp Dec 27, 2024

DanielHunte Dec 30, 2024

facebook-github-bot commented Dec 30, 2024

facebook-github-bot commented Dec 30, 2024

facebook-github-bot commented Dec 30, 2024

facebook-github-bot commented Dec 31, 2024

kevinwilfong Jan 3, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

kevinwilfong Jan 4, 2025

		@@ -60,6 +65,8 @@ class DuckQueryRunner : public ReferenceQueryRunner {
		const RowTypePtr& resultType) override;

		const std::string& sql,
		const core::PlanNodePtr& plan) override;

feat(fuzzer): Support multiple joins in the join node "toSql" methods for reference query runners #11801

Are you sure you want to change the base?

feat(fuzzer): Support multiple joins in the join node "toSql" methods for reference query runners #11801

Conversation

DanielHunte commented Dec 9, 2024 • edited Loading

netlify bot commented Dec 9, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Dec 9, 2024

facebook-github-bot commented Dec 9, 2024

facebook-github-bot commented Dec 9, 2024

facebook-github-bot commented Dec 10, 2024

facebook-github-bot commented Dec 10, 2024

facebook-github-bot commented Dec 14, 2024

facebook-github-bot commented Dec 15, 2024

facebook-github-bot commented Dec 16, 2024

facebook-github-bot commented Dec 16, 2024

pedroerp 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

facebook-github-bot commented Dec 30, 2024

facebook-github-bot commented Dec 30, 2024

facebook-github-bot commented Dec 30, 2024

facebook-github-bot commented Dec 31, 2024

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

DanielHunte commented Dec 9, 2024 •

edited

Loading

netlify bot commented Dec 9, 2024 •

edited

Loading