Skip to content
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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1bd8eca
[CALCITE-6145] Function 'TRIM' without parameters throw NullPointerEx…
timgrein Jul 19, 2024
295c069
Add test for trim(null)
timgrein Jul 22, 2024
ccd20a0
Improve error message when trim has no args
timgrein Jul 22, 2024
a3cb265
Align error message
timgrein Aug 7, 2024
befabe0
Improve explanatory comment
timgrein Aug 7, 2024
344d70c
Fix line length error
timgrein Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/src/main/java/org/apache/calcite/sql/fun/SqlTrimFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,13 @@ public SqlTrimFunction(String name, SqlKind kind,
if (operands[1] == null) {
operands[1] = SqlLiteral.createCharString(" ", pos);
}
if (operands[2] == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@timgrein timgrein Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, currently adjusting this

Copy link
Contributor Author

@timgrein timgrein Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the first two arguments have some default values

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

// This variant occurs, when someone writes TRIM() without any arguments as the first two
// absent arguments are set to default values and the third argument (string to trim)
// stays null
throw new IllegalArgumentException(
"Invalid number of arguments to function 'TRIM'. Was expecting at least 2 arguments");
}
break;
default:
throw new IllegalArgumentException(
Expand Down
2 changes: 2 additions & 0 deletions testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10843,6 +10843,7 @@
f.checkString("trim(trailing 'a' from 'aAa')", "aA", "VARCHAR(3) NOT NULL");
f.checkNull("trim(cast(null as varchar(1)) from 'a')");
f.checkNull("trim('a' from cast(null as varchar(1)))");
f.checkNull("trim(null)");

// SQL:2003 6.29.9: trim string must have length=1. Failure occurs
// at runtime.
Expand All @@ -10857,6 +10858,7 @@
f.checkFails("trim('' from 'abcde')",
"Trim error: trim character must be exactly 1 character",
true);
f.checkFails("trim()", "Invalid number of arguments to function 'TRIM'. Was expecting at least 2 arguments", false);

final SqlOperatorFixture f1 = f.withConformance(SqlConformanceEnum.MYSQL_5);
f1.checkString("trim(leading 'eh' from 'hehe__hehe')", "__hehe",
Expand Down Expand Up @@ -11203,7 +11205,7 @@
+ "multiset union all multiset[1, 4, 5, 7, 8]) "
+ "submultiset of multiset[1, 2, 3, 4, 5, 7, 8]",
false);
f.checkBoolean("(multiset[1, 2, 3, 4, 2] "

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / Linux (JDK 21) 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / Linux (JDK 17) 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / Linux (JDK 8), oldest Guava, America/New_York Timezone 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / Linux (JDK 11), Avatica main 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / Linux (JDK 11), Pacific/Chatham Timezone 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / Linux (JDK 22) 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / Linux (JDK 8), latest Guava, America/New_York Timezone 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).

Check failure on line 11208 in testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

View workflow job for this annotation

GitHub Actions / macOS (JDK 21) 

[Task :testkit:checkstyleMain] [LineLength] Line is longer than 100 characters (found 120).
+ "multiset union all multiset[1, 4, 5, 7, 8]) "
+ "submultiset of multiset[1, 1, 2, 2, 3, 4, 4, 5, 7, 8]",
true);
Expand Down
Loading