[SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` by panbingkun · Pull Request #45177 · apache/spark

panbingkun · 2024-02-20T03:36:14Z

What changes were proposed in this pull request?

The pr aims to use ordinalNumber to uniformly set the value of paramIndex for the error class UNEXPECTED_INPUT_TYPE.

Why are the changes needed?

When I was reviewing the spark code, I found that:

Some expressions may have a starting value of 1 when throwing error DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE, eg:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala

Lines 300 to 302 in b0aad59

    
           errorSubClass = "UNEXPECTED_INPUT_TYPE", 
        
           messageParameters = Map( 
        
             "paramIndex" -> "1",

Some are 0, eg:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/bitmapExpressions.scala

Lines 117 to 119 in b0aad59

    
           errorSubClass = "UNEXPECTED_INPUT_TYPE", 
        
           messageParameters = Map( 
        
             "paramIndex" -> "0",

We should unify it and avoid understanding differences.

Does this PR introduce any user-facing change?

Yes, the value of 'paramIndex' for the error class UNEXPECTED-INPUT-TYPE is uniformly set by ordinalNumber.

How was this patch tested?

Updated existed UT.
Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

…ass `UNEXPECTED_INPUT_TYPE` should be `1`1

panbingkun · 2024-02-20T06:26:05Z

cc @cloud-fan @MaxGekk

MaxGekk

We should unify it and avoid understanding differences.

I agree we should unify. BTW, did we document somewhere that param indexes are 1-based?

cloud-fan · 2024-02-20T07:54:15Z

Shall we make the error message more explicit?

The <paramIndex>th parameter requires the <requiredType> type, ...

or something more accurate in English. cc @srielau

panbingkun · 2024-02-20T08:20:43Z

We should unify it and avoid understanding differences.

I agree we should unify. BTW, did we document somewhere that param indexes are 1-based?

How about this file?
https://github.com/apache/spark/blob/f0f35c8b1c8f3b1d7f7c2b79945eb29ffb3c8f9a/common/utils/src/main/resources/error/README.md?plain=1

Add the following content at the end of this file?

## Extra
1.The parameter paramIndex of error class  `DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE` is 1-based.

panbingkun · 2024-02-20T08:36:15Z

Shall we make the error message more explicit?
The <paramIndex>th parameter requires the <requiredType> type, ...
or something more accurate in English. cc @srielau

How about the following error message?

The <paramIndex>-th parameter of function <functionName> requires the <requiredType> type, however <inputSql> has the type <inputType>.

I found a reference:
https://github.com/alibaba/sentinel-golang/blob/08071855bc67423777353c6e02f1390b419172b1/core/hotspot/rule.go#L79-L80

MaxGekk · 2024-02-20T12:57:51Z

This <paramIndex>th seems oddly: 1th instead of 1st, 2th instead of 2nd.

How about to reuse the function:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Lines 570 to 574 in fb1e787

    
           def ordinalNumber(i: Int): String = i match { 
        
             case 0 => "first" 
        
             case 1 => "second" 
        
             case 2 => "third" 
        
             case i => s"${i + 1}th"

in the error message.

panbingkun · 2024-02-20T14:10:00Z

This <paramIndex>th seems oddly: 1th instead of 1st, 2th instead of 2nd.

How about to reuse the function:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Lines 570 to 574 in fb1e787

def ordinalNumber(i: Int): String = i match {

case 0 => "first"

case 1 => "second"

case 2 => "third"

case i => s"${i + 1}th"

in the error message.

Okay, I think it's great, let me do it.

…`paramIndex` for the error class `UNEXPECTED_INPUT_TYPE`

panbingkun · 2024-02-21T07:27:14Z

Considering that we have already used ordinalNumber to uniformly set the value of paramIndex for the error class UNEXPECTED_INPUT_TYPE, it is more reasonable for us to use 0-based here.

cloud-fan · 2024-02-21T08:06:31Z

+
+## Extra
+
+- Use `ordinalNumber` to uniformly set the value of `paramIndex`(0-based) for the error class `UNEXPECTED_INPUT_TYPE`.


do we still need this? now people will see first, second, ... nth, etc.

Okay, let me remove it.

MaxGekk · 2024-02-21T13:55:15Z

      parameters = Map(
        "sqlExpr" -> "\"bitmap_count(a)\"",
-        "paramIndex" -> "0",
+        "paramIndex" -> "1",


Should be first?

Yea, two were missed and have been fixed.

MaxGekk · 2024-02-21T13:55:24Z

      parameters = Map(
        "sqlExpr" -> "\"bitmap_or_agg(a)\"",
-        "paramIndex" -> "0",
+        "paramIndex" -> "1",


MaxGekk · 2024-02-21T18:24:46Z

+1, LGTM. Merging to master.
Thank you, @panbingkun and @cloud-fan for review.

…TYPE ### What changes were proposed in this pull request? This PR is a followup of #45177 that fixes some leftovers missed. ### Why are the changes needed? For consistency. Also, I think this fixes the Maven build failure: https://github.com/apache/spark/actions/runs/8005710953/job/21865798408 ### Does this PR introduce _any_ user-facing change? Yes, the value of 'paramIndex' for the error class `UNEXPECTED-INPUT-TYPE` is uniformly set by `ordinalNumber`. ### How was this patch tested? CI in this PR. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #45225 from HyukjinKwon/SPARK-47099-followup. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

dongjoon-hyun

Hi, @panbingkun and all.

This seems to break Java 21 environment too.

https://github.com/apache/spark/actions/runs/8020316098/job/21917446965

This PR missed the following while updating try_arithmetic.sql.out.

sql/core/src/test/resources/sql-tests/results/try_arithmetic.sql.out.java21

Here is a follow-up.

#45235

### What changes were proposed in this pull request? This is a follow-up of #45177 ### Why are the changes needed? To recover Java 21 Daily CI. - https://github.com/apache/spark/actions/workflows/build_maven_java21.yml - https://github.com/apache/spark/actions/runs/8020316098/job/21917446965 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manually with Java 21. ``` $ build/sbt "sql/testOnly *.SQLQueryTestSuite -- -z try_arithmetic" Using /Users/dongjoon/.jenv/versions/21 as default JAVA_HOME. Note, this will be overridden by -java-home if it is set. Using SPARK_LOCAL_IP=localhost [info] welcome to sbt 1.9.3 (Apple Inc. Java 21.0.2) ... [info] SQLQueryTestSuite: 12:33:19.091 WARN org.apache.hadoop.util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable [info] - ansi/try_arithmetic.sql (1 second, 37 milliseconds) [info] - ansi/try_arithmetic.sql_analyzer_test (114 milliseconds) [info] - try_arithmetic.sql (688 milliseconds) [info] - try_arithmetic.sql_analyzer_test (93 milliseconds) 12:33:24.980 WARN org.apache.spark.sql.SQLQueryTestSuite: === Metrics of Analyzer/Optimizer Rules === Total number of runs: 75794 Total time: 0.335704411 seconds Rule Effective Time / Total Time Effective Runs / Total Runs org.apache.spark.sql.catalyst.optimizer.ConstantFolding 59501877 / 60217667 108 / 336 org.apache.spark.sql.catalyst.analysis.An... 12:33:24.981 WARN org.apache.spark.sql.SQLQueryTestSuite: === Metrics of Whole-stage Codegen === Total code generation time: 0.051986919 seconds Total compile time: 0.116905665 seconds 12:33:25.012 WARN org.apache.spark.sql.SQLQueryTestSuite: ===== POSSIBLE THREAD LEAK IN SUITE o.a.s.sql.SQLQueryTestSuite, threads: QueryStageCreator-0 (daemon=true), Idle Worker Monitor for python3 (daemon=true), rpc-boss-3-1 (daemon=true), process reaper (pid 15500) (daemon=true), ForkJoinPool.commonPool-worker-3 (daemon=true), ForkJoinPool.commonPool-worker-2 (daemon=true), shuffle-boss-6-1 (daemon=true), ForkJoinPool.commonPool-worker-1 (daemon=true) ===== [info] Run completed in 8 seconds, 79 milliseconds. [info] Total number of tests run: 4 [info] Suites: completed 1, aborted 0 [info] Tests: succeeded 4, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. [success] Total time: 118 s (01:58), completed Feb 23, 2024, 12:33:25 PM ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #45235 from dongjoon-hyun/SPARK-47099. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…`paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` ### What changes were proposed in this pull request? The pr aims to use `ordinalNumber` to `uniformly` set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE`. ### Why are the changes needed? When I was reviewing the spark code, I found that: - Some expressions may have a starting value of 1 when throwing error `DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE`, eg: https://github.com/apache/spark/blob/b0aad59f123581b66515c864873f46ea4ec4e762/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L300-L302 - Some are 0, eg: https://github.com/apache/spark/blob/b0aad59f123581b66515c864873f46ea4ec4e762/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/bitmapExpressions.scala#L117-L119 **We should `unify` it and avoid understanding differences.** ### Does this PR introduce _any_ user-facing change? Yes, the value of 'paramIndex' for the error class `UNEXPECTED-INPUT-TYPE` is uniformly set by `ordinalNumber`. ### How was this patch tested? - Updated existed UT. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#45177 from panbingkun/SPARK-47099. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…TYPE ### What changes were proposed in this pull request? This PR is a followup of apache#45177 that fixes some leftovers missed. ### Why are the changes needed? For consistency. Also, I think this fixes the Maven build failure: https://github.com/apache/spark/actions/runs/8005710953/job/21865798408 ### Does this PR introduce _any_ user-facing change? Yes, the value of 'paramIndex' for the error class `UNEXPECTED-INPUT-TYPE` is uniformly set by `ordinalNumber`. ### How was this patch tested? CI in this PR. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#45225 from HyukjinKwon/SPARK-47099-followup. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

### What changes were proposed in this pull request? This is a follow-up of apache#45177 ### Why are the changes needed? To recover Java 21 Daily CI. - https://github.com/apache/spark/actions/workflows/build_maven_java21.yml - https://github.com/apache/spark/actions/runs/8020316098/job/21917446965 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manually with Java 21. ``` $ build/sbt "sql/testOnly *.SQLQueryTestSuite -- -z try_arithmetic" Using /Users/dongjoon/.jenv/versions/21 as default JAVA_HOME. Note, this will be overridden by -java-home if it is set. Using SPARK_LOCAL_IP=localhost [info] welcome to sbt 1.9.3 (Apple Inc. Java 21.0.2) ... [info] SQLQueryTestSuite: 12:33:19.091 WARN org.apache.hadoop.util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable [info] - ansi/try_arithmetic.sql (1 second, 37 milliseconds) [info] - ansi/try_arithmetic.sql_analyzer_test (114 milliseconds) [info] - try_arithmetic.sql (688 milliseconds) [info] - try_arithmetic.sql_analyzer_test (93 milliseconds) 12:33:24.980 WARN org.apache.spark.sql.SQLQueryTestSuite: === Metrics of Analyzer/Optimizer Rules === Total number of runs: 75794 Total time: 0.335704411 seconds Rule Effective Time / Total Time Effective Runs / Total Runs org.apache.spark.sql.catalyst.optimizer.ConstantFolding 59501877 / 60217667 108 / 336 org.apache.spark.sql.catalyst.analysis.An... 12:33:24.981 WARN org.apache.spark.sql.SQLQueryTestSuite: === Metrics of Whole-stage Codegen === Total code generation time: 0.051986919 seconds Total compile time: 0.116905665 seconds 12:33:25.012 WARN org.apache.spark.sql.SQLQueryTestSuite: ===== POSSIBLE THREAD LEAK IN SUITE o.a.s.sql.SQLQueryTestSuite, threads: QueryStageCreator-0 (daemon=true), Idle Worker Monitor for python3 (daemon=true), rpc-boss-3-1 (daemon=true), process reaper (pid 15500) (daemon=true), ForkJoinPool.commonPool-worker-3 (daemon=true), ForkJoinPool.commonPool-worker-2 (daemon=true), shuffle-boss-6-1 (daemon=true), ForkJoinPool.commonPool-worker-1 (daemon=true) ===== [info] Run completed in 8 seconds, 79 milliseconds. [info] Total number of tests run: 4 [info] Suites: completed 1, aborted 0 [info] Tests: succeeded 4, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. [success] Total time: 118 s (01:58), completed Feb 23, 2024, 12:33:25 PM ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#45235 from dongjoon-hyun/SPARK-47099. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

[SPARK-47099][SQL] The start value of paramIndex for the error cl…

c389215

…ass `UNEXPECTED_INPUT_TYPE` should be `1`1

panbingkun changed the title ~~[SPARK-47099][SQL] The start value of paramIndex for the error class UNEXPECTED_INPUT_TYPE should be 1~~ [WIP][SPARK-47099][SQL] The start value of paramIndex for the error class UNEXPECTED_INPUT_TYPE should be 1 Feb 20, 2024

github-actions Bot added the SQL label Feb 20, 2024

panbingkun changed the title ~~[WIP][SPARK-47099][SQL] The start value of paramIndex for the error class UNEXPECTED_INPUT_TYPE should be 1~~ [SPARK-47099][SQL] The start value of paramIndex for the error class UNEXPECTED_INPUT_TYPE should be 1 Feb 20, 2024

panbingkun marked this pull request as ready for review February 20, 2024 06:25

MaxGekk reviewed Feb 20, 2024

View reviewed changes

Merge branch 'master' into SPARK-47099

0f67597

panbingkun changed the title ~~[SPARK-47099][SQL] The start value of paramIndex for the error class UNEXPECTED_INPUT_TYPE should be 1~~ [SPARK-47099][SQL] Use ordinalNumber to uniformly set the value of paramIndex for the error class UNEXPECTED_INPUT_TYPE Feb 21, 2024

[SPARK-47099][SQL] Use ordinalNumber to uniformly set the value of …

f58102b

…`paramIndex` for the error class `UNEXPECTED_INPUT_TYPE`

github-actions Bot added the DOCS label Feb 21, 2024

panbingkun added 2 commits February 21, 2024 15:20

[SPARK-47099][SQL] Use ordinalNumber to uniformly set the value of …

7329ce0

…`paramIndex` for the error class `UNEXPECTED_INPUT_TYPE`

Merge branch 'master' into SPARK-47099

c3b3405

cloud-fan reviewed Feb 21, 2024

View reviewed changes

fix UT

0aacb83

cloud-fan approved these changes Feb 21, 2024

View reviewed changes

fix UT

e820a79

MaxGekk approved these changes Feb 21, 2024

View reviewed changes

MaxGekk closed this in 9b53b80 Feb 21, 2024

HyukjinKwon mentioned this pull request Feb 22, 2024

[SPARK-47099][SQL][FOLLOW-UP] Uses ordinalNumber in UNEXPECTED_INPUT_TYPE #45225

Closed

dongjoon-hyun reviewed Feb 23, 2024

View reviewed changes

dongjoon-hyun mentioned this pull request Feb 23, 2024

[SPARK-47099][SQL][FOLLOWUP] Regenerate try_arithmetic.sql.out.java21 #45235

Closed

MaxGekk mentioned this pull request Mar 15, 2024

[SPARK-47007][SQL][PYTHON][R][CONNECT] Add the map_sort function #45069

Closed

	errorSubClass = "UNEXPECTED_INPUT_TYPE",
	messageParameters = Map(
	"paramIndex" -> "1",

	errorSubClass = "UNEXPECTED_INPUT_TYPE",
	messageParameters = Map(
	"paramIndex" -> "0",


		## Extra

		- Use `ordinalNumber` to uniformly set the value of `paramIndex`(0-based) for the error class `UNEXPECTED_INPUT_TYPE`.

Uh oh!

Conversation

panbingkun commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

panbingkun commented Feb 20, 2024

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Feb 20, 2024

Uh oh!

panbingkun commented Feb 20, 2024

Uh oh!

panbingkun commented Feb 20, 2024

Uh oh!

MaxGekk commented Feb 20, 2024

Uh oh!

panbingkun commented Feb 20, 2024

Uh oh!

panbingkun commented Feb 21, 2024

Uh oh!

cloud-fan Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkun Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkun Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Feb 21, 2024

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

panbingkun commented Feb 20, 2024 •

edited

Loading