You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by henryr <gi...@git.apache.org> on 2018/02/28 00:52:25 UTC

[GitHub] spark pull request #20687: [SPARK-25000][SQL] Fix complex type simplificatio...

GitHub user henryr opened a pull request:

    https://github.com/apache/spark/pull/20687

    [SPARK-25000][SQL] Fix complex type simplification rules to apply to entire plan

    ## What changes were proposed in this pull request?
    
    Complex type simplification optimizer rules were not applied to the
    entire plan, just the expressions reachable from the root node. This
    patch fixes the rules to transform the entire plan.
    
    ## How was this patch tested?
    
    New unit test + ran sql / core tests.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/henryr/spark spark-25000

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/20687.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 #20687
    
----
commit f446fa2482f2ccdfa6fec7243bb25b2e232da5fa
Author: Henry Robinson <he...@...>
Date:   2018-02-28T00:42:17Z

    [SPARK-25000][SQL] Fix complex type simplification rules to apply to entire plan
    
    ## What changes were proposed in this pull request?
    
    Complex type simplification optimizer rules were not applied to the
    entire plan, just the expressions reachable from the root node. This
    patch fixes the rules to transform the entire plan.
    
    ## How was this patch tested?
    
    New unit test + sql / core tests.

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by gatorsmile <gi...@git.apache.org>.

Github user gatorsmile commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r175149719
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala ---
    @@ -19,57 +19,47 @@ package org.apache.spark.sql.catalyst.optimizer
     
     import org.apache.spark.sql.catalyst.expressions._
     import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
    +import org.apache.spark.sql.catalyst.plans.logical.Aggregate
    --- End diff --
    
    > import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, LogicalPlan}


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by dongjoon-hyun <gi...@git.apache.org>.

Github user dongjoon-hyun commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r174974033
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala ---
    @@ -19,57 +19,47 @@ package org.apache.spark.sql.catalyst.optimizer
     
     import org.apache.spark.sql.catalyst.expressions._
     import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
    +import org.apache.spark.sql.catalyst.plans.logical.Aggregate
    --- End diff --
    
    This should be before line 21 in alphabetical order.
    You can check this locally with `dev/scalastyle`.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    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 #20687: [SPARK-25000][SQL] Fix complex type simplification rules...

Posted by gatorsmile <gi...@git.apache.org>.

Github user gatorsmile commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    cc @dongjoon-hyun Do you want to review this PR? 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-25000][SQL] Fix complex type simplification rules...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    **[Test build #87763 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87763/testReport)** for PR 20687 at commit [`f446fa2`](https://github.com/apache/spark/commit/f446fa2482f2ccdfa6fec7243bb25b2e232da5fa).
     * 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 pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by dongjoon-hyun <gi...@git.apache.org>.

Github user dongjoon-hyun commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r174620713
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala ---
    @@ -22,54 +22,34 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
     import org.apache.spark.sql.catalyst.rules.Rule
     
     /**
    -* push down operations into [[CreateNamedStructLike]].
    -*/
    -object SimplifyCreateStructOps extends Rule[LogicalPlan] {
    -  override def apply(plan: LogicalPlan): LogicalPlan = {
    -    plan.transformExpressionsUp {
    -      // push down field extraction
    + * Simplify redundant [[CreateNamedStructLike]], [[CreateArray]] and [[CreateMap]] expressions.
    + */
    +object SimplifyExtractValueOps extends Rule[LogicalPlan] {
    +  override def apply(plan: LogicalPlan): LogicalPlan = plan transform { case p =>
    +    p.transformExpressionsUp {
    --- End diff --
    
    Since `map` is not orderable, that happens for `struct` and `array` types.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-25000][SQL] Fix complex type simplification rules...

Posted by henryr <gi...@git.apache.org>.

Github user henryr commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by henryr <gi...@git.apache.org>.

Github user henryr commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r173323846
  
    --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala ---
    @@ -331,4 +330,31 @@ class ComplexTypesSuite extends PlanTest with ExpressionEvalHelper {
           .analyze
         comparePlans(Optimizer execute rel, expected)
       }
    +
    +  test("SPARK-23500: Simplify complex ops that aren't at the plan root") {
    +    val structRel = relation
    +      .select(GetStructField(CreateNamedStruct(Seq("att1", 'nullable_id)), 0, None) as "foo")
    +      .groupBy($"foo")("1").analyze
    +    val structExpected = relation
    +      .select('nullable_id as "foo")
    +      .groupBy($"foo")("1").analyze
    +    comparePlans(Optimizer execute structRel, structExpected)
    +
    +    // If nullable attributes aren't used in the 'expected' plans, the array and map test
    +    // cases fail because array and map indexing can return null so the output attribute
    --- End diff --
    
    Done, thanks. I filed SPARK-23634 to fix this. Out of interest, why does `AttributeReference` cache the nullability of its referent? Is it because comparison is too expensive to do if you have to follow a level of indirection to get to the original attribute? 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by cloud-fan <gi...@git.apache.org>.

Github user cloud-fan commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r172985682
  
    --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala ---
    @@ -331,4 +330,30 @@ class ComplexTypesSuite extends PlanTest with ExpressionEvalHelper {
           .analyze
         comparePlans(Optimizer execute rel, expected)
       }
    +
    +  test("SPARK-23500: Simplify complex ops that aren't at the plan root") {
    +    // If nullable attributes aren't used, the array and map test cases fail because array
    +    // and map indexing can return null so the output is marked nullable.
    --- End diff --
    
    why? I think the optimization is still valid, we should show this in the test, instead of hiding it with a nullable attribute.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by henryr <gi...@git.apache.org>.

Github user henryr commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r172992262
  
    --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala ---
    @@ -331,4 +330,30 @@ class ComplexTypesSuite extends PlanTest with ExpressionEvalHelper {
           .analyze
         comparePlans(Optimizer execute rel, expected)
       }
    +
    +  test("SPARK-23500: Simplify complex ops that aren't at the plan root") {
    +    // If nullable attributes aren't used, the array and map test cases fail because array
    +    // and map indexing can return null so the output is marked nullable.
    --- End diff --
    
    The optimization works either way, but in (for example) the map case, `m1` is marked as nullable in the original plan because presumably `GetMapValue(CreateMap(...))` can return `null` if the key is not in the map. 
    
    So for the expected plan to compare the same as the original, it has to be reading a nullable attribute - otherwise the plans don't pass `comparePlans`.  I moved and reworded the comment to hopefully clarify this a bit.
    
    There's an opportunity to fix this up again after the rule completes (since some attributes could be marked too conservatively as nullable). Do you think that's something we should pursue for this PR?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1542/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by gatorsmile <gi...@git.apache.org>.

Github user gatorsmile commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    @henryr Thanks for your great work!


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by dongjoon-hyun <gi...@git.apache.org>.

Github user dongjoon-hyun commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r174663562
  
    --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala ---
    @@ -331,4 +330,37 @@ class ComplexTypesSuite extends PlanTest with ExpressionEvalHelper {
           .analyze
         comparePlans(Optimizer execute rel, expected)
       }
    +
    +  test("SPARK-23500: Simplify complex ops that aren't at the plan root") {
    +    val structRel = relation
    +      .select(GetStructField(CreateNamedStruct(Seq("att1", 'nullable_id)), 0, None) as "foo")
    +      .groupBy($"foo")("1").analyze
    +    val structExpected = relation
    +      .select('nullable_id as "foo")
    +      .groupBy($"foo")("1").analyze
    +    comparePlans(Optimizer execute structRel, structExpected)
    +
    +    // These tests must use nullable attributes from the base relation for the following reason:
    +    // in the 'original' plans below, the Aggregate node produced by groupBy() has a
    +    // nullable AttributeReference to a1, because both array indexing and map lookup are
    +    // nullable expressions. After optimization, the same attribute is now non-nullable,
    +    // but the AttributeReference is not updated to reflect this. In the 'expected' plans,
    +    // the grouping expressions have the same nullability as the original attribute in the
    +    // relation. If that attribute is non-nullable, the tests will fail as the plans will
    +    // compare differently, so for these tests we must use a nullable attribute. See
    +    // SPARK-23634.
    +    val arrayRel = relation
    +      .select(GetArrayItem(CreateArray(Seq('nullable_id, 'nullable_id + 1L)), 0) as "a1")
    +      .groupBy($"a1")("1").analyze
    +    val arrayExpected = relation.select('nullable_id as "a1").groupBy($"a1")("1").analyze
    +    comparePlans(Optimizer execute arrayRel, arrayExpected)
    +
    +    val mapRel = relation
    +      .select(GetMapValue(CreateMap(Seq("id", 'nullable_id)), "id") as "m1")
    +      .groupBy($"m1")("1").analyze
    +    val mapExpected = relation
    +      .select('nullable_id as "m1")
    +      .groupBy($"m1")("1").analyze
    +    comparePlans(Optimizer execute mapRel, mapExpected)
    --- End diff --
    
    @henryr .
    Could you add more test cases mentioned today, for example, like the following?
    ```
        val structRel = relation.groupBy(
          CreateNamedStruct(Seq("att1", 'nullable_id)))(
          GetStructField(CreateNamedStruct(Seq("att1", 'nullable_id)), 0, None)).analyze
        comparePlans(Optimizer execute structRel, structRel)
    ```


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    **[Test build #88035 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/88035/testReport)** for PR 20687 at commit [`63c7098`](https://github.com/apache/spark/commit/63c7098fc4b14af7859580682f17c73abcd7ff08).
     * 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 pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by cloud-fan <gi...@git.apache.org>.

Github user cloud-fan commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r173012266
  
    --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala ---
    @@ -331,4 +330,31 @@ class ComplexTypesSuite extends PlanTest with ExpressionEvalHelper {
           .analyze
         comparePlans(Optimizer execute rel, expected)
       }
    +
    +  test("SPARK-23500: Simplify complex ops that aren't at the plan root") {
    +    val structRel = relation
    +      .select(GetStructField(CreateNamedStruct(Seq("att1", 'nullable_id)), 0, None) as "foo")
    +      .groupBy($"foo")("1").analyze
    +    val structExpected = relation
    +      .select('nullable_id as "foo")
    +      .groupBy($"foo")("1").analyze
    +    comparePlans(Optimizer execute structRel, structExpected)
    +
    +    // If nullable attributes aren't used in the 'expected' plans, the array and map test
    +    // cases fail because array and map indexing can return null so the output attribute
    --- End diff --
    
    `nullable` is mostly calculated on demand, so we don't have rules to change the `nullable` property. For this case, the expression is `Alias(GetArrayItem(CreateArray(Attribute...)))`, which is nullable. After optimize, it becomes `Alias(Attribute...)` and is not nullable(if that attribute is not nullable). So the `nullable` is updated automatically.
    
    I don't know why you hit this issue, please ping us if you can't figure it out, we can help to debug.
    



---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by dongjoon-hyun <gi...@git.apache.org>.

Github user dongjoon-hyun commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r173519701
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala ---
    @@ -22,32 +22,24 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
     import org.apache.spark.sql.catalyst.rules.Rule
     
     /**
    -* push down operations into [[CreateNamedStructLike]].
    +* Simplify redundant [[CreateNamedStructLike]], [[CreateArray]] and [[CreateMap]] expressions.
     */
    --- End diff --
    
    nit. Could you fix the indentation?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by dongjoon-hyun <gi...@git.apache.org>.

Github user dongjoon-hyun commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r173520165
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala ---
    @@ -22,32 +22,24 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
     import org.apache.spark.sql.catalyst.rules.Rule
     
     /**
    -* push down operations into [[CreateNamedStructLike]].
    +* Simplify redundant [[CreateNamedStructLike]], [[CreateArray]] and [[CreateMap]] expressions.
     */
    -object SimplifyCreateStructOps extends Rule[LogicalPlan] {
    -  override def apply(plan: LogicalPlan): LogicalPlan = {
    -    plan.transformExpressionsUp {
    -      // push down field extraction
    +object SimplifyExtractValueOps extends Rule[LogicalPlan] {
    +  override def apply(plan: LogicalPlan): LogicalPlan = plan transform { case p =>
    +    p.transformExpressionsUp {
    +      // Remove redundant field extraction.
           case GetStructField(createNamedStructLike: CreateNamedStructLike, ordinal, _) =>
             createNamedStructLike.valExprs(ordinal)
    -    }
    -  }
    -}
     
    -/**
    -* push down operations into [[CreateArray]].
    -*/
    -object SimplifyCreateArrayOps extends Rule[LogicalPlan] {
    -  override def apply(plan: LogicalPlan): LogicalPlan = {
    -    plan.transformExpressionsUp {
    -      // push down field selection (array of structs)
    +      // Remove redundant array indexing.
           case GetArrayStructFields(CreateArray(elems), field, ordinal, numFields, containsNull) =>
    --- End diff --
    
    nit.
    ```
    case GetArrayStructFields(CreateArray(elems), field, ordinal, _, _) =>
    ```


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    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 #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    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 #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1619/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-25000][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    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 #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1435/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by dongjoon-hyun <gi...@git.apache.org>.

Github user dongjoon-hyun commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r174606237
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala ---
    @@ -22,54 +22,34 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
     import org.apache.spark.sql.catalyst.rules.Rule
     
     /**
    -* push down operations into [[CreateNamedStructLike]].
    -*/
    -object SimplifyCreateStructOps extends Rule[LogicalPlan] {
    -  override def apply(plan: LogicalPlan): LogicalPlan = {
    -    plan.transformExpressionsUp {
    -      // push down field extraction
    + * Simplify redundant [[CreateNamedStructLike]], [[CreateArray]] and [[CreateMap]] expressions.
    + */
    +object SimplifyExtractValueOps extends Rule[LogicalPlan] {
    +  override def apply(plan: LogicalPlan): LogicalPlan = plan transform { case p =>
    +    p.transformExpressionsUp {
    --- End diff --
    
    Expression-level optimizer simplifies both `aggregateExpressions` and `groupingExpressions` together. If the target expression exists at somewhere of both sides, the simplified expression also exists at the same locations of both sides. Given that, `semanticEquals` will work for the updated expressions.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/88024/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    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 #20687: [SPARK-23500][SQL] Fix complex type simplification rules...

Posted by henryr <gi...@git.apache.org>.

Github user henryr commented on the issue:

    https://github.com/apache/spark/pull/20687
  
    This failing because of SPARK-23606, which seems unrelated (I haven't been able to trigger it in local builds, at least). 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #20687: [SPARK-23500][SQL] Fix complex type simplificatio...

Posted by dongjoon-hyun <gi...@git.apache.org>.

Github user dongjoon-hyun commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20687#discussion_r173667671
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala ---
    @@ -22,54 +22,34 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
     import org.apache.spark.sql.catalyst.rules.Rule
     
     /**
    -* push down operations into [[CreateNamedStructLike]].
    -*/
    -object SimplifyCreateStructOps extends Rule[LogicalPlan] {
    -  override def apply(plan: LogicalPlan): LogicalPlan = {
    -    plan.transformExpressionsUp {
    -      // push down field extraction
    + * Simplify redundant [[CreateNamedStructLike]], [[CreateArray]] and [[CreateMap]] expressions.
    + */
    +object SimplifyExtractValueOps extends Rule[LogicalPlan] {
    +  override def apply(plan: LogicalPlan): LogicalPlan = plan transform { case p =>
    +    p.transformExpressionsUp {
    --- End diff --
    
    Sorry for late response, @gatorsmile .
    These are expression-level optimization rules. If the original expressions exists in `SELECT`, `GROUP BY`, and `HAVING`, those are simplified in the same way together. Do you have any concerning cases?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org