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

feat(batch): Implement LookupJoinExecutor #3614

Merged
merged 28 commits into from
Jul 7, 2022
Merged

feat(batch): Implement LookupJoinExecutor #3614

merged 28 commits into from
Jul 7, 2022

Conversation

Graphcalibur
Copy link
Contributor

@Graphcalibur Graphcalibur commented Jul 3, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

  • Summarize your change (mandatory)
    • Implements the LookupJoinExecutor, which joins two tables together by retrieving a row from the probe side and looking up matches in the build side
    • In order to send RPC calls, the LookupJoinExecutor needs to be given some ExchangeSource templates so that it can use the data in them (like the host address) to create the actual sources,. I'm not sure if there is a better way to do this so that we don't need to send the LookupJoinExecutor this information.
    • Parsing, binding, and planning to be added in a later PR

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
    • I don't think it's possible to add unit tests because LookupJoinExecutor relies on performing RPC calls to a RowSeqScanExecutor. Integration tests will be added in a later PR
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

#3549 (closed first draft version of this PR)
#2574

@codecov
Copy link

codecov bot commented Jul 3, 2022

Codecov Report

Merging #3614 (6eb930b) into main (bfa5884) will decrease coverage by 0.05%.
The diff coverage is 64.59%.

@@            Coverage Diff             @@
##             main    #3614      +/-   ##
==========================================
- Coverage   74.31%   74.26%   -0.06%     
==========================================
  Files         793      794       +1     
  Lines      111819   112143     +324     
==========================================
+ Hits        83101    83280     +179     
- Misses      28718    28863     +145     
Flag Coverage Δ
rust 74.26% <64.59%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/batch/src/executor/generic_exchange.rs 47.47% <ø> (ø)
src/batch/src/executor/mod.rs 75.80% <ø> (ø)
src/batch/src/executor/join/lookup_join.rs 53.86% <53.86%> (ø)
src/batch/src/executor/join/mod.rs 84.16% <93.90%> (+21.00%) ⬆️
src/batch/src/executor/join/nested_loop_join.rs 77.29% <100.00%> (-2.93%) ⬇️
src/batch/src/executor/test_utils.rs 80.13% <100.00%> (+3.94%) ⬆️
src/common/src/types/ordered_float.rs 24.90% <0.00%> (+0.19%) ⬆️
src/meta/src/manager/id.rs 96.06% <0.00%> (+1.12%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Nice move!

src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
proto/batch_plan.proto Outdated Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Good job. Generally LGTM, jsut some refinements.

src/batch/src/executor/join/lookup_join.rs Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Show resolved Hide resolved
src/batch/src/executor/join/lookup_join.rs Outdated Show resolved Hide resolved
@liurenjie1024 liurenjie1024 linked an issue Jul 6, 2022 that may be closed by this pull request
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit 103d1c2 into main Jul 7, 2022
@mergify mergify bot deleted the steven/lookup-join branch July 7, 2022 06:05
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
* feat(batch): Add skeleton code for lookup join

* feat(expr): Fix issues and mostly complete lookup join implementation

* Implement BoxedExecutorBuilder for LookupJoinExecutor

* feat(batch): Added LookupJoinNode to batch.proto and fix some bugs

* feat(batch): Refactored lookup join

* feat(batch): Add sending RPC calls for RowSeqScan

* feat(batch): Clean up code and add comments

* feat(batch): Fix bug

* feat(batch): Fix names

* feat(batch): Add tests and fix bugs with left outer join

* feat(batch): Simplified LookupJoinNode attributes

* feat(batch): Refactor to iterate through build side rows one by one

* feat(batch): Refactor LookupJoin to use ExchangeExecutors

* feat(batch): Fix comments

* feat(batch): Add small fixes

* feat(batch): Fix warning

* Fix another warning

Co-authored-by: Renjie Liu <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

batch: Implement lookup join executor.
2 participants