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-23802][SQL] PropagateEmptyRelation can leave query plan in unresolved state #20914

Closed
wants to merge 6 commits into from

Conversation

robert3005
Copy link
Contributor

What changes were proposed in this pull request?

Add cast to nulls introduced by PropagateEmptyRelation so in cases they're part of coalesce they will not break its type checking rules

How was this patch tested?

Added unit test

@hvanhovell
Copy link
Contributor

ok to test

@SparkQA
Copy link

SparkQA commented Mar 27, 2018

Test build #88630 has finished for PR 20914 at commit be950e4.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@robert3005
Copy link
Contributor Author

org.apache.spark.sql.execution.streaming.RateSourceV2Suite.basic microbatch execution failed which looks like a flake to me

@hvanhovell
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Mar 28, 2018

Test build #88642 has finished for PR 20914 at commit be950e4.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Mar 28, 2018

Test build #88646 has finished for PR 20914 at commit be950e4.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 29, 2018

Test build #88705 has finished for PR 20914 at commit d9a2ee8.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@@ -107,7 +112,7 @@ class PropagateEmptyRelationSuite extends PlanTest {
val query = testRelation1
.where(left)
.join(testRelation2.where(right), joinType = jt, condition = Some('a.attr == 'b.attr))
val optimized = Optimize.execute(query.analyze)
val optimized = Optimize.execute(query.analyze).analyze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the subsequent analyze here is to fill in timezones on casts inserted by Optimize

Copy link
Member

Choose a reason for hiding this comment

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

We should fill the time zone in the optimizer rule

@SparkQA
Copy link

SparkQA commented Mar 29, 2018

Test build #88717 has finished for PR 20914 at commit cd96401.

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

@@ -43,7 +43,7 @@ object PropagateEmptyRelation extends Rule[LogicalPlan] with PredicateHelper {

// Construct a project list from plan's output, while the value is always NULL.
private def nullValueProjectList(plan: LogicalPlan): Seq[NamedExpression] =
plan.output.map{ a => Alias(Literal(null), a.name)(a.exprId) }
plan.output.map{ a => Alias(Cast(Literal(null), a.dataType), a.name)(a.exprId) }
Copy link
Member

Choose a reason for hiding this comment

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

Please use CastSupport.cast

@SparkQA
Copy link

SparkQA commented Mar 30, 2018

Test build #88759 has finished for PR 20914 at commit 1c123bd.

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

@robert3005
Copy link
Contributor Author

@gatorsmile how does it look now?

@gatorsmile
Copy link
Member

LGTM

asfgit pushed a commit that referenced this pull request Apr 4, 2018
…esolved state

## What changes were proposed in this pull request?

Add cast to nulls introduced by PropagateEmptyRelation so in cases they're part of coalesce they will not break its type checking rules

## How was this patch tested?

Added unit test

Author: Robert Kruszewski <robertk@palantir.com>

Closes #20914 from robert3005/rk/propagate-empty-fix.

(cherry picked from commit 5cfd5fa)
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
@asfgit asfgit closed this in 5cfd5fa Apr 4, 2018
robert3005 pushed a commit to palantir/spark that referenced this pull request Apr 4, 2018
…esolved state

## What changes were proposed in this pull request?

Add cast to nulls introduced by PropagateEmptyRelation so in cases they're part of coalesce they will not break its type checking rules

## How was this patch tested?

Added unit test

Author: Robert Kruszewski <robertk@palantir.com>

Closes apache#20914 from robert3005/rk/propagate-empty-fix.
mshtelma pushed a commit to mshtelma/spark that referenced this pull request Apr 5, 2018
…esolved state

## What changes were proposed in this pull request?

Add cast to nulls introduced by PropagateEmptyRelation so in cases they're part of coalesce they will not break its type checking rules

## How was this patch tested?

Added unit test

Author: Robert Kruszewski <robertk@palantir.com>

Closes apache#20914 from robert3005/rk/propagate-empty-fix.
peter-toth pushed a commit to peter-toth/spark that referenced this pull request Oct 6, 2018
…esolved state

Add cast to nulls introduced by PropagateEmptyRelation so in cases they're part of coalesce they will not break its type checking rules

Added unit test

Author: Robert Kruszewski <robertk@palantir.com>

Closes apache#20914 from robert3005/rk/propagate-empty-fix.

(cherry picked from commit 5cfd5fa)
Signed-off-by: gatorsmile <gatorsmile@gmail.com>

Change-Id: I9c673e572454a9fa43e296b7fe66e7f2fc569854
@robert3005 robert3005 deleted the rk/propagate-empty-fix branch December 6, 2019 12:49
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