You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2024/02/20 03:36:17 UTC
[PR] [SPARK-47099][SQL] The `start` value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` should be `1` [spark]
panbingkun opened a new pull request, #45177:
URL: https://github.com/apache/spark/pull/45177
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
Pass GA.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1957627936
+1, LGTM. Merging to master.
Thank you, @panbingkun and @cloud-fan for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45177:
URL: https://github.com/apache/spark/pull/45177#discussion_r1497060181
##########
common/utils/src/main/resources/error/README.md:
##########
@@ -117,3 +117,7 @@ The following SQLSTATEs are collated from:
- Oracle 12 (last published)
- SQL Server
- Redshift
+
+## Extra
+
+- Use `ordinalNumber` to uniformly set the value of `paramIndex`(0-based) for the error class `UNEXPECTED_INPUT_TYPE`.
Review Comment:
do we still need this? now people will see first, second, ... nth, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1956040744
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] The `start` value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` should be `1` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1953713901
> 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
<img width="751" alt="image" src="https://github.com/apache/spark/assets/15246973/07276789-15cd-4ad3-b2fd-4809671df1dc">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45177:
URL: https://github.com/apache/spark/pull/45177#discussion_r1497603925
##########
sql/core/src/test/scala/org/apache/spark/sql/BitmapExpressionsQuerySuite.scala:
##########
@@ -217,7 +217,7 @@ class BitmapExpressionsQuerySuite extends QueryTest with SharedSparkSession {
errorClass = "DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE",
parameters = Map(
"sqlExpr" -> "\"bitmap_count(a)\"",
- "paramIndex" -> "0",
+ "paramIndex" -> "1",
Review Comment:
Should be `first`?
##########
sql/core/src/test/scala/org/apache/spark/sql/BitmapExpressionsQuerySuite.scala:
##########
@@ -239,7 +239,7 @@ class BitmapExpressionsQuerySuite extends QueryTest with SharedSparkSession {
errorClass = "DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE",
parameters = Map(
"sqlExpr" -> "\"bitmap_or_agg(a)\"",
- "paramIndex" -> "0",
+ "paramIndex" -> "1",
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] The `start` value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` should be `1` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1954163160
This `<paramIndex>th` seems oddly: `1th` instead of `1st`, `2th` instead of `2nd`.
How about to reuse the function:
https://github.com/apache/spark/blob/fb1e7872a3e64eab6127f9c2b3ffa42b63162f6c/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala#L570-L574
in the error message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] The `start` value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` should be `1` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1953561931
cc @cloud-fan @MaxGekk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] The `start` value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` should be `1` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1954295467
> This `<paramIndex>th` seems oddly: `1th` instead of `1st`, `2th` instead of `2nd`.
>
> How about to reuse the function:
>
> https://github.com/apache/spark/blob/fb1e7872a3e64eab6127f9c2b3ffa42b63162f6c/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala#L570-L574
>
>
> in the error message.
Okay, I think it's great, let me do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #45177:
URL: https://github.com/apache/spark/pull/45177#discussion_r1497612603
##########
sql/core/src/test/scala/org/apache/spark/sql/BitmapExpressionsQuerySuite.scala:
##########
@@ -217,7 +217,7 @@ class BitmapExpressionsQuerySuite extends QueryTest with SharedSparkSession {
errorClass = "DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE",
parameters = Map(
"sqlExpr" -> "\"bitmap_count(a)\"",
- "paramIndex" -> "0",
+ "paramIndex" -> "1",
Review Comment:
Yea, `two` were `missed` and have been fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #45177: [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE`
URL: https://github.com/apache/spark/pull/45177
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] The `start` value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` should be `1` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1953690124
> > 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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] Use `ordinalNumber` to uniformly set the value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #45177:
URL: https://github.com/apache/spark/pull/45177#discussion_r1497162492
##########
common/utils/src/main/resources/error/README.md:
##########
@@ -117,3 +117,7 @@ The following SQLSTATEs are collated from:
- Oracle 12 (last published)
- SQL Server
- Redshift
+
+## Extra
+
+- Use `ordinalNumber` to uniformly set the value of `paramIndex`(0-based) for the error class `UNEXPECTED_INPUT_TYPE`.
Review Comment:
Okay, let me remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47099][SQL] The `start` value of `paramIndex` for the error class `UNEXPECTED_INPUT_TYPE` should be `1` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #45177:
URL: https://github.com/apache/spark/pull/45177#issuecomment-1953653971
Shall we make the error message more explicit?
```
The <paramIndex>th parameter requires the <requiredType> type, ...
```
or something more accurate in English. cc @srielau
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org