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.

Sign up for GitHub

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

Jump to bottom

feat: Add index lookup join node #12163

Closed
wants to merge 1 commit into from
Closed

feat: Add index lookup join node #12163

wants to merge 1 commit into from

Conversation

xiaoxmeng
Copy link
Contributor

Summary:
Add index lookup join node and add a method in table handle to indicate if the table support index lookup
or not.

Differential Revision: D68429744

@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 Jan 24, 2025
@facebook-github-bot
Copy link
Contributor

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

@netlify Netlify
Copy link

netlify bot commented Jan 24, 2025 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4e3c080
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6794112dd84a9c00089a7e26

@facebook-github-bot
Copy link
Contributor

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

@xiaoxmeng xiaoxmeng changed the title feat: add index lookup join node feat: Add index lookup join node Jan 24, 2025
xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
a396606 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
d1834ea 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

mbasmanova
mbasmanova approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@xiaoxmeng Looks great % a few comments. Thank you for adding index join capabilities to Velox.

velox/core/PlanNode.h Outdated
/// SELECT t.sid, t.day_ts, u.event_type
/// FROM t LEFT JOIN u
/// ON t.sid = u.sid
/// AND u.event_type in t.event_list
Copy link
Contributor

Choose a reason for hiding this comment

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

u.event_type in t.event_list

This is not a valid SQL because IN requires a list of literals. Let's change to

contains(t.event_list, u.event_type)

See https://facebookincubator.github.io/velox/functions/presto/array.html#contains

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks and updated.

velox/core/PlanNode.h
/// input is translated to a connector::IndexSource within
/// exec::IndexLookupJoin.
///
/// Take the following query for example, t is left table, r is the right table
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be easier to read if query comes first and break down of its parts after.

Example:

SELECT ....

Here, 
- 'joinType' is JoinType::kLeft
- 'left' describes scan of t with a filter on 'ds': t.ds BETWEEN '2024-01-01' AND '2024-01-07'; 
- 'right' describes indexed table 'u' with ndex keys sid, event_type (and maybe some more)
- 'leftKeys' is a list of one key 't.sid'
- 'rightKeys' is a list of one key 'u.sid'
- 'joinConditions' is a list of one expression: contains(t.event_list, u.event_type)
- 'outputType' contains 3 columns: t.sid, t.day_ts, u.event_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. Thanks for the suggestions!

velox/core/PlanNode.h Outdated
leftKeys,
rightKeys,
/*filter=*/nullptr,
left,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: std::move for left and right (or change signature to take these as const &(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use move

velox/core/PlanNode.h Show resolved Hide resolved
velox/core/PlanNode.h Outdated Show resolved Hide resolved
velox/exec/tests/PlanNodeSerdeTest.cpp Outdated
testSerde(plan);
}

// bad join type.
Copy link
Contributor

Choose a reason for hiding this comment

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

These are nice tests, but they do not belong to 'serde' test. Perhaps, move then into IndexLookupJoinTest.cpp file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will move. Thanks!

velox/exec/tests/PlanNodeSerdeTest.cpp Outdated
auto mockIndexConnectorHandle =
std::make_shared<MockIndexConnectorHandle>("MockIndexConnectorHandle");
auto mockNonIndexConnectorHandle =
std::make_shared<MockNonIndexConnectorHandle>(
Copy link
Contributor

Choose a reason for hiding this comment

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

MockNonIndexConnectorHandle

Do we need this? I assume we use the existing Hive connector, no?

See TEST_F(PlanNodeSerdeTest, scan)

velox/exec/tests/PlanNodeSerdeTest.cpp Outdated
MockNonIndexConnectorHandle::registerSerDe();
MockIndexConnectorHandle::registerSerDe();

auto mockIndexConnectorHandle =
Copy link
Contributor

Choose a reason for hiding this comment

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

mockIndexConnectorHandle

This name is rather long. It is a bit difficult to read and distinguish it from similarly looking mockNonIndexConnectorHandle. Consider renaming mockIndexConnectorHandle to indexHandle and using regular Hive connector for mockNonIndexConnectorHandle.

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
83e865f 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
@xiaoxmeng xiaoxmeng requested a review from assignUser as a code owner January 24, 2025 18:35
@facebook-github-bot
Copy link
Contributor

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

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
213d5a9 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
d32dc20 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
78b036b 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
52c457c 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
eb29398 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

xiaoxmeng added a commit to xiaoxmeng/velox that referenced this pull request Jan 24, 2025
@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
b7b14f1 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

@xiaoxmeng @facebook-github-bot
feat: Add index lookup join plan node (facebookincubator#12163)
4e3c080 
Summary:

Add index lookup join plan node and add a method in table handle to indicate if the table support index lookup
or not.

Reviewed By: mbasmanova

Differential Revision: D68429744
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in cac8000.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 0 benchmark runs that triggered this notification.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

@xiaoxmeng xiaoxmeng deleted the export-D68429744 branch January 25, 2025 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbasmanova mbasmanova mbasmanova approved these changes

@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

@assignUser assignUser Awaiting requested review from assignUser assignUser is a code owner

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xiaoxmeng @facebook-github-bot @mbasmanova