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/06 11:58:59 UTC
[GitHub] [spark] beliefer opened a new pull request, #36776: [SPARK-38997][SPARK-39037][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
beliefer opened a new pull request, #36776:
URL: https://github.com/apache/spark/pull/36776
### What changes were proposed in this pull request?
https://github.com/apache/spark/pull/35768 assume the expression in `And`, `Or` and `Not` must be predicate.
https://github.com/apache/spark/pull/36370 and https://github.com/apache/spark/pull/36325 supported push down expressions in group by and order by. But the children of `And`, `Or` and `Not` can be `FieldReference.column(name)`.
`FieldReference.column(name)` is not a predicate, so the assert may fail.
### Why are the changes needed?
This PR fix the bug for `PushableColumnWithoutNestedColumn`.
### Does this PR introduce _any_ user-facing change?
'Yes'.
Let the push-down framework more correctly.
### How was this patch tested?
New 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] cloud-fan commented on pull request #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #36776:
URL: https://github.com/apache/spark/pull/36776#issuecomment-1150721826
thanks, merging to master/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] beliefer commented on pull request #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36776:
URL: https://github.com/apache/spark/pull/36776#issuecomment-1148132608
ping @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] beliefer commented on a diff in pull request #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36776:
URL: https://github.com/apache/spark/pull/36776#discussion_r890808786
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -55,8 +55,13 @@ class V2ExpressionBuilder(
} else {
Some(FieldReference(name))
}
- case pushableColumn(name) if !nestedPredicatePushdownEnabled =>
- Some(FieldReference.column(name))
+ case col @ pushableColumn(name) if !nestedPredicatePushdownEnabled =>
Review Comment:
OK
--
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 #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36776:
URL: https://github.com/apache/spark/pull/36776#discussion_r890793608
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -55,8 +55,13 @@ class V2ExpressionBuilder(
} else {
Some(FieldReference(name))
}
- case pushableColumn(name) if !nestedPredicatePushdownEnabled =>
- Some(FieldReference.column(name))
+ case col @ pushableColumn(name) if !nestedPredicatePushdownEnabled =>
Review Comment:
can we merge the code a bit more?
```
case col @ pushableColumn(name) =>
val ref = if (nestedPredicatePushdownEnabled) ... else ...
if (predicate) ... else ...
```
--
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 closed pull request #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
URL: https://github.com/apache/spark/pull/36776
--
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] huaxingao commented on a diff in pull request #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #36776:
URL: https://github.com/apache/spark/pull/36776#discussion_r890797844
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -55,8 +55,13 @@ class V2ExpressionBuilder(
} else {
Some(FieldReference(name))
}
- case pushableColumn(name) if !nestedPredicatePushdownEnabled =>
- Some(FieldReference.column(name))
+ case col @ pushableColumn(name) if !nestedPredicatePushdownEnabled =>
Review Comment:
+1
--
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 #36776: [SPARK-38997][SPARK-39037][SQL][FOLLOWUP] `PushableColumnWithoutNestedColumn` need be translated to predicate too
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36776:
URL: https://github.com/apache/spark/pull/36776#issuecomment-1150725531
@cloud-fan @huaxingao Thank you for the review.
--
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