You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2023/08/22 18:09:48 UTC
[GitHub] [spark] MaxGekk opened a new pull request, #42616: [SPARK-44840][SQL][3.5] Make `array_insert()` 1-based for negative indexes
MaxGekk opened a new pull request, #42616:
URL: https://github.com/apache/spark/pull/42616
### What changes were proposed in this pull request?
In the PR, I propose to make the `array_insert` function 1-based for negative indexes. So, the maximum index is -1 should point out to the last element, and the function should insert new element at the end of the given array for the index -1.
The old behaviour can be restored via the SQL config `spark.sql.legacy.negativeIndexInArrayInsert`.
This is a backport of https://github.com/apache/spark/pull/42564
### Why are the changes needed?
1. To match the behaviour of functions such as `substr()` and `element_at()`.
```sql
spark-sql (default)> select element_at(array('a', 'b'), -1), substr('ab', -1);
b b
```
2. To fix an inconsistency in `array_insert` in which positive indexes are 1-based, but negative indexes are 0-based.
### Does this PR introduce _any_ user-facing change?
Yes.
Before:
```sql
spark-sql (default)> select array_insert(array('a', 'b'), -1, 'c');
["a","c","b"]
```
After:
```sql
spark-sql (default)> select array_insert(array('a', 'b'), -1, 'c');
["a","b","c"]
```
### How was this patch tested?
By running the modified test suite:
```
$ build/sbt "test:testOnly *CollectionExpressionsSuite"
$ build/sbt "test:testOnly *DataFrameFunctionsSuite"
$ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite"
```
--
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 #42616: [SPARK-44840][SQL][3.5] Make `array_insert()` 1-based for negative indexes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #42616:
URL: https://github.com/apache/spark/pull/42616#discussion_r1302157936
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4674,23 +4674,34 @@ case class ArrayExcept(left: Expression, right: Expression) extends ArrayBinaryL
@ExpressionDescription(
usage = """
_FUNC_(x, pos, val) - Places val into index pos of array x.
- Array indices start at 1, or start from the end if index is negative.
+ Array indices start at 1. The maximum negative index is -1 for which the function inserts
Review Comment:
> Is it mathematically to indicate -1 is the biggest among all negative values for pos parameter?
Correct.
--
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] dongjoon-hyun commented on a diff in pull request #42616: [SPARK-44840][SQL][3.5] Make `array_insert()` 1-based for negative indexes
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #42616:
URL: https://github.com/apache/spark/pull/42616#discussion_r1302120937
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4674,23 +4674,34 @@ case class ArrayExcept(left: Expression, right: Expression) extends ArrayBinaryL
@ExpressionDescription(
usage = """
_FUNC_(x, pos, val) - Places val into index pos of array x.
- Array indices start at 1, or start from the end if index is negative.
+ Array indices start at 1. The maximum negative index is -1 for which the function inserts
Review Comment:
What is the meaning of `maximum negative index` here? Is it mathematically to indicate `-1` is the biggest among all negative values for `pos` parameter?
--
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] dongjoon-hyun commented on pull request #42616: [SPARK-44840][SQL][3.5] Make `array_insert()` 1-based for negative indexes
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #42616:
URL: https://github.com/apache/spark/pull/42616#issuecomment-1689272709
Merged to branch-3.5. Thank you, @MaxGekk and all.
--
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] dongjoon-hyun commented on a diff in pull request #42616: [SPARK-44840][SQL][3.5] Make `array_insert()` 1-based for negative indexes
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #42616:
URL: https://github.com/apache/spark/pull/42616#discussion_r1302172275
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -4674,23 +4674,34 @@ case class ArrayExcept(left: Expression, right: Expression) extends ArrayBinaryL
@ExpressionDescription(
usage = """
_FUNC_(x, pos, val) - Places val into index pos of array x.
- Array indices start at 1, or start from the end if index is negative.
+ Array indices start at 1. The maximum negative index is -1 for which the function inserts
Review Comment:
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] dongjoon-hyun closed pull request #42616: [SPARK-44840][SQL][3.5] Make `array_insert()` 1-based for negative indexes
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #42616: [SPARK-44840][SQL][3.5] Make `array_insert()` 1-based for negative indexes
URL: https://github.com/apache/spark/pull/42616
--
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