You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by viirya <gi...@git.apache.org> on 2017/09/13 08:44:13 UTC
[GitHub] spark pull request #19216: [SPARK-21990][SQL] QueryPlanConstraints misses so...
GitHub user viirya opened a pull request:
https://github.com/apache/spark/pull/19216
[SPARK-21990][SQL] QueryPlanConstraints misses some constraints that can be recursively inferred
## What changes were proposed in this pull request?
When I inspected the latest change of SPARK-21979, I found we could miss few constraints that can be recursively inferred.
E.g. consider constraints `a === b && b === c && a === 1 && b === 5`. Currently we only infer `a === c && b === 1 && c === 5`, but `c === 1` can be also inferred from newly inferred constraint `a === c`.
## How was this patch tested?
Added test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/viirya/spark-1 SPARK-21990
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19216.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 #19216
----
commit d76f8f0f3b0955c5baae2675157b33d0ceecb49f
Author: Liang-Chi Hsieh <vi...@gmail.com>
Date: 2017-09-13T08:41:09Z
Recursively infer constraints from newly inferred constraints.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19216: [SPARK-21990][SQL] QueryPlanConstraints misses so...
Posted by viirya <gi...@git.apache.org>.
Github user viirya closed the pull request at:
https://github.com/apache/spark/pull/19216
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19216
@gengliangwang Thanks. I'm modifying the test case.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19216
**[Test build #81711 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81711/testReport)** for PR 19216 at commit [`d76f8f0`](https://github.com/apache/spark/commit/d76f8f0f3b0955c5baae2675157b33d0ceecb49f).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19216
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 #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by gengliangwang <gi...@git.apache.org>.
Github user gengliangwang commented on the issue:
https://github.com/apache/spark/pull/19216
Hi @viirya ,
could you find a better example?
The one in the description/test case should be optimized to False.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by gengliangwang <gi...@git.apache.org>.
Github user gengliangwang commented on the issue:
https://github.com/apache/spark/pull/19216
Thanks.
I run the test case with the rule "InferFiltersFromConstraints", the result has `c > 5`.
The rule will be executed multiple times.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19216
@gengliangwang Sorry, what the meaning it should be optimized to False?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19216
I'm thinking more about this.
Actually I think we shouldn't rely on an optimization rule to produce correct constraints. If there is a logical plan other than `Filter` that can get its material of constraints from outside (maybe data source or others) in the future, we won't infer the correct constraints.
But currently looks like it doesn't cause any trouble.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19216
**[Test build #81749 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81749/testReport)** for PR 19216 at commit [`e4cffda`](https://github.com/apache/spark/commit/e4cffda91cf9ab3673e12f1427ad1d02c5e5b71e).
* 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 #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19216
**[Test build #81720 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81720/testReport)** for PR 19216 at commit [`4e85e5f`](https://github.com/apache/spark/commit/4e85e5f6faa7903d72349f0fe69f5ea3d4df6070).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19216
Yeah, as it doesn't cause any trouble now, I don't bother to re-open it.
Btw, another bad to reply on the optimization rule to produce correct constraints is the extra cost introduced by re-constructing the whole query plan tree, and the re-inferring of constraints in `Filter`. Instead of this, inferring the correct constraints in one pass should be more efficient.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19216
@gengliangwang Yeah, I see. `InferFiltersFromConstraints` will construct new `Filter` from inferred constraints and `c > 1` will be inferred in the new `Filter`. I'm trying to see if there can be an exception. If no, I think it's safe and I can close this one.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19216
@gengliangwang I think it should be safe. Let me close this now. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19216
**[Test build #81749 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81749/testReport)** for PR 19216 at commit [`e4cffda`](https://github.com/apache/spark/commit/e4cffda91cf9ab3673e12f1427ad1d02c5e5b71e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19216
@gengliangwang How about the modified one in the description?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19216
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81711/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19216
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 #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19216
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81720/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by gengliangwang <gi...@git.apache.org>.
Github user gengliangwang commented on the issue:
https://github.com/apache/spark/pull/19216
```
a === b && a === 1 && b === 5
```
The result is always False.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by gengliangwang <gi...@git.apache.org>.
Github user gengliangwang commented on the issue:
https://github.com/apache/spark/pull/19216
Yes, I agree. But constraints is used in filter inference only, and it works now. The code changes in this PR make the logic complex.
I think we should make the logic simpler, if we decide to do this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19216
**[Test build #81720 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81720/testReport)** for PR 19216 at commit [`4e85e5f`](https://github.com/apache/spark/commit/4e85e5f6faa7903d72349f0fe69f5ea3d4df6070).
* 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 #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19216
**[Test build #81711 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81711/testReport)** for PR 19216 at commit [`d76f8f0`](https://github.com/apache/spark/commit/d76f8f0f3b0955c5baae2675157b33d0ceecb49f).
* 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 #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19216
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81749/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19216: [SPARK-21990][SQL] QueryPlanConstraints misses some cons...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19216
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org