You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by bersprockets <gi...@git.apache.org> on 2018/04/24 19:30:29 UTC
[GitHub] spark pull request #21144: [SPARK-24043][SQL] Interpreted Predicate should i...
GitHub user bersprockets opened a pull request:
https://github.com/apache/spark/pull/21144
[SPARK-24043][SQL] Interpreted Predicate should initialize nondeterministic expressions
## What changes were proposed in this pull request?
When creating an InterpretedPredicate instance, initialize any Nondeterministic expressions in the expression tree to avoid java.lang.IllegalArgumentException on later call to eval().
## How was this patch tested?
- sbt SQL tests
- python SQL tests
- new unit test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bersprockets/spark interpretedpredicate
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21144.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 #21144
----
commit 43791acd9458c539c7008d1b6aa670f3b9965758
Author: Bruce Robbins <be...@...>
Date: 2018-04-21T23:37:32Z
Initial commit
commit 525bc1a475c5007830eb2e35c30f05bafa600cdd
Author: Bruce Robbins <be...@...>
Date: 2018-04-23T02:12:10Z
Use expression's foreach
commit 46972f7ce2123d7ebda23daad9362650532ca59e
Author: Bruce Robbins <be...@...>
Date: 2018-04-24T14:35:50Z
Call base class's initialize as well
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21144: [SPARK-24043][SQL] Interpreted Predicate should i...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on a diff in the pull request:
https://github.com/apache/spark/pull/21144#discussion_r183853692
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala ---
@@ -36,6 +36,14 @@ object InterpretedPredicate {
case class InterpretedPredicate(expression: Expression) extends BasePredicate {
override def eval(r: InternalRow): Boolean = expression.eval(r).asInstanceOf[Boolean]
+
+ override def initialize(partitionIndex: Int): Unit = {
+ super.initialize(partitionIndex)
+ expression.foreach {
+ case n: Nondeterministic => n.initialize(partitionIndex)
+ case _ =>
+ }
+ }
}
--- End diff --
An alternative to this change: we could simply expect a caller to InterpretedPredicate.create, like SparkPlan.genInterpretedPredicate, to pre-initialized the expression before passing it to create.
This alternative is a little harder to unit test: we would also need to add a flag to shut off predicate codegen so that we can force SparkPlan.newPredicate to use InterpretedPredicate.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21144: [SPARK-24043][SQL] Interpreted Predicate should i...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21144#discussion_r183908521
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala ---
@@ -36,6 +36,14 @@ object InterpretedPredicate {
case class InterpretedPredicate(expression: Expression) extends BasePredicate {
override def eval(r: InternalRow): Boolean = expression.eval(r).asInstanceOf[Boolean]
+
+ override def initialize(partitionIndex: Int): Unit = {
+ super.initialize(partitionIndex)
+ expression.foreach {
--- End diff --
Is it ok to use `foreachUp` instead? The initialization always does not depend on their children?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21144: [SPARK-24043][SQL] Interpreted Predicate should i...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/21144#discussion_r183890298
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala ---
@@ -36,6 +36,14 @@ object InterpretedPredicate {
case class InterpretedPredicate(expression: Expression) extends BasePredicate {
override def eval(r: InternalRow): Boolean = expression.eval(r).asInstanceOf[Boolean]
+
+ override def initialize(partitionIndex: Int): Unit = {
+ super.initialize(partitionIndex)
+ expression.foreach {
+ case n: Nondeterministic => n.initialize(partitionIndex)
+ case _ =>
+ }
+ }
}
--- End diff --
Nah this is good like it is.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/21144
@cloud-fan I don't think this is an issue in 2.3. It would be an issue only once [SPARK-23580](https://issues.apache.org/jira/browse/SPARK-23580) ("Interpreted mode fallback should be implemented") is completed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21144
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 #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21144
shall we backport it to 2.3?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21144
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89805/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/21144
>can you also check if other interpreted code paths have this problem?
@hvanhovell will do.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/21144
LGTM - merging to master. Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21144
**[Test build #89807 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89807/testReport)** for PR 21144 at commit [`46972f7`](https://github.com/apache/spark/commit/46972f7ce2123d7ebda23daad9362650532ca59e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21144
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89807/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21144
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 #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21144
**[Test build #89807 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89807/testReport)** for PR 21144 at commit [`46972f7`](https://github.com/apache/spark/commit/46972f7ce2123d7ebda23daad9362650532ca59e).
* 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 issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21144
**[Test build #89805 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89805/testReport)** for PR 21144 at commit [`46972f7`](https://github.com/apache/spark/commit/46972f7ce2123d7ebda23daad9362650532ca59e).
* This patch **fails Spark unit 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 issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21144
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 #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/21144
@bersprockets can you also check if other interpreted code paths have this problem?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21144
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89798/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21144
@bersprockets I think we assume `Predicate.initialize` is called in not a driver side but executor sides, so it's ok as it is (`SimpleTextSource. buildReader` and `ExternalCatalogUtils.prunePartitionsByFilter`).
btw, as you know, the issue `UnsupportedOperationExceptions ` should be fixed in SPARK-23580.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/21144
@hvanhovell I couldn't find another path that had this particular issue, except possibly in the aggregation area (where a grouping projection is created but not initialized). But I couldn't make it fail there.
Also, I turned off as much of the code generation as I could (through patching, etc,) and ran the SQL unit tests. I didn't get any 'requirement failed' exceptions. However, I did get 1270 failed tests,
90% of which failed with UnsupportedOperationExceptions (SortPrefix.eval is the one I keep seeing). Fixing those issues may allow more 'requirement failed' exceptions to pop out.
The original unit test that got me looking at this ("SPARK-10740: handle nondeterministic expressions correctly for set operations") is still failing, but for a different reason: The rows in the table are processed in a different order between interpreted and codegen mode. rand(7) is used as a filter, and although the sequence of random numbers is the same, the row to which each random number is being compared is different between interpreted and codegen mode.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21144
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 pull request #21144: [SPARK-24043][SQL] Interpreted Predicate should i...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on a diff in the pull request:
https://github.com/apache/spark/pull/21144#discussion_r183937258
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala ---
@@ -36,6 +36,14 @@ object InterpretedPredicate {
case class InterpretedPredicate(expression: Expression) extends BasePredicate {
override def eval(r: InternalRow): Boolean = expression.eval(r).asInstanceOf[Boolean]
+
+ override def initialize(partitionIndex: Int): Unit = {
+ super.initialize(partitionIndex)
+ expression.foreach {
--- End diff --
@maropu At the moment, all of the initialization methods are very simple and don't rely on children. That could change, but that's the current state.
InterpretedProjection, InterpretedUnsafeProjection, InterpretedMutableProjection and ExpressionEvalHelper currently initialize the Nondeterministic expressions with a foreach. If we want to change it, we should change it in those other places too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/21144
@hvanhovell @maropu Is there anything on this PR that I should do?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21144
**[Test build #89798 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89798/testReport)** for PR 21144 at commit [`46972f7`](https://github.com/apache/spark/commit/46972f7ce2123d7ebda23daad9362650532ca59e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/21144
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 #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21144
**[Test build #89805 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89805/testReport)** for PR 21144 at commit [`46972f7`](https://github.com/apache/spark/commit/46972f7ce2123d7ebda23daad9362650532ca59e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/21144
Thanks much!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21144
**[Test build #89798 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89798/testReport)** for PR 21144 at commit [`46972f7`](https://github.com/apache/spark/commit/46972f7ce2123d7ebda23daad9362650532ca59e).
* This patch **fails Spark unit 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 #21144: [SPARK-24043][SQL] Interpreted Predicate should i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21144
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21144
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 #21144: [SPARK-24043][SQL] Interpreted Predicate should initiali...
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/21144
@hvanhovell @maropu As it turns out, there are at least two places where an InterpretedPredicate is created but never initialized: SimpleTextSource.buildReader, and ExternalCatalogUtils.prunePartitionsByFilter.
It seems unlikely that nondeterministic expressions would be used for partitions, but maybe I am not imaginative enough. Also, these seem like bugs in those two classes, not in InterpretedPredicate.
Also, I reran the mostly-interpreted SQL unit tests, this time with SortPrefix implemented, and got the error count down from 1270 to 114. None of the errors were 'requirement failed' exceptions (so no uninitialized nondeterministic expressions).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org