[CALCITE-6999] Invalid unparse for TRIM in PrestoDialect by xuzifu666 · Pull Request #4360 · apache/calcite

xuzifu666 · 2025-05-04T09:46:47Z

As description in JIRA: https://issues.apache.org/jira/browse/CALCITE-6999

xiedeyantu

LGTM

caicancai

Why the BigQuery dialect files were modified ?

xuzifu666 · 2025-05-05T08:15:55Z

Why the BigQuery dialect files were modified ?

The logic can be reused by other dialects，so move it to RelToSqlConverterUtil avoid duplicate code. @caicancai

caicancai · 2025-05-05T08:38:15Z

There are two reasons. The first is that you cannot be sure that every dialect has this function and the format is the same. The second is that you did not modify the above comment.

xuzifu666 · 2025-05-05T08:47:07Z

There are two reasons. The first is that you cannot be sure that every dialect has this function and the format is the same. The second is that you did not modify the above comment.

Yes, here need to adapt system which support trim/ltrim/rtrim functions and presto is supported well(which test result in JIRA:https://issues.apache.org/jira/browse/CALCITE-6999), and I had updated the comments with presto link. thanks for reminder @caicancai

caicancai · 2025-05-05T08:50:26Z

There are two reasons. The first is that you cannot be sure that every dialect has this function and the format is the same. The second is that you did not modify the above comment.

Yes, here need to adapt system which support trim/ltrim/rtrim functions and presto is supported well(which test result in JIRA:https://issues.apache.org/jira/browse/CALCITE-6999), and I had updated the comments with presto link. thanks for reminder @caicancai

So now all dialects support trim function parsing?

xuzifu666 · 2025-05-05T08:52:43Z

There are two reasons. The first is that you cannot be sure that every dialect has this function and the format is the same. The second is that you did not modify the above comment.

Yes, here need to adapt system which support trim/ltrim/rtrim functions and presto is supported well(which test result in JIRA:https://issues.apache.org/jira/browse/CALCITE-6999), and I had updated the comments with presto link. thanks for reminder @caicancai

So now all dialects support trim function parsing?

This can not confirm，but which need to use(like BigQuery/Presto) to convert right way can use the method.

mihaibudiu · 2025-05-05T18:46:00Z

core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java

+   * <a href="https://prestodb.io/docs/current/functions/string.html#trim-string-varchar">
+   * Presto Trim Function</a>.
+   */
+  public static void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec,


if this function is specific to BigQuery, the function name should reflect that.
I would not abbreviate BQ in the JavaDoc, why force people to try to guess what it is when you can just write it down?

Yes，make sense， this method not only used for BigQuery，can be used by any need convert trim(both from) to trim/ltrim/rtrim functions, so method name may not need to be changed, I had updated the bbreviate BQ in JavaDoc.Thanks for suggestion @mihaibudiu

the current name is too generic, this function is not suitable for unparsingTrim for all dialects, as the name would suggest
I think unparseTrimBigQuery is better, or unparseTrimLR if you want.
Many dialects can share it.

Yes, I thought unparseTrimLR is better, so changed the function to unparseTrimLR~ @mihaibudiu

mihaibudiu · 2025-05-05T18:46:28Z

core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java

+    call.operand(2).unparse(writer, leftPrec, rightPrec);
+
+    // If the trimmed character is a non-space character, add it to the target SQL.
+    // eg: TRIM(BOTH 'A' from 'ABCD'


missing parens in comment

mihaibudiu · 2025-05-05T18:47:59Z

core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java

+    // eg: TRIM(BOTH 'A' from 'ABCD'
+    // Output Query: TRIM('ABC', 'A')
+    String value = requireNonNull(valueToTrim.toValue(), "valueToTrim.toValue()");
+    if (!value.matches("\\s+")) {


is this correct?
is trim(s, "\n") the same as trimming all whitespaces?

Here should be correct, I had test for it:

select trim(' aa c', ' '); >aa c

trim with '\n'

select trim(' aa c', '\n'); > aa c

trim(s, "\n") is not the same as trimming all whitespaces.

What about a literal tab character (not the \t escape sequence)?

the regexp \s+ will match a tab, or even a sequence of spaces and tabs and newlines

Yes，you are right，maybe we can use StringUtils.isWhitespace(value) to instead of it? which can only match white spaces. @mihaibudiu

xuzifu666 · 2025-05-06T11:03:47Z

Hi @mihaibudiu all comments had been addressed, whether is OK currently?

mihaibudiu · 2025-05-06T20:37:40Z

core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java

+    // eg: TRIM(BOTH 'A' from 'ABCD')
+    // Output Query: TRIM('ABC', 'A')
+    String value = requireNonNull(valueToTrim.toValue(), "valueToTrim.toValue()");
+    if (!StringUtils.isWhitespace(value)) {


I still don't think this is correct.
The spec says that the default value is just a single space.
So the test should be for value.equals(" "), or it can be removed altogether.

Make sense, I agree with it and had changed it, PTAL thanks @mihaibudiu

mihaibudiu

you can squash the commits

xuzifu666 · 2025-05-08T05:21:23Z

Thanks for review，I had squashed commits. @mihaibudiu

sonarqubecloud · 2025-05-08T05:52:02Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

xuzifu666 force-pushed the support_trim_presto_mysql branch 3 times, most recently from 135157d to b9c2983 Compare May 4, 2025 10:22

xiedeyantu approved these changes May 4, 2025

View reviewed changes

caicancai reviewed May 5, 2025

View reviewed changes

xiedeyantu mentioned this pull request May 5, 2025

[CALCITE-6997] SQLite dialect implementation #4356

Merged

mihaibudiu reviewed May 5, 2025

View reviewed changes

mihaibudiu reviewed May 6, 2025

View reviewed changes

xuzifu666 requested a review from mihaibudiu May 8, 2025 01:39

mihaibudiu approved these changes May 8, 2025

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label May 8, 2025

[CALCITE-6999] Invalid unparse for TRIM in PrestoDialect

089c626

xuzifu666 force-pushed the support_trim_presto_mysql branch from 7aa2abb to 089c626 Compare May 8, 2025 05:20

mihaibudiu merged commit 06355b6 into apache:main May 8, 2025
20 checks passed

Conversation

xuzifu666 commented May 4, 2025

Uh oh!

xiedeyantu left a comment

Choose a reason for hiding this comment

Uh oh!

caicancai left a comment

Choose a reason for hiding this comment

Uh oh!

xuzifu666 commented May 5, 2025

Uh oh!

caicancai commented May 5, 2025

Uh oh!

xuzifu666 commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caicancai commented May 5, 2025

Uh oh!

xuzifu666 commented May 5, 2025

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

xuzifu666 May 6, 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!

xuzifu666 commented May 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzifu666 May 7, 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!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

xuzifu666 commented May 8, 2025

Uh oh!

sonarqubecloud bot commented May 8, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xuzifu666 commented May 5, 2025 •

edited

Loading

xuzifu666 May 6, 2025 •

edited

Loading

xuzifu666 May 7, 2025 •

edited

Loading