You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/08/09 13:58:33 UTC
[GitHub] [spark] AngersZhuuuu opened a new pull request, #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
AngersZhuuuu opened a new pull request, #37453:
URL: https://github.com/apache/spark/pull/37453
### What changes were proposed in this pull request?
ArrayType's parameter `containsNull` means this array can contains null, related to nullable, this is easy to misunderstand in reading logic. In this pr, we refactor the comment about `containsNull` and add a method in ArrayType to replace the usage of `containsNull`
```
def nullable: Boolean = containsNull
```
### Why are the changes needed?
Refactor code
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Not need
--
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] AngersZhuuuu commented on pull request #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1210121769
> I am -1 on this change. it's confusing if this indicates `array(null)` or `null`. In addition, this is related to the public API, e.g., `ArrayType`.
>
> In addition, there's `MapType.valueContainsNull` too.
How about the current? I know MapType have the same problem and I will raise a pr for MapType too
--
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] AngersZhuuuu commented on pull request #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1210071823
> I am -1 on this change. it's confusing if this indicates `array(null)` or `null`. In addition, this is related to the public API, e.g., `ArrayType`.
>
> In addition, there's `MapType.valueContainsNull` too.
Have discussed with wenchen, change this later
--
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 pull request #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1211493857
can you update the PR title?
--
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] AngersZhuuuu commented on pull request #37453: [SPARK-40019][SQL] Refactor comment of ArrayType's containsNull and refactor the misunderstanding logics in collectionOperator's expression about `containsNull`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1211527525
> can you update the PR title?
How about current?
--
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] AngersZhuuuu commented on pull request #37453: [SPARK-40019][SQL] Refactor comment of ArrayType's containsNull and refactor the misunderstanding logics in collectionOperator's expression about `containsNull`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1214131207
ping @cloud-fan
--
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 pull request #37453: [SPARK-40019][SQL] Refactor comment of ArrayType's containsNull and refactor the misunderstanding logics in collectionOperator's expression about `containsNull`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1215016298
thanks, merging to master!
--
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] HyukjinKwon commented on pull request #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1210224859
Yeah, I am fine with the current approach.
--
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 #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37453:
URL: https://github.com/apache/spark/pull/37453#discussion_r943054136
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/types/ArrayType.scala:
##########
@@ -53,11 +53,12 @@ object ArrayType extends AbstractDataType {
* Please use `DataTypes.createArrayType()` to create a specific instance.
*
* An [[ArrayType]] object comprises two fields, `elementType: [[DataType]]` and
- * `containsNull: Boolean`. The field of `elementType` is used to specify the type of
- * array elements. The field of `containsNull` is used to specify if the array has `null` values.
+ * `containsNull: Boolean`.
+ * The field of `elementType` is used to specify the type of array elements.
+ * The field of `containsNull` is used to specify if the array can have `null` values.
*
* @param elementType The data type of values.
- * @param containsNull Indicates if values have `null` values
+ * @param containsNull Indicates if values can have `null` values
Review Comment:
```suggestion
* @param containsNull Indicates if the array can have `null` values
```
--
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 #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37453:
URL: https://github.com/apache/spark/pull/37453#discussion_r943053914
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -1079,6 +1084,9 @@ case class Shuffle(child: Expression, randomSeed: Option[Long] = None)
override def dataType: DataType = child.dataType
+ @transient private lazy val resultArrayElementNullable =
Review Comment:
```suggestion
private def resultArrayElementNullable =
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -1162,6 +1170,9 @@ case class Reverse(child: Expression)
override def dataType: DataType = child.dataType
+ @transient private lazy val resultArrayElementNullable =
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
[GitHub] [spark] AngersZhuuuu commented on pull request #37453: [SPARK-40019][SQL] Refactor comment and logic of ArrayType's containsNull
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #37453:
URL: https://github.com/apache/spark/pull/37453#issuecomment-1211489643
> Yeah, I am fine with the current approach.
Any more suggestions? also ping @cloud-fan
--
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 closed pull request #37453: [SPARK-40019][SQL] Refactor comment of ArrayType's containsNull and refactor the misunderstanding logics in collectionOperator's expression about `containsNull`
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #37453: [SPARK-40019][SQL] Refactor comment of ArrayType's containsNull and refactor the misunderstanding logics in collectionOperator's expression about `containsNull`
URL: https://github.com/apache/spark/pull/37453
--
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