[CALCITE-5984] Allow disabling field trim via `SqlToRelConverter` config #3436

WegdanGhazi · 2023-09-21T13:58:52Z

CALCITE-5984 Allow disabling field trim via SqlToRelConverter config

Use the value of isTrimUnusedFields set to SqlToRelConverter.Config to determine whether to trim unused fields, making it possible to completely disable trimming.

As seen here, the Prepare#trim function creates a new instance of SqlToRelConverter.Config, overwriting existing isTrimUnusedFields settings.

This PR fixes this by checking the boolean trimUnusedFields before calling #trim.

Unit tests were added to ensure that trimming can be completely disabled by controlling:

Disabling isTrimUnusedFields on SqlToRelConverter.Config.
Removing TrimFieldsProgram from the initialization of the optimization program.

LakeShen · 2023-09-22T10:31:55Z

core/src/main/java/org/apache/calcite/prepare/Prepare.java


-    Hook.TRIMMED.run(root.rel);


I see that this is same code,why you changed this?

There's no change in the code itself, I've only moved this logic inside a condition based on the config that determined whether or not to trim unused fields

LakeShen · 2023-09-22T14:11:37Z

core/src/test/java/org/apache/calcite/test/JdbcTest.java

+                + "  join \"hr\".\"emps\" as \"e\" on \"d\".\"deptno\" = \"e\".\"deptno\" \n")
+        .withHook(Hook.SQL2REL_CONVERTER_CONFIG_BUILDER,
+            (Consumer<Holder<Config>>) configHolder ->
+                configHolder.set(configHolder.get().withTrimUnusedFields(true)))


I see the sql is same as above,the withTrimUnusedFields method has different parameters, but the plan is the same.

Why did you use this SQL example？

Allowing field trimming happens by

Specifying SQL2RelConverter Config
or

Specifying a Program that contains the Field trimming program (the standard program)

I tried to showcase all the different ways config 1 & 2 will enable trimming, while trimming will be disabled if both 1. Converter config and 2. Program Config disable it

Hi @WegdanGhazi ,thanks for your reply.

I have saw the difference between testJoinWithTrimmingEnabledByProgramAndConfig and testJoinWithTrimmingEnabledByProgram is the withTrimUnusedFields param value,one is false,another is true.

But the input sql and expected plan is same,so I'm a little confused about the purpose of these two unit tests.I thought a few code comments illustrating this case would make it easier for others to understand.

Got it! Will add comments, thanks!

LakeShen · 2023-09-23T02:21:07Z

core/src/test/java/org/apache/calcite/test/JdbcTest.java

@@ -8247,6 +8343,33 @@ private static String sums(int n, boolean c) {
    return b.toString();
  }

+  /** A program that omits {@link TrimFieldsProgram}. */
+  private static Program getProgram() {


Maybe could use org.apache.calcite.tools.Programs#standard(org.apache.calcite.rel.metadata.RelMetadataProvider) method.

The standard program is initialized as follows:

return sequence(subQuery(metadataProvider), new DecorrelateProgram(), new TrimFieldsProgram(), program1, // Second planner pass to do physical "tweaks". This the first time // that EnumerableCalcRel is introduced. calc(metadataProvider));

Which includes the TrimFieldsProgram(), so I had to manually initialize one to avoid adding that program.
What do you think? Do you suggest a better way to create the program without TrimFieldsProgram()?

Could we make the standard(RelMetadataProvider) method has parameters with RelMetadataProvider and boolean flag?

The boolean flag indicates whether TrimFieldsProgram should be added,The standard() method default value is true.I finded Programs#standard(RelMetadataProvider) method only used in Programs.

That sounds good! Will go for that!

LakeShen · 2023-09-23T02:25:11Z

@WegdanGhazi thank you for opening this PR, left a couple of comments!

sonarcloud · 2023-09-26T09:17:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.7% Coverage
0.0% Duplication

LakeShen · 2023-09-28T03:21:05Z

Hi @WegdanGhazi ,If you think we could add this feature in 1.36.0,please change this jira Fix Version to 1.36.0.

And also need the others review.

WegdanGhazi · 2023-10-10T07:50:03Z

Hi @LakeShen, Thank you so much for the review! The current state is that we're waiting for more reviews, right?

LakeShen · 2023-10-10T11:04:58Z

Yes,you are right.

zstan · 2023-10-24T14:00:11Z

core/src/test/java/org/apache/calcite/test/JdbcTest.java

+   * <a href="https://issues.apache.org/jira/browse/CALCITE-5984">[CALCITE-5984]</a>
+   * Disabling trimming of unused fields via config and program.
+   * Case: trimmingByProgram: false ; trimmingByConfig: true. */
+  @Test void testJoinWithTrimmingEnabledByConfig() {


you can use @ParameterizedTest from junit5 or make your custom input collection and unite 3 below tests into one, wdyt ?

Stephan202

🚀

libenchao · 2023-10-31T12:11:16Z

core/src/main/java/org/apache/calcite/tools/Programs.java

  }

  /** Returns the standard program with user metadata provider. */
-  public static Program standard(RelMetadataProvider metadataProvider) {
+  public static Program standard(RelMetadataProvider metadataProvider,


I'm afraid that this is a public API that we need to consider compatibility.

WDYT about using method overloads to keep the functionality? Pushed something.

libenchao · 2023-10-31T12:23:16Z

testkit/src/main/java/org/apache/calcite/test/CalciteAssert.java

@@ -679,6 +679,7 @@ static void assertPrepare(
      Connection connection,
      String sql,
      boolean materializationsEnabled,
+      List<Pair<Hook, Consumer>> hooks,


This is also a public API (testkit are expected to be used by third party projects)

Added an overload function to maintain the public API.

sonarcloud · 2023-10-31T15:33:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

81.8% Coverage
0.0% Duplication

libenchao

+1

[CALCITE-5984] Allow disabling field trim via SqlToRelConverter config

5a39ca3

LakeShen reviewed Sep 22, 2023

View reviewed changes

LakeShen reviewed Sep 23, 2023

View reviewed changes

[CALCITE-5984] Add test case comments

26ee209

WegdanGhazi requested a review from LakeShen September 25, 2023 07:30

[CALCITE-5984] Add boolean flag to program init

1c8346e

zstan reviewed Oct 24, 2023

View reviewed changes

[CALCITE-5984] Use ParameterizedTest in testJoinWithTrimmingConfigs

b0612e0

WegdanGhazi requested a review from zstan October 28, 2023 13:38

Stephan202 approved these changes Oct 28, 2023

View reviewed changes

libenchao reviewed Oct 31, 2023

View reviewed changes

[CALCITE-5984] Add method overloads for public API

965fac1

WegdanGhazi requested a review from libenchao October 31, 2023 16:02

libenchao approved these changes Nov 1, 2023

View reviewed changes

libenchao closed this in 8af1f11 Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-5984] Allow disabling field trim via `SqlToRelConverter` config #3436

[CALCITE-5984] Allow disabling field trim via `SqlToRelConverter` config #3436

WegdanGhazi commented Sep 21, 2023 •

edited

Loading

LakeShen Sep 22, 2023

WegdanGhazi Sep 22, 2023

LakeShen Sep 22, 2023

WegdanGhazi Sep 22, 2023

LakeShen Sep 23, 2023

WegdanGhazi Sep 25, 2023

LakeShen Sep 23, 2023

WegdanGhazi Sep 25, 2023

LakeShen Sep 26, 2023

WegdanGhazi Sep 26, 2023 •

edited

Loading

LakeShen commented Sep 23, 2023

sonarcloud bot commented Sep 26, 2023

LakeShen commented Sep 28, 2023

WegdanGhazi commented Oct 10, 2023

LakeShen commented Oct 10, 2023

zstan Oct 24, 2023

Stephan202 left a comment

libenchao Oct 31, 2023

WegdanGhazi Oct 31, 2023

libenchao Oct 31, 2023

WegdanGhazi Oct 31, 2023 •

edited

Loading

sonarcloud bot commented Oct 31, 2023

libenchao left a comment

[CALCITE-5984] Allow disabling field trim via SqlToRelConverter config #3436

[CALCITE-5984] Allow disabling field trim via SqlToRelConverter config #3436

Conversation

WegdanGhazi commented Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WegdanGhazi Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

LakeShen commented Sep 23, 2023

sonarcloud bot commented Sep 26, 2023

LakeShen commented Sep 28, 2023

WegdanGhazi commented Oct 10, 2023

LakeShen commented Oct 10, 2023

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WegdanGhazi Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Oct 31, 2023

libenchao left a comment

Choose a reason for hiding this comment

[CALCITE-5984] Allow disabling field trim via `SqlToRelConverter` config #3436

[CALCITE-5984] Allow disabling field trim via `SqlToRelConverter` config #3436

WegdanGhazi commented Sep 21, 2023 •

edited

Loading

WegdanGhazi Sep 26, 2023 •

edited

Loading

WegdanGhazi Oct 31, 2023 •

edited

Loading