Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-17409] [SQL] [FOLLOW-UP] Do Not Optimize Query in CTAS More Than Once #15459

Closed
wants to merge 3 commits into from

Conversation

gatorsmile
Copy link
Member

What changes were proposed in this pull request?

This follow-up PR is for addressing the comment.

We added two test cases based on the suggestion from @yhuai . One is a new test case using the saveAsTable API to create a data source table. Another is for CTAS on Hive serde table.

Note: No need to backport this PR to 2.0. Will submit a new PR to backport the whole fix with new test cases to Spark 2.0

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented Oct 13, 2016

Test build #66881 has finished for PR 15459 at commit 6898f4a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class MetastoreRelationSuite extends QueryTest with SQLTestUtils with TestHiveSingleton

withTable("bar") {
withTempView("foo") {
sql("select 0 as id").createOrReplaceTempView("foo")
sql("CREATE TABLE bar AS SELECT * FROM foo group by id")
Copy link
Contributor

Choose a reason for hiding this comment

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

Also put the comment of https://github.com/apache/spark/pull/15459/files#diff-5d2ebf4e9ca5a990136b276859769289R1626 at here? Also mention that this test is for hive format table?

@SparkQA
Copy link

SparkQA commented Oct 15, 2016

Test build #66989 has finished for PR 15459 at commit 765d104.

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

asfgit pushed a commit that referenced this pull request Oct 17, 2016
…15048

### What changes were proposed in this pull request?
This PR is to backport #15048 and #15459.

However, in 2.0, we do not have a unified logical node `CreateTable` and the analyzer rule `PreWriteCheck` is also different. To minimize the code changes, this PR adds a new rule `AnalyzeCreateTableAsSelect`. Please treat it as a new PR to review. Thanks!

As explained in #14797:
>Some analyzer rules have assumptions on logical plans, optimizer may break these assumption, we should not pass an optimized query plan into QueryExecution (will be analyzed again), otherwise we may some weird bugs.
For example, we have a rule for decimal calculation to promote the precision before binary operations, use PromotePrecision as placeholder to indicate that this rule should not apply twice. But a Optimizer rule will remove this placeholder, that break the assumption, then the rule applied twice, cause wrong result.

We should not optimize the query in CTAS more than once. For example,
```Scala
spark.range(99, 101).createOrReplaceTempView("tab1")
val sqlStmt = "SELECT id, cast(id as long) * cast('1.0' as decimal(38, 18)) as num FROM tab1"
sql(s"CREATE TABLE tab2 USING PARQUET AS $sqlStmt")
checkAnswer(spark.table("tab2"), sql(sqlStmt))
```
Before this PR, the results do not match
```
== Results ==
!== Correct Answer - 2 ==       == Spark Answer - 2 ==
![100,100.000000000000000000]   [100,null]
 [99,99.000000000000000000]     [99,99.000000000000000000]
```
After this PR, the results match.
```
+---+----------------------+
|id |num                   |
+---+----------------------+
|99 |99.000000000000000000 |
|100|100.000000000000000000|
+---+----------------------+
```

In this PR, we do not treat the `query` in CTAS as a child. Thus, the `query` will not be optimized when optimizing CTAS statement. However, we still need to analyze it for normalizing and verifying the CTAS in the Analyzer. Thus, we do it in the analyzer rule `PreprocessDDL`, because so far only this rule needs the analyzed plan of the `query`.

### How was this patch tested?

Author: gatorsmile <gatorsmile@gmail.com>

Closes #15502 from gatorsmile/ctasOptimize2.0.
@gatorsmile
Copy link
Member Author

@yhuai Any further comment about it? Thanks!

@SparkQA
Copy link

SparkQA commented Oct 24, 2016

Test build #67462 has finished for PR 15459 at commit 1f2e7b8.

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

@gatorsmile
Copy link
Member Author

cc @cloud-fan @yhuai This has been reviewed in the backport PR #15502. Can we merge this to master now? Thanks!

@cloud-fan
Copy link
Contributor

LGTM

@cloud-fan
Copy link
Contributor

merging to master, thanks!

@asfgit asfgit closed this in d479c52 Oct 25, 2016
robert3005 pushed a commit to palantir/spark that referenced this pull request Nov 1, 2016
… Once

### What changes were proposed in this pull request?
This follow-up PR is for addressing the [comment](apache#15048).

We added two test cases based on the suggestion from yhuai . One is a new test case using the `saveAsTable` API to create a data source table. Another is for CTAS on Hive serde table.

Note: No need to backport this PR to 2.0. Will submit a new PR to backport the whole fix with new test cases to Spark 2.0

### How was this patch tested?
N/A

Author: gatorsmile <gatorsmile@gmail.com>

Closes apache#15459 from gatorsmile/ctasOptimizedTestCases.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
… Once

### What changes were proposed in this pull request?
This follow-up PR is for addressing the [comment](apache#15048).

We added two test cases based on the suggestion from yhuai . One is a new test case using the `saveAsTable` API to create a data source table. Another is for CTAS on Hive serde table.

Note: No need to backport this PR to 2.0. Will submit a new PR to backport the whole fix with new test cases to Spark 2.0

### How was this patch tested?
N/A

Author: gatorsmile <gatorsmile@gmail.com>

Closes apache#15459 from gatorsmile/ctasOptimizedTestCases.
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.

4 participants