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 <gi...@git.apache.org> on 2018/06/20 11:43:39 UTC
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
GitHub user MaxGekk opened a pull request:
https://github.com/apache/spark/pull/21598
[SPARK-24605][SQL] size(null) returns null instead of -1
## What changes were proposed in this pull request?
In PR, I propose new behavior of `size(null)` under the config flag `spark.sql.legacy.sizeOfNull`. If the former one is disabled, the `size()` function returns `null` for `null` input. By default the `spark.sql.legacy.sizeOfNull` is enabled to keep backward compatibility with previous versions. In that case, `size(null)` returns `-1`.
## How was this patch tested?
Modified existing tests for the `size()` function to check new behavior (`null`) and old one (`-1`).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MaxGekk/spark-1 legacy-size-of-null
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21598.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21598
----
commit 2338b787c55d100ac7457e9576054044597acd50
Author: Maxim Gekk <ma...@...>
Date: 2018-06-20T05:59:49Z
New implementation of size returns null for null input
commit e18568f2fc182e41d043c6713d5df7186513fe05
Author: Maxim Gekk <ma...@...>
Date: 2018-06-20T06:00:24Z
Test for legacy and new implementation of size()
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/21598
It’s actually common software engineering practice to keep “buggy”
semantics if a bug has been out there long enough and a lot of applications
depend on the semantics.
On Tue, Jun 26, 2018 at 9:18 AM Hyukjin Kwon <no...@github.com>
wrote:
> I don't think it's too discretionary. We have a safeguard to control the
> behaviour. Spark mentions it in the migration guide. In case of changing
> public interface which breaks binary or source compatibility, there should
> really be strong argument, sure. For clarification, I don't think such
> change is made usually.
>
> In this case, it changes a behaviour even with a safeguard. Sounds pretty
> minor. I wonder why this is suddenly poped up. As I said, if the standards
> don't reflect the practice, the standards should be corrected or the
> practice should be complied. Committer's judgement is needed time to time.
> We need more committers for the more proper review iteration. Let's back it
> roll.
>
> If you prefer more conservative distribution, it should be an option to
> consider using a maintenance release.
>
> we may choose to retain that buggy behavior
>
> I strongly disagree. We should fix the buggy behavior. There's no point of
> having upper versions.
>
> If you strongly doubt it, please open a discussion in the mailing list and
> see if we get agreed at some point.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/spark/pull/21598#issuecomment-400372980>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AATvPGegZOtghAWSAzJFk-PHGWTMthCMks5uAl7VgaJpZM4UvHQD>
> .
>
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r198364343
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1324,6 +1324,12 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
+ .doc("If it is set to true, size of null returns -1. This behavior was inherited from Hive. " +
+ "The size function returns null for null input if the flag is disabled.")
--- End diff --
perhaps you should say this will be updated to false in spark 3.0?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
I am good to have the configuration as that's basically what I suggested too. Removing the behaviour in 3.0.0 is fine. My only question left is the default value.
> We should change the behavior in 3.0. Before 3.0 release, we introduce a conf and make it configurable. The default is to keep the current behavior unchanged.
@gatorsmile, do you think this is specific to this PR and JIRA, or we should do the same things for other changes from now on? If it's latter, it should really be discussed in the dev mailing list in a separate thread.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21598
It seems we don't any behaviour change in current pr (IIUC `spark.sql.legacy.sizeOfNull=true` keeps the current behaviour). If so, we don't need that update?
But, is it okay to set true at this option by default? Based on the three-value logic, `size(null)=null` is more reasonable?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196750347
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
`spark.sql.function.sizeOfNull` is better? btw, If major releases happen in Spark, we can remove these kinds of options for back-compatibility? (just a question)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92354 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92354/testReport)** for PR 21598 at commit [`ae2c7f1`](https://github.com/apache/spark/commit/ae2c7f1e5748cf9e2563bf3d8bdbf01f1f4fd707).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196754013
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
Sounds more consistent to have `spark.sql.function` prefix.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on the issue:
https://github.com/apache/spark/pull/21598
> I think the JIRA and this PR claim the current behaviour is righter?
@HyukjinKwon Yes but changing current behavior can potentially break existing user's applications. I am not sure we can do it till Spark v3.0 . Correct me if I am wrong.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
> Based on my understanding, the decision is made case by case.
I concur.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92142/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196751433
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala ---
@@ -556,21 +559,28 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSQLContext {
checkAnswer(df8.selectExpr("arrays_zip(v1, v2)"), expectedValue8)
}
- test("map size function") {
+ def testSizeOfMap(sizeOfNull: Any): Unit = {
val df = Seq(
(Map[Int, Int](1 -> 1, 2 -> 2), "x"),
(Map[Int, Int](), "y"),
(Map[Int, Int](1 -> 1, 2 -> 2, 3 -> 3), "z"),
(null, "empty")
).toDF("a", "b")
- checkAnswer(
- df.select(size($"a")),
- Seq(Row(2), Row(0), Row(3), Row(-1))
- )
- checkAnswer(
- df.selectExpr("size(a)"),
- Seq(Row(2), Row(0), Row(3), Row(-1))
- )
+
+ checkAnswer(df.select(size($"a")), Seq(Row(2), Row(0), Row(3), Row(sizeOfNull)))
+ checkAnswer(df.selectExpr("size(a)"), Seq(Row(2), Row(0), Row(3), Row(sizeOfNull)))
+ }
+
+ test("map size function - legacy") {
+ withSQLConf("spark.sql.legacy.sizeOfNull" -> "true") {
--- End diff --
`SQLConf. LEGACY_SIZE_OF_NULL.key -> "true"`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92354/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
Shall we update migration guide too?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r197098227
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
Removing is fine. I am good to have such prefix but I wonder what's changed after https://github.com/apache/spark/pull/21427#issuecomment-396142545. Sounds basically similar to what I suggested. Where did that discussion happen?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196824616
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
+ .internal()
+ .doc("If it is set to true, size of null returns -1. This is legacy behavior of Hive. " +
--- End diff --
What do you mean `legacy behavior`? If Hive changes its behavior at certain version, it would be good to describe version number explicitly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92142 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92142/testReport)** for PR 21598 at commit [`64a1b7d`](https://github.com/apache/spark/commit/64a1b7dc759736c72530b6ca5dff3a86a953829a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r197618318
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -75,28 +75,47 @@ trait BinaryArrayExpressionWithImplicitCast extends BinaryExpression
> SELECT _FUNC_(array('b', 'd', 'c', 'a'));
--- End diff --
Update the description of this function?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
We should store the right behaviour. It can be avoided by setting the configuration. Let's set it to true if you believe this is the righter behaviour. That's at the very least what I have been used to in Spark so far.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21598
All the behavior changes need very careful reviews and discussions. Whenever we decide to make a behavior change, we should document it in the migration guide and provide a conf to revert it back to the original behavior. Before the release, the whole community can review the changes again and decide whether any change we should revert or adjust. Based on my understanding, the decision is made case by case. For this specific case, we do not have a very strong reason to change the default value. Thus, we can keep it unchanged.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on the issue:
https://github.com/apache/spark/pull/21598
@HyukjinKwon I am sure the changes are right but we would like to keep current behavior up to release 3.0 in which we will remove the flag and old implementation.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r197326162
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
That's basically the same except that the postfix includes a specific version, which was just a rough idea.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on the issue:
https://github.com/apache/spark/pull/21598
> case by case
Yes, but... this by itself makes the decision appear far too discretionary. Instead, in any PR where you are changing the published interface or behavior of part of Spark's public API, you should be highlighting the change for additional review and providing a really strong argument for why we cannot retain the prior interface and/or default behavior. It is simply not up to an individual committer to decide on their own discretion that the public API should be different than what it, in fact, is. Changing the public API is a big deal -- which is why most additions to the public API should, in my opinion, come in with InterfaceStability annotation that will allow us to change them before a new major-number release.
This doesn't apply to changes to internal APIs. Neither does it apply to bug fixes where Spark isn't actually doing what the public API says it is supposed to do -- although in cases where we expect that users have come to safely rely upon certain buggy behavior, we may choose to retain that buggy behavior under a new configuration setting.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r197227370
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
I've created https://issues.apache.org/jira/browse/SPARK-24625 to track it.
It's similar to https://github.com/apache/spark/pull/21427#issuecomment-396142545 , but as I replied in that PR, having version specific config is an overkill, while `legacy` is simpler and more explicit that it will be removed in the future.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92130/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92128 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92128/testReport)** for PR 21598 at commit [`e18568f`](https://github.com/apache/spark/commit/e18568f2fc182e41d043c6713d5df7186513fe05).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21598
IMHO we need to have clear decision rules for these kinds of behaviour changes in the contribution guide. In the past migration guide descriptions, it seems we've already accept some behaviour changes? e.g., `elt` and `concat`, these apis are of public APIs though.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196993126
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
Do we plan to fix other things accordingly too?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on the issue:
https://github.com/apache/spark/pull/21598
> Sorry, can you elaborate why it's special?
Actually nothing special. I just don't want to break existing user's apps on upgrading Spark's minor releases.
> Who are you referring by "We" BTW
Sorry, I made typo. Definitely I meant only me.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/21598
Do we have other "legacy" configs that we haven't released and can change to match this prefix? It's pretty nice to have a single prefix for stuff like this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21598
thanks, merging to master!
@HyukjinKwon the discussion is specific to this PR. We've changed a bunch of buggy behaviors in this release.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196888409
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
+ .internal()
+ .doc("If it is set to true, size of null returns -1. This is legacy behavior of Hive. " +
--- End diff --
I will change the sentence. It seems it is not clear. I just wanted to say that Spark inherited the behavior from Hive. When the `size()` was implemented, Hive's `size(null)` returns `-1`. Most likely Hive still has the behavior at the moment.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196798555
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -75,28 +75,44 @@ trait BinaryArrayExpressionWithImplicitCast extends BinaryExpression
> SELECT _FUNC_(array('b', 'd', 'c', 'a'));
4
""")
-case class Size(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+case class Size(
+ child: Expression,
+ legacySizeOfNull: Boolean)
+ extends UnaryExpression with ExpectsInputTypes {
+
+ def this(child: Expression) =
+ this(
+ child,
+ legacySizeOfNull = SQLConf.get.getConf(SQLConf.LEGACY_SIZE_OF_NULL))
--- End diff --
since now we can access the conf also on executor side, do we need these changes? Can't we just get this value as a `val`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
@markhamstra, Spark sometimes has some behaviour changes for some bug fixes or in few other cases so far. At least, see the similar configurations added in the migration guide. It sounded we are setting a hard limit here whether it's a bug or not. If the standards don't reflect the practice, it really should be discussed to be correct or complied. It is a matter of "from now on" to me since it sounds a bit different so far if I understood correctly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/21598
Here: https://en.wikipedia.org/wiki/Bug_compatibility
On Tue, Jun 26, 2018 at 9:28 AM Reynold Xin <rx...@databricks.com> wrote:
> It’s actually common software engineering practice to keep “buggy”
> semantics if a bug has been out there long enough and a lot of applications
> depend on the semantics.
>
> On Tue, Jun 26, 2018 at 9:18 AM Hyukjin Kwon <no...@github.com>
> wrote:
>
>> I don't think it's too discretionary. We have a safeguard to control the
>> behaviour. Spark mentions it in the migration guide. In case of changing
>> public interface which breaks binary or source compatibility, there should
>> really be strong argument, sure. For clarification, I don't think such
>> change is made usually.
>>
>> In this case, it changes a behaviour even with a safeguard. Sounds pretty
>> minor. I wonder why this is suddenly poped up. As I said, if the standards
>> don't reflect the practice, the standards should be corrected or the
>> practice should be complied. Committer's judgement is needed time to time.
>> We need more committers for the more proper review iteration. Let's back it
>> roll.
>>
>> If you prefer more conservative distribution, it should be an option to
>> consider using a maintenance release.
>>
>> we may choose to retain that buggy behavior
>>
>> I strongly disagree. We should fix the buggy behavior. There's no point
>> of having upper versions.
>>
>> If you strongly doubt it, please open a discussion in the mailing list
>> and see if we get agreed at some point.
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <https://github.com/apache/spark/pull/21598#issuecomment-400372980>, or mute
>> the thread
>> <https://github.com/notifications/unsubscribe-auth/AATvPGegZOtghAWSAzJFk-PHGWTMthCMks5uAl7VgaJpZM4UvHQD>
>> .
>>
>
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92128/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/21598
This is not a "bug" and there is no "right" behavior in APIs. It's been defined as -1 since the very beginning (when was it added?), so we can't just change the default value in a feature release.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92130 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92130/testReport)** for PR 21598 at commit [`b6539a5`](https://github.com/apache/spark/commit/b6539a5a8fe45c81cac506caba8c438269dad881).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21598
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
I don't think it's too discretionary. We have a safeguard to control the behaviour. Spark mentions it in the migration guide. In case of changing public interface which breaks binary or source compatibility, there should really be strong argument, sure. For clarification, I don't think such change is made usually.
In this case, it changes a behaviour even with a safeguard. Sounds pretty minor. I wonder why this is suddenly poped up. As I said, if the standards don't reflect the practice, the standards should be corrected or the practice should be complied. Committer's judgement is needed time to time. We need more committers for the more proper review iteration. Let's back it roll.
If you prefer more conservative distribution, it should be an option to consider using a maintenance release.
> we may choose to retain that buggy behavior
I strongly disagree. We should fix the buggy behavior. There's no point of having upper versions.
If you strongly doubt it, please open a discussion in the mailing list and see if we get agreed at some point.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21598
@rxin yes we have, I think they are all listed in the [2.4 migration guide](https://github.com/apache/spark/blob/master/docs/sql-programming-guide.md#upgrading-from-spark-sql-23-to-24)
I've created https://issues.apache.org/jira/browse/SPARK-24625 to track it
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r197618603
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -75,28 +75,47 @@ trait BinaryArrayExpressionWithImplicitCast extends BinaryExpression
> SELECT _FUNC_(array('b', 'd', 'c', 'a'));
4
""")
-case class Size(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+case class Size(
+ child: Expression,
+ legacySizeOfNull: Boolean)
+ extends UnaryExpression with ExpectsInputTypes {
+
+ def this(child: Expression) =
+ this(
+ child,
+ legacySizeOfNull = SQLConf.get.getConf(SQLConf.LEGACY_SIZE_OF_NULL))
+
override def dataType: DataType = IntegerType
override def inputTypes: Seq[AbstractDataType] = Seq(TypeCollection(ArrayType, MapType))
- override def nullable: Boolean = false
+ override def nullable: Boolean = if (legacySizeOfNull) false else super.nullable
override def eval(input: InternalRow): Any = {
val value = child.eval(input)
if (value == null) {
- -1
+ if (legacySizeOfNull) -1 else null
} else child.dataType match {
case _: ArrayType => value.asInstanceOf[ArrayData].numElements()
case _: MapType => value.asInstanceOf[MapData].numElements()
+ case other => throw new UnsupportedOperationException(
+ s"The size function doesn't support the operand type ${other.getClass.getCanonicalName}")
}
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
- val childGen = child.genCode(ctx)
- ev.copy(code = code"""
+ if (legacySizeOfNull) {
+ val childGen = child.genCode(ctx)
+ ev.copy(code = code"""
boolean ${ev.isNull} = false;
${childGen.code}
${CodeGenerator.javaType(dataType)} ${ev.value} = ${childGen.isNull} ? -1 :
(${childGen.value}).numElements();""", isNull = FalseLiteral)
+ } else {
+ child.dataType match {
+ case _: ArrayType | _: MapType => defineCodeGen(ctx, ev, c => s"($c).numElements()")
+ case other => throw new UnsupportedOperationException(
--- End diff --
Could you reorganize the code? The current flow looks confusing. It appears like that the other types are not supported when `legacySizeOfNull` is false. However, the input types are already limited to ArrayType and MapType.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92130 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92130/testReport)** for PR 21598 at commit [`b6539a5`](https://github.com/apache/spark/commit/b6539a5a8fe45c81cac506caba8c438269dad881).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r197082278
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
Yes, we would like to change/improve external behavior under flags in the `spark.sql.legacy.*` namespace. All those flags should be removed in the next major release 3.0. Please, share your thoughts about it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196891392
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
--- End diff --
We would like to reserve the namespace `spark.sql.legacy.*` for all legacy configurations. /cc @marmbrus @rxin
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21598
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
I don't object about the default value here if it's specific to this JIRA and PR for clarification as I said above. If this says about general stuff, to me it sounds it needs a separate discussion.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
My assumption was that the PR and JIRA claim that it's the right behaviour, as I said multiple times. If there's no such thing, there should be of course no need to argue about the default value, as I said above.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r198280518
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -75,28 +75,44 @@ trait BinaryArrayExpressionWithImplicitCast extends BinaryExpression
> SELECT _FUNC_(array('b', 'd', 'c', 'a'));
4
""")
-case class Size(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+case class Size(
+ child: Expression,
+ legacySizeOfNull: Boolean)
+ extends UnaryExpression with ExpectsInputTypes {
+
+ def this(child: Expression) =
+ this(
+ child,
+ legacySizeOfNull = SQLConf.get.getConf(SQLConf.LEGACY_SIZE_OF_NULL))
--- End diff --
If it works now, I will try to read the config on executor's side. I was just struggling to an issue in tests for another PR when SQL configs were not propagated to executors. For example: https://github.com/apache/spark/blob/a40ffc656d62372da85e0fa932b67207839e7fde/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L52-L54
Also I see some other places where configs are read via passing to constructors: https://github.com/apache/spark/blob/b8f27ae3b34134a01998b77db4b7935e7f82a4fe/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala#L534
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
@rxin, I understand the concerns about having behaviour changes. I discussed about this with other committers and community a lot.
So, do you basically imply that we should support to keep the buggy behaviours, leave them by default and have configurations to enable them? I was thinking this is a bit different with what I am used to and it needs a discussion and agreement from the community.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on the issue:
https://github.com/apache/spark/pull/21598
> so we can't just change the default value in a feature release
Agreed. Once a particular interface and behavior is in our released public API, then we effectively have a contract not to change that behavior. If we are going to provide another behavior before making a new major-number release (e.g. spark-3.0.0), then we have to provide a user configuration option to select that new behavior, and the default behavior if a user doesn't change configuration must be the same as before the optional new behavior.
If there is a clear, severe bug (such as data loss or corruption), only then we can consider changing the public API before making a new major-number release -- but even then we are likely to either go immediately to a new major-number or to at least preserve the old, buggy behavior with a configuration option.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
Sorry, can you elaborate why it's special? Who's referring "We" BTW. Where did the discussion happen?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196751561
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -75,28 +75,44 @@ trait BinaryArrayExpressionWithImplicitCast extends BinaryExpression
> SELECT _FUNC_(array('b', 'd', 'c', 'a'));
4
""")
-case class Size(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+case class Size(
+ child: Expression,
+ legacySizeOfNull: Boolean)
+ extends UnaryExpression with ExpectsInputTypes {
+
+ def this(child: Expression) =
+ this(
+ child,
+ legacySizeOfNull = SQLConf.get.getConf(SQLConf.LEGACY_SIZE_OF_NULL))
+
override def dataType: DataType = IntegerType
override def inputTypes: Seq[AbstractDataType] = Seq(TypeCollection(ArrayType, MapType))
- override def nullable: Boolean = false
+ override def nullable: Boolean = if (legacySizeOfNull) false else super.nullable
override def eval(input: InternalRow): Any = {
val value = child.eval(input)
if (value == null) {
- -1
+ if (legacySizeOfNull) -1 else null
} else child.dataType match {
case _: ArrayType => value.asInstanceOf[ArrayData].numElements()
case _: MapType => value.asInstanceOf[MapData].numElements()
}
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
- val childGen = child.genCode(ctx)
- ev.copy(code = code"""
+ if (legacySizeOfNull) {
+ val childGen = child.genCode(ctx)
+ ev.copy(code = code"""
boolean ${ev.isNull} = false;
${childGen.code}
${CodeGenerator.javaType(dataType)} ${ev.value} = ${childGen.isNull} ? -1 :
(${childGen.value}).numElements();""", isNull = FalseLiteral)
+ } else {
+ child.dataType match {
+ case _: ArrayType => defineCodeGen(ctx, ev, c => s"($c).numElements()")
+ case _: MapType => defineCodeGen(ctx, ev, c => s"($c).numElements()")
--- End diff --
Can we fold both cases?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r196750981
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1314,6 +1314,13 @@ object SQLConf {
"Other column values can be ignored during parsing even if they are malformed.")
.booleanConf
.createWithDefault(true)
+
+ val LEGACY_SIZE_OF_NULL = buildConf("spark.sql.legacy.sizeOfNull")
+ .internal()
--- End diff --
`internal`? Since this is an user-facing option, it is not `internal`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21598
I created a JIRA https://issues.apache.org/jira/browse/SPARK-24640. We should change the behavior in 3.0. Before 3.0 release, we introduce a conf and make it configurable. The default is to keep the current behavior unchanged.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21598
@cloud-fan This merge breaks the build? https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92364/consoleFull
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
We will add a configuration for it as a safe safeguard. I think we should incrementally fix things to be the righter behaviour in upper versions .. really. If you meant in the JIRA and PR, this behaivour should be considered as an option, then probably it makes sense to leave it `false` by default.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21598
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21598
re: https://github.com/apache/spark/pull/21598#issuecomment-398737499 I missed the default value. Shall we set it to `false`? I think the JIRA and this PR claim the current behaviour is righter?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92128 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92128/testReport)** for PR 21598 at commit [`e18568f`](https://github.com/apache/spark/commit/e18568f2fc182e41d043c6713d5df7186513fe05).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21598: [SPARK-24605][SQL] size(null) returns null instea...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21598#discussion_r198562854
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -75,28 +75,44 @@ trait BinaryArrayExpressionWithImplicitCast extends BinaryExpression
> SELECT _FUNC_(array('b', 'd', 'c', 'a'));
4
""")
-case class Size(child: Expression) extends UnaryExpression with ExpectsInputTypes {
+case class Size(
+ child: Expression,
+ legacySizeOfNull: Boolean)
+ extends UnaryExpression with ExpectsInputTypes {
+
+ def this(child: Expression) =
+ this(
+ child,
+ legacySizeOfNull = SQLConf.get.getConf(SQLConf.LEGACY_SIZE_OF_NULL))
--- End diff --
it was made possible in https://github.com/apache/spark/pull/21376
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92142 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92142/testReport)** for PR 21598 at commit [`64a1b7d`](https://github.com/apache/spark/commit/64a1b7dc759736c72530b6ca5dff3a86a953829a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21598
LGTM and WFT
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21598
**[Test build #92354 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92354/testReport)** for PR 21598 at commit [`ae2c7f1`](https://github.com/apache/spark/commit/ae2c7f1e5748cf9e2563bf3d8bdbf01f1f4fd707).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21598: [SPARK-24605][SQL] size(null) returns null instead of -1
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on the issue:
https://github.com/apache/spark/pull/21598
@HyukjinKwon this is not new policy. It is what Apache Spark has guaranteed in its version numbering and public API since 1.0.0. It is not a matter of "from now on", but rather of whether committers have started allowing our standards to slip. It may well be time for a discussion of that and of better tools to help guarantee that additions and changes to the public API are adequately discussed and reviewed, appropriate InterfaceStability annotations are applied, etc.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org