[SPARK-20329][SQL] Make timezone aware expression without timezone unresolved #17641

hvanhovell · 2017-04-14T20:24:29Z

What changes were proposed in this pull request?

A cast expression with a resolved time zone is not equal to a cast expression without a resolved time zone. The ResolveAggregateFunction assumed that these expression were the same, and would fail to resolve HAVING clauses which contain a Cast expression.

This is in essence caused by the fact that a TimeZoneAwareExpression can be resolved without a set time zone. This PR fixes this, and makes a TimeZoneAwareExpression unresolved as long as it has no TimeZone set.

How was this patch tested?

Added a regression test to the SQLQueryTestSuite.having file.

SparkQA · 2017-04-14T22:07:36Z

Test build #75816 has finished for PR 17641 at commit 0654409.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ResolveTimeZone(conf: SQLConf) extends Rule[LogicalPlan]

cloud-fan · 2017-04-16T04:24:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

 trait TimeZoneAwareExpression extends Expression {
+  /** The expression is only resolved when the time zone has been set. */
+  override lazy val resolved: Boolean =
+    childrenResolved && checkInputDataTypes().isSuccess && timeZoneId.isDefined


The implementation may have some custom resolution logic, how about super.resolved && timeZoneId.isDefined?

I tried doing this. Scala does not allow you to call super.resolved in resolved :(....

viirya · 2017-04-17T03:12:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInlineTables.scala

-            case e: TimeZoneAwareExpression if e.timeZoneId.isEmpty =>
-              e.withTimeZone(conf.sessionLocalTimeZone)
-          }.eval()
+          castedExpr.eval()


If there are nested expressions which are timezone aware, I think we still need to attach time zone to them?

I guess now that TimeZoneAwareExpression is resolved if it has timeZoneId, so we don't need to transform children.

oh, right. I saw the changes to TimeZoneAwareExpression. :-)

SparkQA · 2017-04-18T15:07:56Z

Test build #75898 has finished for PR 17641 at commit ceec2b0.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ResolveTimeZone(conf: SQLConf) extends Rule[LogicalPlan]
trait CastSupport
case class AliasViewChild(conf: SQLConf) extends Rule[LogicalPlan] with CastSupport
case class DataSourceAnalysis(conf: SQLConf) extends Rule[LogicalPlan] with CastSupport
case class DataSourceStrategy(conf: SQLConf) extends Strategy with Logging with CastSupport
case class PreprocessTableInsertion(conf: SQLConf) extends Rule[LogicalPlan] with CastSupport

SparkQA · 2017-04-18T16:36:46Z

Test build #75901 has finished for PR 17641 at commit b5848ca.

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

ueshin · 2017-04-19T02:41:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInlineTables.scala

            e
          } else {
-            Cast(e, targetType)
+            Cast(e, targetType, Some(conf.sessionLocalTimeZone))


ResolveInlineTables can be CastSupport and we should use cast of it here?

cloud-fan · 2017-04-19T02:48:29Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInlineTables.scala

            e
          } else {
-            Cast(e, targetType)
+            Cast(e, targetType, Some(conf.sessionLocalTimeZone))


also make ResolveInlineTables mix-in CastSupport?

cloud-fan · 2017-04-19T02:51:42Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

    }
  }

+  private def castWithDefaultTZ(v: Any, targetType: DataType): Cast = {


shall we just change the default value of timeZoneId parameter in def cast to GMT?

cloud-fan · 2017-04-19T02:52:55Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala

        val partValue = potentialSpecs.head._2
-        Some(Alias(Cast(Literal(partValue), field.dataType), field.name)())
+        Some(Alias(cast(Literal(partValue), field.dataType),
+          field.name)())


nit: this can be put in the previous line

cloud-fan · 2017-04-19T02:53:59Z

sql/core/src/main/scala/org/apache/spark/sql/internal/BaseSessionStateBuilder.scala

    override val postHocResolutionRules: Seq[Rule[LogicalPlan]] =
      PreprocessTableCreation(session) +:
        PreprocessTableInsertion(conf) +:
+        ResolveTimeZone(conf) +:


why run this rule again? it's already in the resolution batch

SparkQA · 2017-04-20T12:17:26Z

Test build #75979 has finished for PR 17641 at commit cc8de3d.

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

SparkQA · 2017-04-20T12:26:20Z

Test build #75981 has finished for PR 17641 at commit d7bc1e8.

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

cloud-fan · 2017-04-21T02:06:30Z

LGTM, merging to master/2.2

…resolved ## What changes were proposed in this pull request? A cast expression with a resolved time zone is not equal to a cast expression without a resolved time zone. The `ResolveAggregateFunction` assumed that these expression were the same, and would fail to resolve `HAVING` clauses which contain a `Cast` expression. This is in essence caused by the fact that a `TimeZoneAwareExpression` can be resolved without a set time zone. This PR fixes this, and makes a `TimeZoneAwareExpression` unresolved as long as it has no TimeZone set. ## How was this patch tested? Added a regression test to the `SQLQueryTestSuite.having` file. Author: Herman van Hovell <[email protected]> Closes #17641 from hvanhovell/SPARK-20329. (cherry picked from commit 760c8d0) Signed-off-by: Wenchen Fan <[email protected]>

…resolved ## What changes were proposed in this pull request? A cast expression with a resolved time zone is not equal to a cast expression without a resolved time zone. The `ResolveAggregateFunction` assumed that these expression were the same, and would fail to resolve `HAVING` clauses which contain a `Cast` expression. This is in essence caused by the fact that a `TimeZoneAwareExpression` can be resolved without a set time zone. This PR fixes this, and makes a `TimeZoneAwareExpression` unresolved as long as it has no TimeZone set. ## How was this patch tested? Added a regression test to the `SQLQueryTestSuite.having` file. Author: Herman van Hovell <[email protected]> Closes apache#17641 from hvanhovell/SPARK-20329.

Make timezone aware expression without timezone unresolved.

0654409

hvanhovell changed the title ~~[SPARK-20329][SQL] Make timezone aware expression without timezone unresolved.~~ [SPARK-20329][SQL] Make timezone aware expression without timezone unresolved [WIP] Apr 14, 2017

cloud-fan reviewed Apr 16, 2017

View reviewed changes

viirya reviewed Apr 17, 2017

View reviewed changes

hvanhovell added 2 commits April 18, 2017 14:43

Fix tests, and try to improve cast handling.

ceec2b0

Add regression test

b5848ca

hvanhovell changed the title ~~[SPARK-20329][SQL] Make timezone aware expression without timezone unresolved [WIP]~~ [SPARK-20329][SQL] Make timezone aware expression without timezone unresolved Apr 18, 2017

ueshin reviewed Apr 19, 2017

View reviewed changes

cloud-fan reviewed Apr 19, 2017

View reviewed changes

hvanhovell added 3 commits April 20, 2017 11:51

Code review

88be49e

Code review

cc8de3d

Remove unneeded ResolveTimeZone in analyzer

d7bc1e8

asfgit closed this in 760c8d0 Apr 21, 2017

[SPARK-20329][SQL] Make timezone aware expression without timezone unresolved #17641

[SPARK-20329][SQL] Make timezone aware expression without timezone unresolved #17641

Uh oh!

Conversation

hvanhovell commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ueshin Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 18, 2017

Uh oh!

SparkQA commented Apr 18, 2017

Uh oh!

ueshin Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 20, 2017

Uh oh!

SparkQA commented Apr 20, 2017

Uh oh!

cloud-fan commented Apr 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hvanhovell commented Apr 14, 2017 •

edited

Loading

ueshin Apr 17, 2017 •

edited

Loading

ueshin Apr 19, 2017 •

edited

Loading