You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by cloud-fan <gi...@git.apache.org> on 2017/03/28 05:59:13 UTC
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/17450
[SPARK-20121][SQL] simplify NullPropagation with NullIntolerant
## What changes were proposed in this pull request?
Instead of iterating all expressions that can return null for null inputs, we can just check `NullIntolerant`.
## How was this patch tested?
existing tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark null
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17450.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 #17450
----
commit dfb70fba832a213f11790a147e505724e1a5b13a
Author: Wenchen Fan <we...@databricks.com>
Date: 2017-03-28T05:32:50Z
simplify NullPropagation with NullIntolerant
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108764164
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -1122,7 +1119,7 @@ case class StringSpace(child: Expression)
""")
// scalastyle:on line.size.limit
case class Substring(str: Expression, pos: Expression, len: Expression)
- extends TernaryExpression with ImplicitCastInputTypes {
+ extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {
--- End diff --
@nsyca If you have a bandwidth, could you please review all the expressions and see whether they can be marked as `NullIntolerant`?
You can check the impl of these expressions and compare them with the corresponding ones in the other RDBMS. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17450
**[Test build #75318 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75318/testReport)** for PR 17450 at commit [`dfb70fb`](https://github.com/apache/spark/commit/dfb70fba832a213f11790a147e505724e1a5b13a).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108756250
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -297,8 +297,8 @@ case class Lower(child: Expression) extends UnaryExpression with String2StringEx
}
/** A base trait for functions that compare two strings, returning a boolean. */
-trait StringPredicate extends Predicate with ImplicitCastInputTypes {
- self: BinaryExpression =>
+abstract class StringPredicate extends BinaryExpression
+ with Predicate with ImplicitCastInputTypes {
--- End diff --
This PR is just to simplify the existing rule `NullPropagation`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108530204
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -1122,7 +1119,7 @@ case class StringSpace(child: Expression)
""")
// scalastyle:on line.size.limit
case class Substring(str: Expression, pos: Expression, len: Expression)
- extends TernaryExpression with ImplicitCastInputTypes {
+ extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {
--- End diff --
> The result can be null; if any argument is null, the result is the null value.
Ref: https://www.ibm.com/support/knowledgecenter/en/SSEPEK_10.0.0/sqlref/src/tpc/db2z_bif_substr.html
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108809796
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -297,8 +297,8 @@ case class Lower(child: Expression) extends UnaryExpression with String2StringEx
}
/** A base trait for functions that compare two strings, returning a boolean. */
-trait StringPredicate extends Predicate with ImplicitCastInputTypes {
- self: BinaryExpression =>
+abstract class StringPredicate extends BinaryExpression
+ with Predicate with ImplicitCastInputTypes {
--- End diff --
See above `StringRegexExpression`, similar to it, in order to simplify the `NullPropagation`, we need to add `NullIntolerant`, so it can propagate null value...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17450
**[Test build #75390 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75390/testReport)** for PR 17450 at commit [`63287ef`](https://github.com/apache/spark/commit/63287ef766b779255054bc463a9e3f9e49149083).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17450
**[Test build #75298 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75298/testReport)** for PR 17450 at commit [`dfb70fb`](https://github.com/apache/spark/commit/dfb70fba832a213f11790a147e505724e1a5b13a).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/75390/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108778378
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -1122,7 +1119,7 @@ case class StringSpace(child: Expression)
""")
// scalastyle:on line.size.limit
case class Substring(str: Expression, pos: Expression, len: Expression)
- extends TernaryExpression with ImplicitCastInputTypes {
+ extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {
--- End diff --
I will certainly take a look. On a second thought, since "most" of the SQL functions are null-intolerant, isn't easier to mark only functions that are null-tolerant such as ISNOTNULL? I am just pitching an idea here, not indicating we should abandon this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/75298/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17450
**[Test build #75392 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75392/testReport)** for PR 17450 at commit [`63287ef`](https://github.com/apache/spark/commit/63287ef766b779255054bc463a9e3f9e49149083).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17450
cc @gatorsmile
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17450
retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17450
**[Test build #75390 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75390/testReport)** for PR 17450 at commit [`63287ef`](https://github.com/apache/spark/commit/63287ef766b779255054bc463a9e3f9e49149083).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108785672
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -1122,7 +1119,7 @@ case class StringSpace(child: Expression)
""")
// scalastyle:on line.size.limit
case class Substring(str: Expression, pos: Expression, len: Expression)
- extends TernaryExpression with ImplicitCastInputTypes {
+ extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {
--- End diff --
At the beginning, when we introduce `NullIntolerant `, @marmbrus said we should do it more carefully. We prefer to using the white-list solution for avoiding hidden bugs. Marking it `NullIntolerant` if and only if we are ensure that they are null intolerant. Thus, when we doing it, we should also add the corresponding test cases and documents.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108837093
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -297,8 +297,8 @@ case class Lower(child: Expression) extends UnaryExpression with String2StringEx
}
/** A base trait for functions that compare two strings, returning a boolean. */
-trait StringPredicate extends Predicate with ImplicitCastInputTypes {
- self: BinaryExpression =>
+abstract class StringPredicate extends BinaryExpression
+ with Predicate with ImplicitCastInputTypes {
--- End diff --
I finally got your point. `StringPredicate` is used for inferring the null constants in the rule `NullPropagation`. Thus, we should mark it as `NullIntolerant `.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108843221
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -297,8 +297,8 @@ case class Lower(child: Expression) extends UnaryExpression with String2StringEx
}
/** A base trait for functions that compare two strings, returning a boolean. */
-trait StringPredicate extends Predicate with ImplicitCastInputTypes {
- self: BinaryExpression =>
+abstract class StringPredicate extends BinaryExpression
+ with Predicate with ImplicitCastInputTypes {
--- End diff --
Yeah. :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17450
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108607884
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -297,8 +297,8 @@ case class Lower(child: Expression) extends UnaryExpression with String2StringEx
}
/** A base trait for functions that compare two strings, returning a boolean. */
-trait StringPredicate extends Predicate with ImplicitCastInputTypes {
- self: BinaryExpression =>
+abstract class StringPredicate extends BinaryExpression
+ with Predicate with ImplicitCastInputTypes {
--- End diff --
Missing `with NullIntolerant` here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108529414
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala ---
@@ -384,33 +379,13 @@ case class NullPropagation(conf: CatalystConf) extends Rule[LogicalPlan] {
Coalesce(newChildren)
}
- case e @ Substring(Literal(null, _), _, _) => Literal.create(null, e.dataType)
- case e @ Substring(_, Literal(null, _), _) => Literal.create(null, e.dataType)
- case e @ Substring(_, _, Literal(null, _)) => Literal.create(null, e.dataType)
-
- // Put exceptional cases above if any
- case e @ BinaryArithmetic(Literal(null, _), _) => Literal.create(null, e.dataType)
- case e @ BinaryArithmetic(_, Literal(null, _)) => Literal.create(null, e.dataType)
-
- case e @ BinaryComparison(Literal(null, _), _) => Literal.create(null, e.dataType)
- case e @ BinaryComparison(_, Literal(null, _)) => Literal.create(null, e.dataType)
-
- case e: StringRegexExpression => e.children match {
- case Literal(null, _) :: right :: Nil => Literal.create(null, e.dataType)
- case left :: Literal(null, _) :: Nil => Literal.create(null, e.dataType)
- case _ => e
- }
-
- case e: StringPredicate => e.children match {
- case Literal(null, _) :: right :: Nil => Literal.create(null, e.dataType)
- case left :: Literal(null, _) :: Nil => Literal.create(null, e.dataType)
- case _ => e
- }
-
// If the value expression is NULL then transform the In expression to
// Literal(null)
case In(Literal(null, _), list) => Literal.create(null, BooleanType)
+ // Put exceptional cases above if any
--- End diff --
Nit: `Attribute` is also `NullIntolerant` Maybe add a comment?
Non-leaf `NullIntolerant ` expressions will return `null`, if at least one of its children is null literal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108519047
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -1122,7 +1119,7 @@ case class StringSpace(child: Expression)
""")
// scalastyle:on line.size.limit
case class Substring(str: Expression, pos: Expression, len: Expression)
- extends TernaryExpression with ImplicitCastInputTypes {
+ extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {
--- End diff --
Is the function SUBSTRING null-intolerant? What is the return value if `str` is a null value?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17450
Great! LGTM except a minor comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108720512
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala ---
@@ -384,33 +379,13 @@ case class NullPropagation(conf: CatalystConf) extends Rule[LogicalPlan] {
Coalesce(newChildren)
}
- case e @ Substring(Literal(null, _), _, _) => Literal.create(null, e.dataType)
- case e @ Substring(_, Literal(null, _), _) => Literal.create(null, e.dataType)
- case e @ Substring(_, _, Literal(null, _)) => Literal.create(null, e.dataType)
-
- // Put exceptional cases above if any
- case e @ BinaryArithmetic(Literal(null, _), _) => Literal.create(null, e.dataType)
- case e @ BinaryArithmetic(_, Literal(null, _)) => Literal.create(null, e.dataType)
-
- case e @ BinaryComparison(Literal(null, _), _) => Literal.create(null, e.dataType)
- case e @ BinaryComparison(_, Literal(null, _)) => Literal.create(null, e.dataType)
-
- case e: StringRegexExpression => e.children match {
- case Literal(null, _) :: right :: Nil => Literal.create(null, e.dataType)
- case left :: Literal(null, _) :: Nil => Literal.create(null, e.dataType)
- case _ => e
- }
-
- case e: StringPredicate => e.children match {
- case Literal(null, _) :: right :: Nil => Literal.create(null, e.dataType)
- case left :: Literal(null, _) :: Nil => Literal.create(null, e.dataType)
- case _ => e
- }
-
// If the value expression is NULL then transform the In expression to
// Literal(null)
case In(Literal(null, _), list) => Literal.create(null, BooleanType)
+ // Put exceptional cases above if any
--- End diff --
I might be confused with the terminologies: `NullIntolerant` expression versus "null-intolerant predicate". But if SUBSTRING is marked null-intolerant expression, why do we not mark the class of string functions such as CONTAINS, STARTSWITH, etc. the same way? Am I missing anything here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/17450
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17450
**[Test build #75392 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75392/testReport)** for PR 17450 at commit [`63287ef`](https://github.com/apache/spark/commit/63287ef766b779255054bc463a9e3f9e49149083).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108720888
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -1122,7 +1119,7 @@ case class StringSpace(child: Expression)
""")
// scalastyle:on line.size.limit
case class Substring(str: Expression, pos: Expression, len: Expression)
- extends TernaryExpression with ImplicitCastInputTypes {
+ extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {
--- End diff --
I might be confused with the terminologies: NullIntolerant expression versus "null-intolerant predicate". But if SUBSTRING is marked null-intolerant expression, why do we not mark the class of string functions such as CONTAINS, STARTSWITH, etc. the same way? Am I missing anything here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17450
retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17450
Thanks! Merging to master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17450
**[Test build #75318 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75318/testReport)** for PR 17450 at commit [`dfb70fb`](https://github.com/apache/spark/commit/dfb70fba832a213f11790a147e505724e1a5b13a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `abstract class BinaryArithmetic extends BinaryOperator with NullIntolerant `
* `case class Add(left: Expression, right: Expression) extends BinaryArithmetic `
* `case class Subtract(left: Expression, right: Expression) extends BinaryArithmetic `
* `case class Multiply(left: Expression, right: Expression) extends BinaryArithmetic `
* `case class Divide(left: Expression, right: Expression) extends BinaryArithmetic `
* `case class Remainder(left: Expression, right: Expression) extends BinaryArithmetic `
* `case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic `
* `case class Like(left: Expression, right: Expression) extends StringRegexExpression `
* `case class RLike(left: Expression, right: Expression) extends StringRegexExpression `
* `case class Contains(left: Expression, right: Expression) extends StringPredicate `
* `case class StartsWith(left: Expression, right: Expression) extends StringPredicate `
* `case class EndsWith(left: Expression, right: Expression) extends StringPredicate `
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/75392/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17450: [SPARK-20121][SQL] simplify NullPropagation with NullInt...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17450
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/75318/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17450: [SPARK-20121][SQL] simplify NullPropagation with ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17450#discussion_r108755645
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -1122,7 +1119,7 @@ case class StringSpace(child: Expression)
""")
// scalastyle:on line.size.limit
case class Substring(str: Expression, pos: Expression, len: Expression)
- extends TernaryExpression with ImplicitCastInputTypes {
+ extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {
--- End diff --
Yes, we should mark `NullIntolerant` to the other expressions and also update the document.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org