[SPARK-43011][SQL] array_insert should fail with 0 index
#40641
Conversation
| "message" : [ | ||
| "The index 0 is invalid. An index shall be either < 0 or > 0 (the first element has index 1)." | ||
| ], | ||
| "sqlState" : "22003" |
There was a problem hiding this comment.
I just reuse the sqlState of ELEMENT_AT_BY_INDEX_ZERO, is it fine? @itholic
There was a problem hiding this 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 ?
There was a problem hiding this comment.
how about INVALID_INDEX_OF_ZERO?
There was a problem hiding this comment.
Sounds good. We need to start hardening the error classes, though. They are documented API.
| @@ -536,7 +536,7 @@ | |||
| ], | |||
| "sqlState" : "23505" | |||
| }, | |||
| "ELEMENT_AT_BY_INDEX_ZERO" : { | |||
| "INVALID_INDEX_OF_ZERO" : { | |||
There was a problem hiding this comment.
Highly likely, it should be moved since error classes must be sorted in the file.
There was a problem hiding this comment.
got it, will fix it soon
docs/sql-error-conditions.md
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Could you move it according to alphabetical sorting, please.
rename as INVALID_INDEX_OF_ZERO
zhengruifeng
force-pushed
the
sql_array_insert_fails_zero
branch
from
9617564
to
b1d77d1
Compare
|
@cloud-fan @MaxGekk would you mind taking another look? |
|
+1, LGTM. Merging to master/3.4. |
### What changes were proposed in this pull request? Make `array_insert` fail when input index `pos` is zero. ### Why are the changes needed? see #40563 (comment) ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? updated UT Closes #40641 from zhengruifeng/sql_array_insert_fails_zero. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 3e9574c) Signed-off-by: Max Gekk <max.gekk@gmail.com>
### What changes were proposed in this pull request? Make `array_insert` fail when input index `pos` is zero. ### Why are the changes needed? see apache#40563 (comment) ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? updated UT Closes apache#40641 from zhengruifeng/sql_array_insert_fails_zero. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 3e9574c) Signed-off-by: Max Gekk <max.gekk@gmail.com>
What changes were proposed in this pull request?
Make
array_insertfail when input indexposis zero.Why are the changes needed?
see #40563 (comment)
Does this PR introduce any user-facing change?
Yes
How was this patch tested?
updated UT