Introduce RelBuilder wrapper for dynamic fields #4688

ykmr1224 · 2025-10-28T17:12:49Z

This PR is for feature branch feature/permissive

Description

Introduce RelBuilderWrapper and RelFieldBuilder to abstract the field operations with dynamic fields.
RelBuilderWrapper uses delegation to RelBuilder so that it won't allow direct operation on fields without considering dynamic fields.
- It uses delegation instead of inheritance since RelBuilder internally call field or fields methods and inheritance cannot prohibit access to some methods.
- RelBuilderWrapper hide original field/fields operations, and provide some package private method to allow RelFieldBuilder to access raw operations.
It leaves fixes for several commands to later PR to avoid this PR becomes too big.

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.

Signed-off-by: Tomoyuki Morita <[email protected]>

LantaoJin · 2025-10-30T02:00:33Z

core/src/main/java/org/opensearch/sql/calcite/rel/RelBuilderWrapper.java

+ * direct field access methods to maintain consistency with dynamic fields handling.
+ */
+@RequiredArgsConstructor
+public class RelBuilderWrapper {


why not just leverage the OpenSearchRelBuilder directly?

If you want a derived class of RexBuilder, we can create one as well in OpenSearchPrepareImpl.perform()

As I described in the PR description, inheritance didn't work well since there are some internal access within RelBuilder to the methods we want to prohibit.

Override all fields of raw RelBuilder in OpenSearchRelBuilder cannot work?

In CalciteRelNodeVisitor, all calling to context.relBuilder.fields/field are interrupted in OpenSearchRelBuilder.fields/field.
But in calcite.RelBuilder, all fields/field will access its raw fields/field method. <=== do you want to prohibit this?

Delegation might not work either. For example, RelBuilder.sort() calls RelBuilder.field(). How to prohibit this calling with delegation class?

In CalciteRelNodeVisitor, all calling to context.relBuilder.fields/field are interrupted in OpenSearchRelBuilder.fields/field.
But in calcite.RelBuilder, all fields/field will access its raw fields/field method. <=== do you want to prohibit this?

No, inheritance won't work like that. If we override fields/field in a subclass, even the call from calcite.RelBuilder will call the method in subclass.

Delegation might not work either. For example, RelBuilder.sort() calls RelBuilder.field(). How to prohibit this calling with delegation class?

The idea is that, if the method won't work as we expect, customize or prohibit (and provide alternative way) the usage in RelBuilderWrapper.

Overall, the simple approach is merging current RelFieldBuilder and OpenSearchRelBuilder. But It's okey that we keep the current RelBuilderWrapper.

Oh, I think we still need to use RelBuilder or its derived class OpenSearchRelBuilder because RelBuilder will be used in Calcite optimization rules. For example,

sql/core/src/main/java/org/opensearch/sql/calcite/plan/PPLAggGroupMergeRule.java

Line 84 in c9a8f47

final RelBuilder relBuilder = call.builder();

we do have opportunity to get the OpenSearchRelBuilder from call.builder() but no way to get the wrapper.

How about just add dynamic methods in OpenSearchRelBuilder and keep the original field and fields as static field methods.

Hi Lantao,
Can you revisit my comment?
I think you haven't understood why inheritance won't work.
If we inherit and override method with throwing exception, internal call from other methods in RelBuilder will also fail. And I don't have good idea to workaround this other than reimplementing the method and call super.field instead, which duplicates the implementation from Calcite and not maintainable.

Yes, you are right, in inheritance, if the method calling chain is A() -> B() -> C(). The overridden method A'() will still call the overridden C'(): A'() -> B() -> C'() .

Can we just add @Deprecated and java doc on the override method to callout this method should not be used in the project?

Just a suggestion @ykmr1224 . I am okey on the current POC, we can postpone the problem about how to restrict the usage of RelBuilder in optimize rules (Calcite internal).

core/src/main/java/org/opensearch/sql/calcite/plan/PPLAggregateConvertRule.java

LantaoJin · 2025-10-31T07:09:38Z

core/src/main/java/org/opensearch/sql/calcite/rel/RelFieldBuilder.java

+    return relBuilder.peek(n).getRowType().getFieldNames();
+  }
+
+  public RexInputRef staticField(String fieldName) {


how about simplify all staticField to field?

I named it as staticField so it would be more explicit to access static field.
We want to avoid someone unintentionally omit the existence of dynamic fields.

LantaoJin · 2025-10-31T07:11:57Z

core/src/main/java/org/opensearch/sql/calcite/rel/RelBuilderWrapper.java

+ * direct field access methods to maintain consistency with dynamic fields handling.
+ */
+@RequiredArgsConstructor
+public class RelBuilderWrapper {


Override all fields of raw RelBuilder in OpenSearchRelBuilder cannot work?

In CalciteRelNodeVisitor, all calling to context.relBuilder.fields/field are interrupted in OpenSearchRelBuilder.fields/field.
But in calcite.RelBuilder, all fields/field will access its raw fields/field method. <=== do you want to prohibit this?

Delegation might not work either. For example, RelBuilder.sort() calls RelBuilder.field(). How to prohibit this calling with delegation class?

ykmr1224 self-assigned this Oct 28, 2025

ykmr1224 added enhancement New feature or request PPL Piped processing language calcite calcite migration releated labels Oct 28, 2025

ykmr1224 force-pushed the fieldbuilder branch from 74dd6b8 to 51d0e4e Compare October 28, 2025 18:32

Introduce RelBuilder wrapper for dynamic fields

20e61a1

Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 force-pushed the fieldbuilder branch from 51d0e4e to 20e61a1 Compare October 28, 2025 18:59

ykmr1224 added 2 commits October 28, 2025 13:10

Fix appendcol

b0730b0

Signed-off-by: Tomoyuki Morita <[email protected]>

Fix test

5df8cfc

Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 marked this pull request as ready for review October 28, 2025 22:14

ykmr1224 requested review from GumpacG, LantaoJin, MaxKsyunz, Swiddis, YANG-DB, Yury-Fridlyand, acarbonetto, anirudha, dai-chen, derek-ho, forestmvey, joshuali925, kavithacm, mengweieric, noCharger, penghuo, ps48, qianheng-aws, seankao-az and vamsimanohar as code owners October 28, 2025 22:14

ykmr1224 requested review from RyanL1997 and yuancu as code owners October 28, 2025 22:14

Swiddis approved these changes Oct 29, 2025

View reviewed changes

LantaoJin reviewed Oct 30, 2025

View reviewed changes

core/src/main/java/org/opensearch/sql/calcite/plan/PPLAggregateConvertRule.java Show resolved Hide resolved

LantaoJin reviewed Oct 31, 2025

View reviewed changes

LantaoJin approved these changes Nov 4, 2025

View reviewed changes

ykmr1224 merged commit 8fe17a0 into opensearch-project:feature/permissive Nov 4, 2025
50 of 51 checks passed

Introduce RelBuilder wrapper for dynamic fields #4688

Introduce RelBuilder wrapper for dynamic fields #4688

Uh oh!

Conversation

ykmr1224 commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ykmr1224 commented Oct 28, 2025 •

edited

Loading

LantaoJin Oct 31, 2025 •

edited

Loading

LantaoJin Nov 4, 2025 •

edited

Loading

LantaoJin Nov 4, 2025 •

edited

Loading

LantaoJin Oct 31, 2025 •

edited

Loading