feat: Add index join conditions #12461

xiaoxmeng · 2025-02-27T00:13:08Z

Summary:
Change index join condition from SQL expression to a more strict condition structure
to present in query plan node

Differential Revision: D70233337

facebook-github-bot · 2025-02-27T00:13:17Z

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

netlify · 2025-02-27T00:13:27Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`c856260`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67c15927bf920900086049b4

zhztheplayer · 2025-02-27T05:09:45Z

velox/core/PlanNode.h

@@ -1768,6 +1768,74 @@ class MergeJoinNode : public AbstractJoinNode {
  static PlanNodePtr create(const folly::dynamic& obj, void* context);
 };

+struct IndexJoinCondition;
+using IndexJoinConditionPtr = std::shared_ptr<IndexJoinCondition>;
+struct IndexJoinCondition : public ISerializable {


Just my 2 cents: Does IndexJoinCondition have to extend ISerializable given the subclasses are unlikely using name JSON field as type annotations?

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

facebook-github-bot · 2025-02-27T18:14:36Z

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

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

facebook-github-bot · 2025-02-27T18:18:55Z

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

mbasmanova

@xiaoxmeng Meng, would you update PR description to explain what kinds of conditions are supported for index join?

mbasmanova · 2025-02-27T18:44:54Z

velox/connectors/Connector.h

@@ -584,8 +588,8 @@ class Connector {
  /// Here,
  /// - 'inputType' is ROW{t.sid, t.event_list}
  /// - 'numJoinKeys' is 1 since only t.sid is used in join equi-clauses.
-  /// - 'joinConditions' is list of one expression: contains(t.event_list,
-  ///    u.event_type)
+  /// - 'joinConditions' is list of one join condition: contains(t.event_list,


typo: "is list of one join condition" - why would we need a list of one?

This is to describe a particular example. The join conditions is a list of conditions and in above example, there is only one condition. I rephrase a bit to make it clear.

velox/core/PlanNode.h

mbasmanova · 2025-02-27T18:47:19Z

velox/core/PlanNode.h

+
+  IndexJoinCondition(FieldAccessTypedExprPtr _key) : key(std::move(_key)) {}
+
+  folly::dynamic serialize() const override;


Should we leave this not implemented?

velox/core/PlanNode.h

mbasmanova · 2025-02-27T18:47:58Z

velox/core/PlanNode.h

+  TypedExprPtr upper;
+
+  BetweenIndexJoinCondition(
+      FieldAccessTypedExprPtr key,


velox/core/PlanNode.h

mbasmanova · 2025-02-27T18:49:55Z

velox/exec/tests/utils/PlanBuilder.h

  /// @param joinType Type of the join supported: inner, left.
  ///
  /// See hashJoin method for the description of the other parameters.
  PlanBuilder& indexLookupJoin(
      const std::vector<std::string>& leftKeys,
      const std::vector<std::string>& rightKeys,
      const core::TableScanNodePtr& right,
-      const std::vector<std::string>& joinCondition,
+      const std::vector<IndexJoinCondition>& joinConditions,


Can we keep this as a SQL string as it is easier to use? Let's document what SQL strings are supported.

Good point.

mbasmanova · 2025-02-27T18:51:15Z

velox/exec/IndexLookupJoin.cpp

+  return isConstant;
+}
+
+void addBetweenCondition(


Would you ad comments to explain what these functions do?

Comment added.

velox/exec/IndexLookupJoin.cpp

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

facebook-github-bot · 2025-02-27T20:09:16Z

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

Summary: Pull Request resolved: facebookincubator#12461 Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

mbasmanova · 2025-02-27T20:51:25Z

velox/exec/tests/utils/PlanBuilder.cpp

+        std::move(keyColumnExpr), std::move(lowerExpr), std::move(upperExpr));
+  }
+
+  VELOX_USER_FAIL("Invalid index join condition: {}", joinCondition);


Would you update the API comments to clarify what are supported conditions and enhance this error message to include that as well?

facebook-github-bot · 2025-02-27T20:51:55Z

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

Summary: Pull Request resolved: facebookincubator#12461 Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

facebook-github-bot · 2025-02-27T22:05:44Z

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

facebook-github-bot · 2025-02-27T22:10:28Z

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

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Differential Revision: D70233337

facebook-github-bot · 2025-02-27T22:13:47Z

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

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

facebook-github-bot · 2025-02-28T00:41:20Z

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

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

facebook-github-bot · 2025-02-28T01:07:10Z

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

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

facebook-github-bot · 2025-02-28T04:15:19Z

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

Summary: Pull Request resolved: facebookincubator#12461 Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

facebook-github-bot · 2025-02-28T06:15:16Z

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

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

facebook-github-bot · 2025-02-28T06:15:44Z

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

Summary: Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

facebook-github-bot · 2025-02-28T06:31:22Z

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

Summary: Pull Request resolved: facebookincubator#12461 Change index join condition from SQL expression to a more strict condition structure to present in query plan node Reviewed By: wenqiwooo, zacw7 Differential Revision: D70233337

facebook-github-bot · 2025-02-28T06:35:14Z

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

xiaoxmeng requested a review from majetideepak as a code owner February 27, 2025 00:13

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 Feb 27, 2025

facebook-github-bot added the fb-exported label Feb 27, 2025

zhztheplayer reviewed Feb 27, 2025

View reviewed changes

xiaoxmeng force-pushed the export-D70233337 branch from 6383bac to dafb100 Compare February 27, 2025 18:14

xiaoxmeng force-pushed the export-D70233337 branch from dafb100 to 257e44a Compare February 27, 2025 18:18

mbasmanova reviewed Feb 27, 2025

View reviewed changes

velox/exec/IndexLookupJoin.cpp Outdated Show resolved Hide resolved

xiaoxmeng force-pushed the export-D70233337 branch from 257e44a to 87afeb2 Compare February 27, 2025 20:06

xiaoxmeng force-pushed the export-D70233337 branch from 87afeb2 to f618f14 Compare February 27, 2025 20:09

xiaoxmeng force-pushed the export-D70233337 branch from f618f14 to d143565 Compare February 27, 2025 20:46

mbasmanova reviewed Feb 27, 2025

View reviewed changes

xiaoxmeng force-pushed the export-D70233337 branch from d143565 to 72d863d Compare February 27, 2025 20:51

xiaoxmeng force-pushed the export-D70233337 branch from 72d863d to 7507a1d Compare February 27, 2025 22:05

xiaoxmeng force-pushed the export-D70233337 branch from f006e0a to 2d00197 Compare February 27, 2025 22:13

zacw7 approved these changes Feb 27, 2025

View reviewed changes

xiaoxmeng force-pushed the export-D70233337 branch from 2d00197 to e028762 Compare February 28, 2025 00:40

xiaoxmeng force-pushed the export-D70233337 branch from e028762 to 3f8a705 Compare February 28, 2025 01:06

xiaoxmeng force-pushed the export-D70233337 branch from 3f8a705 to 32bab37 Compare February 28, 2025 04:11

xiaoxmeng force-pushed the export-D70233337 branch from 32bab37 to 566d785 Compare February 28, 2025 04:15

xiaoxmeng force-pushed the export-D70233337 branch from 566d785 to 3d6ed3b Compare February 28, 2025 06:14

xiaoxmeng force-pushed the export-D70233337 branch from 3d6ed3b to 323ec58 Compare February 28, 2025 06:15

xiaoxmeng force-pushed the export-D70233337 branch from 323ec58 to 3f53873 Compare February 28, 2025 06:31

xiaoxmeng force-pushed the export-D70233337 branch from 3f53873 to 495a376 Compare February 28, 2025 06:31

xiaoxmeng force-pushed the export-D70233337 branch from 495a376 to c856260 Compare February 28, 2025 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add index join conditions #12461

feat: Add index join conditions #12461

xiaoxmeng commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

netlify bot commented Feb 27, 2025 •

edited

Loading

zhztheplayer Feb 27, 2025 •

edited

Loading

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

mbasmanova left a comment

mbasmanova Feb 27, 2025

xiaoxmeng Feb 27, 2025 •

edited

Loading

mbasmanova Feb 27, 2025

mbasmanova Feb 27, 2025

mbasmanova Feb 27, 2025

xiaoxmeng Feb 27, 2025 •

edited

Loading

mbasmanova Feb 27, 2025

xiaoxmeng Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

mbasmanova Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025


		IndexJoinCondition(FieldAccessTypedExprPtr _key) : key(std::move(_key)) {}

		folly::dynamic serialize() const override;

feat: Add index join conditions #12461

Are you sure you want to change the base?

feat: Add index join conditions #12461

Conversation

xiaoxmeng commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

netlify bot commented Feb 27, 2025 • edited Loading

✅ Deploy Preview for meta-velox canceled.

zhztheplayer Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova Feb 27, 2025

Choose a reason for hiding this comment

xiaoxmeng Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

mbasmanova Feb 27, 2025

Choose a reason for hiding this comment

mbasmanova Feb 27, 2025

Choose a reason for hiding this comment

mbasmanova Feb 27, 2025

Choose a reason for hiding this comment

xiaoxmeng Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

mbasmanova Feb 27, 2025

Choose a reason for hiding this comment

xiaoxmeng Feb 27, 2025

Choose a reason for hiding this comment

facebook-github-bot commented Feb 27, 2025

mbasmanova Feb 27, 2025

Choose a reason for hiding this comment

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 27, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

facebook-github-bot commented Feb 28, 2025

netlify bot commented Feb 27, 2025 •

edited

Loading

zhztheplayer Feb 27, 2025 •

edited

Loading

xiaoxmeng Feb 27, 2025 •

edited

Loading

xiaoxmeng Feb 27, 2025 •

edited

Loading