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/08/03 04:24:55 UTC

[GitHub] [spark] beliefer opened a new pull request, #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

beliefer opened a new pull request, #37391:
URL: https://github.com/apache/spark/pull/37391

   ### What changes were proposed in this pull request?
   Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
   
   After this PR, the translate have only one code path, developers will easy to coding and reading.
   
   ### Why are the changes needed?
   Unify the translate path for DS V2 pushdown.
   
   
   ### Does this PR introduce _any_ user-facing change?
   'No'.
   Just update the inner implementation.
   
   
   ### How was this patch tested?
   N/A
   


-- 
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 #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37391:
URL: https://github.com/apache/spark/pull/37391#discussion_r937298399


##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -90,6 +93,8 @@ class V2ExpressionBuilder(e: Expression, isPredicate: Boolean = false) {
       }
     case Cast(child, dataType, _, true) =>
       generateExpression(child).map(v => new V2Cast(v, dataType))
+    case AggregateExpression(aggregateFunction, _, isDistinct, None, _) =>

Review Comment:
   since we only translate expressions in logical plan to v2, the agg mode should always be `Complete`.
   ```suggestion
       case AggregateExpression(aggregateFunction, Complete, isDistinct, None, _) =>
   ```



-- 
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 #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #37391:
URL: https://github.com/apache/spark/pull/37391#issuecomment-1207131948

   @cloud-fan @huaxingao Thank you for you 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

[GitHub] [spark] huaxingao commented on a diff in pull request #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #37391:
URL: https://github.com/apache/spark/pull/37391#discussion_r937028530


##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -90,6 +93,52 @@ class V2ExpressionBuilder(e: Expression, isPredicate: Boolean = false) {
       }
     case Cast(child, dataType, _, true) =>
       generateExpression(child).map(v => new V2Cast(v, dataType))
+    case AggregateExpression(aggregateFunction, _, isDistinct, None, _) =>
+      aggregateFunction match {

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 #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #37391:
URL: https://github.com/apache/spark/pull/37391#issuecomment-1203551349

   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] cloud-fan commented on a diff in pull request #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37391:
URL: https://github.com/apache/spark/pull/37391#discussion_r936959796


##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -90,6 +93,52 @@ class V2ExpressionBuilder(e: Expression, isPredicate: Boolean = false) {
       }
     case Cast(child, dataType, _, true) =>
       generateExpression(child).map(v => new V2Cast(v, dataType))
+    case AggregateExpression(aggregateFunction, _, isDistinct, None, _) =>
+      aggregateFunction match {

Review Comment:
   can we put the new code in a private method? The current method is already super long.



-- 
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 #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37391:
URL: https://github.com/apache/spark/pull/37391#issuecomment-1206303422

   thanks, merging to master!


-- 
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 #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path

Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #37391: [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
URL: https://github.com/apache/spark/pull/37391


-- 
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