You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/06/21 03:03:31 UTC
[GitHub] [spark] beliefer opened a new pull request, #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
beliefer opened a new pull request, #36934:
URL: https://github.com/apache/spark/pull/36934
### What changes were proposed in this pull request?
Currently, `InferFiltersFromConstraints` construct a new `Filter` which condition without the same order in scala 2.12 and 2.13
It seems `--` of `ExpressionSet` cannot guarantee the order of elements.
The behavior lead to https://github.com/apache/spark/pull/35947#discussion_r901193520
### Why are the changes needed?
Fix bug
### Does this PR introduce _any_ user-facing change?
'Yes'.
The behavior will be consistent between scala 2.12 and 2.13
### How was this patch tested?
Exists tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer closed pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
beliefer closed pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
URL: https://github.com/apache/spark/pull/36934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902192967
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
Yes, after SPARK-39520, this issue should no longer exists， and @HyukjinKwon already mark SPARK-39523 as RESOLVED
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902220614
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
@cloud-fan @LuciferYang I have tested and the issue no longer exists. I will close this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902196418
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
It seems https://github.com/apache/spark/pull/36925 fixed this issue ? @LuciferYang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36934:
URL: https://github.com/apache/spark/pull/36934#issuecomment-1161301750
ping @HyukjinKwon @huaxingao cc @cloud-fan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902196437
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
[SPARK-39520](https://issues.apache.org/jira/browse/SPARK-39520) was not merged to 3.3, do we have this issue in 3.3?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902197149
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
> It seems #36925 fixed this issue ? @LuciferYang
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902197865
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
> [SPARK-39520](https://issues.apache.org/jira/browse/SPARK-39520) was not merged to 3.3, do we have this issue in 3.3?
Let me double check this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902224853
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
> [SPARK-39520](https://issues.apache.org/jira/browse/SPARK-39520) was not merged to 3.3, do we have this issue in 3.3?
@cloud-fan 3.3 no this issue due to SPARK-38836 not in branch-3.3, the `--` method hasn't been deleted, and I manually check the relevant suites(inlcude TPCDSV1_4_PlanStabilitySuite, AdaptiveQueryExecSuite, ExpressionSetSuite, JDBCV2Suite, GeneratorFunctionSuite), all passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902192967
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
Yes, after SPARK-39520, this issue no longer exists， and @HyukjinKwon already mark SPARK-39523 as RESOLVED
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36934: [SPARK-39537][SQL] Ensure that the expressions order returned by `InferFiltersFromConstraints`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36934:
URL: https://github.com/apache/spark/pull/36934#discussion_r902185170
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
is it possible to override `--` in `ExpressionSet`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1272,8 +1272,11 @@ object InferFiltersFromConstraints extends Rule[LogicalPlan]
private def inferFilters(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(FILTER, JOIN)) {
case filter @ Filter(condition, child) =>
- val newFilters = filter.constraints --
Review Comment:
cc @LuciferYang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org