[CALCITE-6145] Function 'TRIM' without parameters throw NullPointerEx… #3869

timgrein · 2024-07-19T22:10:14Z

Added an explicit check at parsing time to make sure that the string to trim is not null

…ception

caicancai

You should improve the description in reference.md

timgrein · 2024-07-21T17:48:31Z

You should improve the description in reference.md

Do you've something explicit on your mind? I've checked other functions from Character string operators and functions inside reference.md and it doesn't seem to be common to explicitly specify behavior, if certain args are null. I've fixed one issue though in another change: TRIM function documentation refers to string1 two times. Did you have this on your mind?

caicancai · 2024-07-22T00:58:22Z

You should improve the description in reference.md

Do you've something explicit on your mind? I've checked other functions from Character string operators and functions inside reference.md and it doesn't seem to be common to explicitly specify behavior, if certain args are null. I've fixed one issue though in another change: TRIM function documentation refers to string1 two times. Did you have this on your mind?

Thanks for reminding me, I didn't notice this.

mihaibudiu · 2024-07-19T22:12:15Z

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

@@ -10857,6 +10857,7 @@ void assertSubFunReturns(boolean binary, String s, int start,
    f.checkFails("trim('' from 'abcde')",
        "Trim error: trim character must be exactly 1 character",
        true);
+    f.checkFails("trim()", "String to trim cannot be null", false);


Is this the expected error for this function?
Or should the result be null?

The creator of the original https://issues.apache.org/jira/browse/CALCITE-6145 suggested to throw an exception with an exact description.

I've checked the behavior of PostgreSQL (v15):

SELECT TRIM() AS trimmed_string;

leads to a syntax error: Query Error: error: syntax error at or near ")".

SELECT TRIM("") AS trimmed_string;

leads to an error: Query Error: error: zero-length delimited identifier at or near """".

SELECT TRIM(null) AS trimmed_string;

returns null.

So I think this aligns with the behavior of PostgreSQL throwing an error during query parsing, but I think the message could be a bit better: trim cannot be called without arguments maybe?

I've added an explicit test for when the value is null, which indeed returns null (aligns with PostgreSQL's behavior).

WDYT? @mihaibudiu

If the function trim() need to have arguments, then the type checker should have complained about not finding a function with a signature without arguments. I don't expect the parser to complain about it, that is strange. The parser should just accept the function, and the type checker should validate it.

The original message is not good, since clearly there is no 'null' involved. However, if the error comes from the parser, there isn't much you can do about it.

mihaibudiu · 2024-07-22T20:21:47Z

core/src/main/java/org/apache/calcite/sql/fun/SqlTrimFunction.java

@@ -130,6 +130,9 @@ public SqlTrimFunction(String name, SqlKind kind,
      if (operands[1] == null) {
        operands[1] = SqlLiteral.createCharString(" ", pos);
      }
+      if (operands[2] == null) {
+        throw new IllegalArgumentException("String to trim cannot be null");


"The second argument of trim cannot be null"
It's not clear what "string" you are referring to.

I think it's either the first or the third:

You can specify one argument (only the string to trim) or three (where to trim (leading, trailing both sides), what to trim from the string, the string to trim).

"The second argument of trim cannot be null"

This is not 100% correct I think as the string to trim from can be null: trim('a' from cast(null as varchar(1))) returns null, which is valid. I think it's more about the absence of the argument, which feels to me like a different thing than a value/field being null.

Ideally we should correctly fall through to the default case or handle the cases, where the string to trim is absent explicitly.

mihaibudiu · 2024-07-22T20:58:03Z

core/src/main/java/org/apache/calcite/sql/fun/SqlTrimFunction.java

@@ -130,6 +130,9 @@ public SqlTrimFunction(String name, SqlKind kind,
      if (operands[1] == null) {
        operands[1] = SqlLiteral.createCharString(" ", pos);
      }
+      if (operands[2] == null) {


something is wrong here. If you reached this point you have one argument.

I think I've two (flag from which side(s) to trim, string to trim from the 3rd operand) right?

For trim(both 'a' from 'aAa') you've three operands:

operand[0]: "BOTH" operand[1]: "a" operand[2]: "aAa"

yes, and the message says that there are "no arguments"

Yup, currently adjusting this

Ok so I've debugged through this:

If you call trim() the following happens:

Enters the block for 3 operands (first gets a default value, second and third are null, not in the sense of their value, but of their absence)

operands[0] gets set to BOTH

operands[1] gets so to " "

operands[2] is null (indicating absence of the arg not that its actual value is null)

So the actual method call from a user perspective was one without arguments (trim()).

trim(" a ") works fine
trim(both " a ") works fine
trim("a" from) syntax error (string to trim is absent)
trim(both "a" from) syntax error (string to trim is absent)

So to handle the no-args case I think it's correct to check for if (operands[2] == null) in the block with 3 operands. I'll add a comment explaining it, otherwise it's confusing I think.

yes, the first two arguments have some default values

@mihaibudiu @timgrein Perhaps we can refer to SparkSQL's exception information.
SparkSQL:

spark-sql> select trim(); Error in query: Invalid number of arguments for function trim. Expected: one of 1 and 2; Found: 0; line 1 pos 7

mihaibudiu · 2024-07-25T18:44:28Z

Please let us know when this is again ready for review.

timgrein · 2024-07-25T21:37:20Z

Please let us know when this is again ready for review.

Looking into this again, is there any guidance on where these type of validations/semantic analysis should live? I see there's another method called validateCall, which feels like a more appropriate place for these kind of validations.

mihaibudiu · 2024-07-25T21:39:12Z

I think your code is in the right place.
the validation methods do not know anything about the function being called, just its type signature.

mihaibudiu · 2024-07-30T17:49:51Z

So it this ready for review?

timgrein · 2024-08-06T07:53:08Z

So it this ready for review?

@mihaibudiu Sorry I was on vacation the last week, I'll give it a read again and let you know, whether it's ready to review!

timgrein · 2024-08-07T10:57:52Z

Thank you for your patience!

I've adjusted the error message to follow what Calcite uses at a lot of other places Invalid number of arguments to function.... @mihaibudiu also added an explanatory comment, so it's ready to review again. Let me know, if you want to use a different approach or if this fine as a bug fix.

Edit: grepped through the codebase and it seems like messages starting with Invalid number of arguments to function... shouldn't be constructed manually, but rather thrown through CalciteResource#invalidArgCount during type checking (there's also only one instance, where this is called in a specific function SqlDatePartFunction). I'm wondering if a better fix should adapt the corresponding type checker. Maybe something like ONE_OR_MORE would be useful before checking ANY_STRING_STRING? I think that's also something you've mentioned already, I'll check if I can move the fix to the type checker, although I'm not sure yet what's the order of execution (call creation before type checking?).

sonarqubecloud · 2024-08-07T11:17:43Z

Quality Gate passed

Issues
3 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 SonarCloud

mihaibudiu · 2024-08-07T18:46:34Z

Looks like you have some lines that are too long.
You can validate your commits before submitting them by running ./gradlew build

mihaibudiu · 2024-08-13T17:24:41Z

@timgrein this is almost ready, with just a little more work we can merge it

github-actions · 2024-12-05T03:42:27Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

[CALCITE-6145] Function 'TRIM' without parameters throw NullPointerEx…

1bd8eca

…ception

caicancai reviewed Jul 19, 2024

View reviewed changes

timgrein requested a review from caicancai July 21, 2024 18:02

caicancai approved these changes Jul 22, 2024

View reviewed changes

mihaibudiu reviewed Jul 22, 2024

View reviewed changes

Add test for trim(null)

295c069

timgrein requested a review from mihaibudiu July 22, 2024 20:18

Improve error message when trim has no args

ccd20a0

mihaibudiu reviewed Jul 22, 2024

View reviewed changes

timgrein added 2 commits August 7, 2024 12:53

Align error message

a3cb265

Improve explanatory comment

befabe0

timgrein requested a review from mihaibudiu August 7, 2024 10:55

Fix line length error

344d70c

github-actions bot added the stale label Dec 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6145] Function 'TRIM' without parameters throw NullPointerEx… #3869

[CALCITE-6145] Function 'TRIM' without parameters throw NullPointerEx… #3869

timgrein commented Jul 19, 2024 •

edited

Loading

caicancai left a comment

timgrein commented Jul 21, 2024

caicancai commented Jul 22, 2024

mihaibudiu Jul 19, 2024

timgrein Jul 22, 2024

mihaibudiu Jul 22, 2024

mihaibudiu Jul 22, 2024

timgrein Jul 22, 2024

timgrein Jul 22, 2024 •

edited

Loading

mihaibudiu Jul 22, 2024

timgrein Jul 22, 2024 •

edited

Loading

mihaibudiu Jul 22, 2024

timgrein Jul 22, 2024

timgrein Jul 22, 2024 •

edited

Loading

mihaibudiu Jul 22, 2024

NobiGo Jul 23, 2024

mihaibudiu commented Jul 25, 2024

timgrein commented Jul 25, 2024

mihaibudiu commented Jul 25, 2024

mihaibudiu commented Jul 30, 2024

timgrein commented Aug 6, 2024 •

edited

Loading

timgrein commented Aug 7, 2024 •

edited

Loading

sonarqubecloud bot commented Aug 7, 2024

mihaibudiu commented Aug 7, 2024

mihaibudiu commented Aug 13, 2024

github-actions bot commented Dec 5, 2024

[CALCITE-6145] Function 'TRIM' without parameters throw NullPointerEx… #3869

Are you sure you want to change the base?

[CALCITE-6145] Function 'TRIM' without parameters throw NullPointerEx… #3869

Conversation

timgrein commented Jul 19, 2024 • edited Loading

caicancai left a comment

Choose a reason for hiding this comment

timgrein commented Jul 21, 2024

caicancai commented Jul 22, 2024

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

timgrein Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgrein Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgrein Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Jul 25, 2024

timgrein commented Jul 25, 2024

mihaibudiu commented Jul 25, 2024

mihaibudiu commented Jul 30, 2024

timgrein commented Aug 6, 2024 • edited Loading

timgrein commented Aug 7, 2024 • edited Loading

sonarqubecloud bot commented Aug 7, 2024

Quality Gate passed

mihaibudiu commented Aug 7, 2024

mihaibudiu commented Aug 13, 2024

github-actions bot commented Dec 5, 2024

timgrein commented Jul 19, 2024 •

edited

Loading

timgrein Jul 22, 2024 •

edited

Loading

timgrein Jul 22, 2024 •

edited

Loading

timgrein Jul 22, 2024 •

edited

Loading

timgrein commented Aug 6, 2024 •

edited

Loading

timgrein commented Aug 7, 2024 •

edited

Loading