Skip to content

Support join with dynamic fields #4746

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
Open
ykmr1224 wants to merge 7 commits into
base: feature/permissive
Choose a base branch
from
Open

Support join with dynamic fields #4746

ykmr1224 wants to merge 7 commits into from

Conversation

@ykmr1224
Copy link
Collaborator

@ykmr1224 ykmr1224 commented Nov 5, 2025 •
edited
Loading

This PR is for feature branch feature/permissive

Description

  • Support join with dynamic fields (join / lookup)
  • Mainly two modifications were needed:

Related Issues

Permissive mode RFC: #4349
Dynamic fields RFC: #4433

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ykmr1224 ykmr1224 added PPL Piped processing language calcite calcite migration releated labels Nov 5, 2025
@ykmr1224 ykmr1224 self-assigned this Nov 5, 2025
@ykmr1224 ykmr1224 added the enhancement New feature or request label Nov 5, 2025
@ykmr1224 ykmr1224 force-pushed the dynamic-join branch 3 times, most recently from de18e85 to bd31d16 Compare November 6, 2025 17:46
@ykmr1224 ykmr1224 marked this pull request as ready for review November 6, 2025 22:08
@ykmr1224 ykmr1224 force-pushed the dynamic-join branch 3 times, most recently from 56eb6f9 to e2587bd Compare November 11, 2025 22:44
@ykmr1224
Support join with dynamic fields
f7f8f86 
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
@ykmr1224
Fix test failure
379a6f9 
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
@ykmr1224
Fix rexBuilder issue
50830fc 
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
@ykmr1224
Fix join issue
703757c 
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
@ykmr1224
Cast join condition automatically
072bd82 
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
@ykmr1224
Fix IT
ce40b23 
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
Swiddis
Swiddis reviewed Nov 20, 2025
View reviewed changes
Copy link
Collaborator

@Swiddis Swiddis left a comment

Choose a reason for hiding this comment

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

Some design thoughts. I don't see any correctness issues but I'm a little concerned about maintainability. Especially this separate handling for left & right seems error-prone if we revisit this code later.

core/src/main/java/org/opensearch/sql/calcite/rel/RelFieldBuilder.java
Comment on lines +152 to +153
boolean isInLeft;
boolean isInRight;
Copy link
Collaborator

@Swiddis Swiddis Nov 20, 2025

Choose a reason for hiding this comment

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

Is keeping this as two booleans a better choice than having a left/right enum?

Keeping two separate booleans introduces the trivial illegal state of having both of them be true or false, if a code path accidentally triggers it. Enum might be safer.

Copy link
Collaborator Author

@ykmr1224 ykmr1224 Nov 20, 2025

Choose a reason for hiding this comment

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

Field with the same name could exist in both side at the same time, and that's why I ended up with this approach.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
context.relBuilder.literal(context.sysLimit.joinSubsearchLimit())));
}
List<String> leftAllFields = context.fieldBuilder.getAllFieldNames(1);
List<String> rightAllFields = context.fieldBuilder.getAllFieldNames(0);
Copy link
Collaborator

@Swiddis Swiddis Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not entirely the fault of this PR, but CalciteRelNodeVisitor is already a very large file, and I dislike that we're adding more complexity with this logic.

I'd like if we could move the bulk of this visit logic to a dedicated class and leverage that to simplify the method. I did similar refactoring for SPath via rewriteAsEval.

I also think the class would be a better place to encapsulate the left/right distinction, to make it easier to vet that we've applied any relevant logic to both halves.

Copy link
Collaborator Author

@ykmr1224 ykmr1224 Nov 20, 2025

Choose a reason for hiding this comment

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

I totally agree with that.
Although, I don't want to do that in this series of PRs as it would make it difficult to merge back to main branch.

core/src/main/java/org/opensearch/sql/calcite/ExtendedRexBuilder.java
Comment on lines +205 to +213
public RexNode visitCall(RexCall call) {
if (call.getKind() == SqlKind.EQUALS) {
RexNode n0 = call.operands.get(0);
RexNode n1 = call.operands.get(1);
return super.visitCall((RexCall) equalsWithCastAsNeeded(n0, n1));
} else {
return super.visitCall(call);
}
}
Copy link
Collaborator

@Swiddis Swiddis Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

question (non-blocking): Can you explain this more? What's special about EQUALS here?

Copy link
Collaborator Author

@ykmr1224 ykmr1224 Nov 20, 2025

Choose a reason for hiding this comment

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

Ah, forgot to add comment. I'll add comment.
It is a workaround for Calcite issue: https://issues.apache.org/jira/browse/CALCITE-7206

@ykmr1224 ykmr1224 mentioned this pull request Nov 20, 2025
Open
8 tasks
@ykmr1224
Address comments
3cc00f7 
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
@opensearch-trigger-bot
Copy link
Contributor

opensearch-trigger-bot bot commented Dec 5, 2025

This PR is stalled because it has been open for 2 weeks with no activity.

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

Reviewers

@Swiddis Swiddis Swiddis approved these changes

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

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

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

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

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

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

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

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

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

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

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

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

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

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

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

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

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

@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner

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

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

Assignees

@ykmr1224 ykmr1224

Labels

calcite calcite migration releated enhancement New feature or request PPL Piped processing language stalled

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ykmr1224 @Swiddis