[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r429007625
##
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/SchemaPruningSuite.scala
##
@@ -338,6 +340,75 @@ abstract class SchemaPruningSuite
}
}
+ testSchemaPruning("select one deep nested complex field after repartition") {
Review comment:
Ah, 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r428991332
##
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/SchemaPruningSuite.scala
##
@@ -338,6 +340,75 @@ abstract class SchemaPruningSuite
}
}
+ testSchemaPruning("select one deep nested complex field after repartition") {
Review comment:
Is this test related to 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r428991085
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasingSuite.scala
##
@@ -341,6 +341,100 @@ class NestedColumnAliasingSuite extends SchemaPruningTest
{
.analyze
comparePlans(optimized, expected)
}
+
+ test("Nested field pruning for Aggregate") {
+def runTest(basePlan: LogicalPlan => LogicalPlan): Unit = {
+ val query1 =
basePlan(contact).groupBy($"id")(first($"name.first").as("first")).analyze
+ val optimized1 = Optimize.execute(query1)
+ val aliases1 = collectGeneratedAliases(optimized1)
+
+ val expected1 = basePlan(
+contact
+.select($"id", 'name.getField("first").as(aliases1(0)))
+ ).groupBy($"id")(first($"${aliases1(0)}").as("first")).analyze
+ comparePlans(optimized1, expected1)
+
+ val query2 =
basePlan(contact).groupBy($"name.last")(first($"name.first").as("first")).analyze
+ val optimized2 = Optimize.execute(query2)
+ val aliases2 = collectGeneratedAliases(optimized2)
+
+ val expected2 = basePlan(
+contact
+.select('name.getField("last").as(aliases2(0)),
'name.getField("first").as(aliases2(1)))
+
).groupBy($"${aliases2(0)}")(first($"${aliases2(1)}").as("first")).analyze
+ comparePlans(optimized2, expected2)
+}
+
+Seq(
+ (plan: LogicalPlan) => plan,
+ (plan: LogicalPlan) => plan.limit(100),
+ (plan: LogicalPlan) => plan.repartition(100),
+ (plan: LogicalPlan) => Sample(0.0, 0.6, false, 11L, plan)).foreach {
base =>
Review comment:
Thanks for this update and looks fine.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r428969502
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -48,7 +53,10 @@ object NestedColumnAliasing {
case Project(projectList, child) =>
Project(
getNewProjectList(projectList, nestedFieldToAlias),
-replaceChildrenWithAliases(child, attrToAliases))
+replaceChildrenWithAliases(child, nestedFieldToAlias, attrToAliases))
+
+case other =>
Review comment:
Ah, I see. Thanks for the check. If so, it might be better to explicitly
filiter them here for readablity like `case other if canPruneOn(plan)`, or
leave some comments about that. Both is okay to me.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r428666366
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasingSuite.scala
##
@@ -341,6 +341,78 @@ class NestedColumnAliasingSuite extends SchemaPruningTest {
.analyze
comparePlans(optimized, expected)
}
+
+ test("Nested field pruning for Aggregate") {
+val query1 =
contact.groupBy($"id")(first($"name.first").as("first")).analyze
+val optimized1 = Optimize.execute(query1)
+val aliases1 = collectGeneratedAliases(optimized1)
+
+val expected1 = contact
+ .select($"id", 'name.getField("first").as(aliases1(0)))
+ .groupBy($"id")(first($"${aliases1(0)}").as("first")).analyze
+comparePlans(optimized1, expected1)
+
+val query2 =
contact.groupBy($"name.last")(first($"name.first").as("first")).analyze
+val optimized2 = Optimize.execute(query2)
+val aliases2 = collectGeneratedAliases(optimized2)
+
+val expected2 = contact
+ .select('name.getField("last").as(aliases2(0)),
'name.getField("first").as(aliases2(1)))
+ .groupBy($"${aliases2(0)}")(first($"${aliases2(1)}").as("first")).analyze
+comparePlans(optimized2, expected2)
+
+val query3 = contact.groupBy($"id")(first($"name"),
first($"name.first").as("first")).analyze
+val optimized3 = Optimize.execute(query3)
+val expected3 = contact.select($"id", $"name")
+ .groupBy($"id")(first($"name"), first($"name.first").as("first")).analyze
+comparePlans(optimized3, expected3)
+ }
+
+ test("Nested field pruning for Expand") {
+val query1 = Expand(
+ Seq(
+Seq($"name.first", $"name.middle"),
+Seq(ConcatWs(Seq($"name.first", $"name.middle")),
+ ConcatWs(Seq($"name.middle", $"name.first")))
+ ),
+ Seq('a.string, 'b.string),
+ contact
Review comment:
ditto; https://github.com/apache/spark/pull/28560#discussion_r428665197
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r428662926
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -48,7 +53,10 @@ object NestedColumnAliasing {
case Project(projectList, child) =>
Project(
getNewProjectList(projectList, nestedFieldToAlias),
-replaceChildrenWithAliases(child, attrToAliases))
+replaceChildrenWithAliases(child, nestedFieldToAlias, attrToAliases))
+
+case other =>
Review comment:
This case only matches `Aggregate` and `Expand` now?
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasingSuite.scala
##
@@ -341,6 +341,78 @@ class NestedColumnAliasingSuite extends SchemaPruningTest {
.analyze
comparePlans(optimized, expected)
}
+
+ test("Nested field pruning for Aggregate") {
+val query1 =
contact.groupBy($"id")(first($"name.first").as("first")).analyze
Review comment:
Could you add tests for the cases,
`contact.(limit/repartition/sample).groupBy()...`? IIUC this PR could support
them, too?
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -35,6 +35,11 @@ object NestedColumnAliasing {
case Project(projectList, child)
if SQLConf.get.nestedSchemaPruningEnabled &&
canProjectPushThrough(child) =>
getAliasSubMap(projectList)
+
+case plan if SQLConf.get.nestedSchemaPruningEnabled && canPruneOn(plan) =>
+ val exprsToPrune = plan.expressions
Review comment:
nit: the consistent var name?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r426344997
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -68,10 +76,23 @@ object NestedColumnAliasing {
*/
def replaceChildrenWithAliases(
plan: LogicalPlan,
+ nestedFieldToAlias: Map[ExtractValue, Alias],
attrToAliases: Map[ExprId, Seq[Alias]]): LogicalPlan = {
plan.withNewChildren(plan.children.map { plan =>
Project(plan.output.flatMap(a => attrToAliases.getOrElse(a.exprId,
Seq(a))), plan)
-})
+}).transformExpressions {
+ case f: ExtractValue if nestedFieldToAlias.contains(f) =>
+nestedFieldToAlias(f).toAttribute
+}
+ }
+
+ /**
+ * Returns true for those operators that we can prune nested column on it.
+ */
+ private def canPruneOn(plan: LogicalPlan) = plan match {
+case _: Aggregate => true
+case _: Expand => true
+case _ => false
Review comment:
cc: @HyukjinKwon
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r426343642
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -68,10 +76,23 @@ object NestedColumnAliasing {
*/
def replaceChildrenWithAliases(
plan: LogicalPlan,
+ nestedFieldToAlias: Map[ExtractValue, Alias],
attrToAliases: Map[ExprId, Seq[Alias]]): LogicalPlan = {
plan.withNewChildren(plan.children.map { plan =>
Project(plan.output.flatMap(a => attrToAliases.getOrElse(a.exprId,
Seq(a))), plan)
-})
+}).transformExpressions {
+ case f: ExtractValue if nestedFieldToAlias.contains(f) =>
+nestedFieldToAlias(f).toAttribute
+}
+ }
+
+ /**
+ * Returns true for those operators that we can prune nested column on it.
+ */
+ private def canPruneOn(plan: LogicalPlan) = plan match {
+case _: Aggregate => true
+case _: Expand => true
+case _ => false
Review comment:
> Currently I think nested column pruning test cases are all in Scala,
no Python. I will think about how to add test.
okay, thanks for the check. Yea, having tests for Python cases looks nice.
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -68,10 +76,23 @@ object NestedColumnAliasing {
*/
def replaceChildrenWithAliases(
plan: LogicalPlan,
+ nestedFieldToAlias: Map[ExtractValue, Alias],
attrToAliases: Map[ExprId, Seq[Alias]]): LogicalPlan = {
plan.withNewChildren(plan.children.map { plan =>
Project(plan.output.flatMap(a => attrToAliases.getOrElse(a.exprId,
Seq(a))), plan)
-})
+}).transformExpressions {
+ case f: ExtractValue if nestedFieldToAlias.contains(f) =>
+nestedFieldToAlias(f).toAttribute
+}
+ }
+
+ /**
+ * Returns true for those operators that we can prune nested column on it.
+ */
+ private def canPruneOn(plan: LogicalPlan) = plan match {
+case _: Aggregate => true
+case _: Expand => true
+case _ => false
Review comment:
> Currently I think nested column pruning test cases are all in Scala,
no Python. I will think about how to add test.
Okay, thanks for the check. Yea, having tests for Python cases looks nice.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r426343642
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -68,10 +76,23 @@ object NestedColumnAliasing {
*/
def replaceChildrenWithAliases(
plan: LogicalPlan,
+ nestedFieldToAlias: Map[ExtractValue, Alias],
attrToAliases: Map[ExprId, Seq[Alias]]): LogicalPlan = {
plan.withNewChildren(plan.children.map { plan =>
Project(plan.output.flatMap(a => attrToAliases.getOrElse(a.exprId,
Seq(a))), plan)
-})
+}).transformExpressions {
+ case f: ExtractValue if nestedFieldToAlias.contains(f) =>
+nestedFieldToAlias(f).toAttribute
+}
+ }
+
+ /**
+ * Returns true for those operators that we can prune nested column on it.
+ */
+ private def canPruneOn(plan: LogicalPlan) = plan match {
+case _: Aggregate => true
+case _: Expand => true
+case _ => false
Review comment:
okay, thanks for the check.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28560: [SPARK-27217][SQL] Nested column aliasing for more operators which can prune nested column
maropu commented on a change in pull request #28560:
URL: https://github.com/apache/spark/pull/28560#discussion_r426341109
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala
##
@@ -68,10 +76,23 @@ object NestedColumnAliasing {
*/
def replaceChildrenWithAliases(
plan: LogicalPlan,
+ nestedFieldToAlias: Map[ExtractValue, Alias],
attrToAliases: Map[ExprId, Seq[Alias]]): LogicalPlan = {
plan.withNewChildren(plan.children.map { plan =>
Project(plan.output.flatMap(a => attrToAliases.getOrElse(a.exprId,
Seq(a))), plan)
-})
+}).transformExpressions {
+ case f: ExtractValue if nestedFieldToAlias.contains(f) =>
+nestedFieldToAlias(f).toAttribute
+}
+ }
+
+ /**
+ * Returns true for those operators that we can prune nested column on it.
+ */
+ private def canPruneOn(plan: LogicalPlan) = plan match {
+case _: Aggregate => true
+case _: Expand => true
+case _ => false
Review comment:
Are the current entries all the case we can support for nested column
pruning? How about `FlatMapGroupsInPandas`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
