You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhengruifeng (via GitHub)" <gi...@apache.org> on 2023/04/03 10:57:16 UTC
[GitHub] [spark] zhengruifeng opened a new pull request, #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
zhengruifeng opened a new pull request, #40641:
URL: https://github.com/apache/spark/pull/40641
### What changes were proposed in this pull request?
make `array_insert` fail when input pos==0
### Why are the changes needed?
see https://github.com/apache/spark/pull/40563#discussion_r1155673089
### Does this PR introduce _any_ user-facing change?
yes
### How was this patch tested?
updated UT
--
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
[GitHub] [spark] zhengruifeng commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1155807804
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -542,6 +542,12 @@
],
"sqlState" : "22003"
},
+ "ARRAY_INSERT_BY_INDEX_ZERO" : {
+ "message" : [
+ "The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
+ ],
+ "sqlState" : "22003"
Review Comment:
I just reuse the `sqlState` of `ELEMENT_AT_BY_INDEX_ZERO`, is it fine? @itholic
--
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
[GitHub] [spark] itholic commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1155813814
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -542,6 +542,12 @@
],
"sqlState" : "22003"
},
+ "ARRAY_INSERT_BY_INDEX_ZERO" : {
+ "message" : [
+ "The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
+ ],
+ "sqlState" : "22003"
Review Comment:
Yes, if a particular SQL state is appropriate for representing a specific error, we can use the same SQL state in multiple error classes.
But on my second thought, maybe can we consolidate `ELEMENT_AT_BY_INDEX_ZERO` and `ARRAY_INSERT_BY_INDEX_ZERO` into single error class if they have exactly the same error message??
For example:
```
"ZERO_INDEX_ERROR" : {
"message" : [
"The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
],
"sqlState" : "22003"
},
```
Although I'm not 100% sure if the name `ZERO_INDEX_ERROR` is proper to cover the both cases. WDYT @srielau @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
[GitHub] [spark] MaxGekk closed pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
URL: https://github.com/apache/spark/pull/40641
--
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
[GitHub] [spark] itholic commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1155813814
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -542,6 +542,12 @@
],
"sqlState" : "22003"
},
+ "ARRAY_INSERT_BY_INDEX_ZERO" : {
+ "message" : [
+ "The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
+ ],
+ "sqlState" : "22003"
Review Comment:
Yes, if a particular SQL state is appropriate for representing a specific error, we can use the same SQL state in multiple error classes.
But on my second thought, maybe can we consolidate `ELEMENT_AT_BY_INDEX_ZERO` and `ARRAY_INSERT_BY_INDEX_ZERO` into single error class if they have exactly the same error message??
For example:
```
"ZERO_INDEX_ERROR" : {
"message" : [
"The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
],
"sqlState" : "22003"
},
```
I'm not 100% sure if `ZERO_INDEX_ERROR` is proper to cover the both cases. WDYT @srielau @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
[GitHub] [spark] zhengruifeng commented on pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #40641:
URL: https://github.com/apache/spark/pull/40641#issuecomment-1494110470
cc @cloud-fan @itholic
--
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
[GitHub] [spark] MaxGekk commented on pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #40641:
URL: https://github.com/apache/spark/pull/40641#issuecomment-1495471186
+1, LGTM. Merging to master/3.4.
Thank you, @zhengruifeng and @cloud-fan @itholic 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
[GitHub] [spark] zhengruifeng commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1156557034
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -536,7 +536,7 @@
],
"sqlState" : "23505"
},
- "ELEMENT_AT_BY_INDEX_ZERO" : {
+ "INVALID_INDEX_OF_ZERO" : {
Review Comment:
got it, will fix it soon
##########
docs/sql-error-conditions.md:
##########
@@ -289,7 +289,7 @@ Duplicate map key `<key>` was found, please check the input data. If you want to
Found duplicate keys `<keyColumn>`.
-### ELEMENT_AT_BY_INDEX_ZERO
+### INVALID_INDEX_OF_ZERO
Review Comment:
got it, thanks
--
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
[GitHub] [spark] zhengruifeng commented on pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #40641:
URL: https://github.com/apache/spark/pull/40641#issuecomment-1495243707
@cloud-fan @MaxGekk would you mind taking another look?
--
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
[GitHub] [spark] itholic commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1155813814
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -542,6 +542,12 @@
],
"sqlState" : "22003"
},
+ "ARRAY_INSERT_BY_INDEX_ZERO" : {
+ "message" : [
+ "The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
+ ],
+ "sqlState" : "22003"
Review Comment:
Yes, it sounds good to me.
But on my second thought, maybe can we consolidate `ELEMENT_AT_BY_INDEX_ZERO` and `ARRAY_INSERT_BY_INDEX_ZERO` into single error class if they have exactly the same error message??
For example:
```
"ZERO_INDEX_ERROR" : {
"message" : [
"The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
],
"sqlState" : "22003"
},
```
I'm not 100% sure if `ZERO_INDEX_ERROR` is proper to cover the both cases. WDYT @srielau @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
[GitHub] [spark] srielau commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1157492500
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -542,6 +542,12 @@
],
"sqlState" : "22003"
},
+ "ARRAY_INSERT_BY_INDEX_ZERO" : {
+ "message" : [
+ "The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
+ ],
+ "sqlState" : "22003"
Review Comment:
Sounds good. We need to start hardening the error classes, though. They are documented API.
--
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1156308161
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -536,7 +536,7 @@
],
"sqlState" : "23505"
},
- "ELEMENT_AT_BY_INDEX_ZERO" : {
+ "INVALID_INDEX_OF_ZERO" : {
Review Comment:
Highly likely, it should be moved since error classes must be sorted in the file.
##########
docs/sql-error-conditions.md:
##########
@@ -289,7 +289,7 @@ Duplicate map key `<key>` was found, please check the input data. If you want to
Found duplicate keys `<keyColumn>`.
-### ELEMENT_AT_BY_INDEX_ZERO
+### INVALID_INDEX_OF_ZERO
Review Comment:
Could you move it according to alphabetical sorting, please.
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #40641: [SPARK-43011][SQL] `array_insert` should fail with 0 index
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40641:
URL: https://github.com/apache/spark/pull/40641#discussion_r1155846553
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -542,6 +542,12 @@
],
"sqlState" : "22003"
},
+ "ARRAY_INSERT_BY_INDEX_ZERO" : {
+ "message" : [
+ "The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)."
+ ],
+ "sqlState" : "22003"
Review Comment:
how about `INVALID_INDEX_OF_ZERO`?
--
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