Skip to content

Support semi projection join type in smj#10456

Closed
JkSelf wants to merge 1 commit intofacebookincubator:mainfrom
JkSelf:semi-project
Closed

Support semi projection join type in smj#10456
JkSelf wants to merge 1 commit intofacebookincubator:mainfrom
JkSelf:semi-project

Conversation

@JkSelf
Copy link
Copy Markdown
Contributor

@JkSelf JkSelf commented Jul 12, 2024

Return each row from the left or right side with a boolean flag indicating whether there exists a match on the right or left side. If there is a filter, the boolean value will be set based on the result of the filter in the applyFilter method. If there is no filter, the filterOutputForSemiProject is added to set the boolean value based on the records in joinTracker's matchingRows.

@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 Jul 12, 2024
@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 12, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit b460dc5
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6706302253d3530008c9d084

@pedroerp pedroerp self-requested a review July 12, 2024 18:33
@JkSelf
Copy link
Copy Markdown
Contributor Author

JkSelf commented Sep 18, 2024

@pedroerp Can you help to review this PR? Thanks for your help.

Copy link
Copy Markdown
Contributor

@pedroerp pedroerp left a comment

Choose a reason for hiding this comment

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

@JkSelf could you add more context on what exactly is needed to support semi projection joins to the summary, and some documentation on the file header?

@JkSelf
Copy link
Copy Markdown
Contributor Author

JkSelf commented Oct 9, 2024

@JkSelf could you add more context on what exactly is needed to support semi projection joins to the summary, and some documentation on the file header?

@pedroerp Yes. I have refine the summary and also add the comments in the header file. Can you help to review again? Thanks.

Copy link
Copy Markdown
Contributor

@pedroerp pedroerp left a comment

Choose a reason for hiding this comment

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

@JkSelf sorry, this ended up falling out of my radar. Just did another pass and it looks good. Just a few minor comments.

Comment thread velox/exec/MergeJoin.cpp
isRightFlattened_ = false;
}
currentRight_ = right;
if (isRightSemiProjectJoin(joinType_) || isLeftSemiProjectJoin(joinType_)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: maybe add a quick comment here that for semi project joins we need to produce the match output column

Comment thread velox/exec/MergeJoin.cpp
}
currentRight_ = right;
if (isRightSemiProjectJoin(joinType_) || isLeftSemiProjectJoin(joinType_)) {
localColumns[outputType_->size() - 1] = BaseVector::create(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: could we do localColumns.back() = ... instead?

Comment thread velox/exec/MergeJoin.cpp
const auto& filterRows = joinTracker_->matchingRows(numRows);

auto lastChildren = output->children().back();
auto flatMatch = lastChildren->as<FlatVector<bool>>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can't we here do a simple copy of the underlying buffers instead of an explicit for loop?

@stale
Copy link
Copy Markdown

stale Bot commented Feb 20, 2025

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. merge-join stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants