Skip to content

Conversation

@maropu
Copy link
Member

@maropu maropu commented May 13, 2017

What changes were proposed in this pull request?

This pr added a new Optimizer rule to combine nested Concat. The master supports a pipeline operator '||' to concatenate strings in #17711 (This pr is follow-up). Since the parser currently generates nested Concat expressions, the optimizer needs to combine the nested expressions.

How was this patch tested?

Added tests in CombineConcatSuite and SQLQueryTestSuite.

@SparkQA
Copy link

SparkQA commented May 13, 2017

Test build #76889 has finished for PR 17970 at commit 7b5bff5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

SimplifyCreateArrayOps,
SimplifyCreateMapOps) ++
SimplifyCreateMapOps,
CombineConcat) ++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @maropu .
CombineConcats like the other Combine~ optimizer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, comments! Fixed.

@dongjoon-hyun
Copy link
Member

+1, LGTM except one minor naming comment.

@SparkQA
Copy link

SparkQA commented May 13, 2017

Test build #76902 has finished for PR 17970 at commit 4a98693.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gatorsmile
Copy link
Member

cc @cloud-fan

import org.apache.spark.sql.types.StringType


class CombineConcatsSuite extends PlanTest with PredicateHelper {
Copy link
Member

@viirya viirya May 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we used PredicateHelper here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Removed.

@viirya
Copy link
Member

viirya commented May 14, 2017

LGTM except one minor comment.

@SparkQA
Copy link

SparkQA commented May 14, 2017

Test build #76908 has finished for PR 17970 at commit 676d483.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class CombineConcatsSuite extends PlanTest

@cloud-fan
Copy link
Contributor

LGTM, merging to master!

@asfgit asfgit closed this in b0888d1 May 15, 2017
robert3005 pushed a commit to palantir/spark that referenced this pull request May 19, 2017
## What changes were proposed in this pull request?
This pr added a new Optimizer rule to combine nested Concat. The master supports a pipeline operator '||' to concatenate strings in apache#17711 (This pr is follow-up). Since the parser currently generates nested Concat expressions, the optimizer needs to combine the nested expressions.

## How was this patch tested?
Added tests in `CombineConcatSuite` and `SQLQueryTestSuite`.

Author: Takeshi Yamamuro <[email protected]>

Closes apache#17970 from maropu/SPARK-20730.
liyichao pushed a commit to liyichao/spark that referenced this pull request May 24, 2017
## What changes were proposed in this pull request?
This pr added a new Optimizer rule to combine nested Concat. The master supports a pipeline operator '||' to concatenate strings in apache#17711 (This pr is follow-up). Since the parser currently generates nested Concat expressions, the optimizer needs to combine the nested expressions.

## How was this patch tested?
Added tests in `CombineConcatSuite` and `SQLQueryTestSuite`.

Author: Takeshi Yamamuro <[email protected]>

Closes apache#17970 from maropu/SPARK-20730.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants