[SPARK-21739][SQL]Cast expression should initialize timezoneId when it is called statically to convert something into TimestampType #18960

DonnyZone · 2017-08-16T08:13:58Z

What changes were proposed in this pull request?

https://issues.apache.org/jira/projects/SPARK/issues/SPARK-21739

This issue is caused by introducing TimeZoneAwareExpression.
When the Cast expression converts something into TimestampType, it should be resolved with setting timezoneId. In general, it is resolved in LogicalPlan phase.

However, there are still some places that use Cast expression statically to convert datatypes without setting timezoneId. In such cases, NoSuchElementException: None.get will be thrown for TimestampType.

This PR is proposed to fix the issue. We have checked the whole project and found two such usages(i.e., inTableReader and HiveTableScanExec).

How was this patch tested?

unit test

DonnyZone · 2017-08-16T13:48:17Z

@cloud-fan @gatorsmile

gatorsmile · 2017-08-16T19:24:28Z

ok to test

gatorsmile · 2017-08-16T19:24:37Z

Also cc @ueshin

gatorsmile · 2017-08-16T19:31:09Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveTableScanExec.scala

@@ -104,7 +105,7 @@ case class HiveTableScanExec(
    hadoopConf)

  private def castFromString(value: String, dataType: DataType) = {
-    Cast(Literal(value), dataType).eval(null)
+    Cast(Literal(value), dataType, Option(SQLConf.get.sessionLocalTimeZone)).eval(null)


Please use CastSupport.cast

BTW, is it elegant to initialize a CastSupport (DataSourceAnalysis rule or DataSourceStrategy) here, in which we still need to pass SQLConf?

Here, we can obtain SQLConf directly with sparkSession.sessionState.conf

gatorsmile · 2017-08-16T19:31:18Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/TableReader.scala

@@ -227,7 +228,8 @@ class HadoopTableReader(
      def fillPartitionKeys(rawPartValues: Array[String], row: InternalRow): Unit = {
        partitionKeyAttrs.foreach { case (attr, ordinal) =>
          val partOrdinal = partitionKeys.indexOf(attr)
-          row(ordinal) = Cast(Literal(rawPartValues(partOrdinal)), attr.dataType).eval(null)
+          row(ordinal) = Cast(Literal(rawPartValues(partOrdinal)), attr.dataType,
+            Option(SQLConf.get.sessionLocalTimeZone)).eval(null)


The same here.

We also need a test case for verifying this.

Do you mean a test case for HadoopTableReader? a little confusing

SparkQA · 2017-08-16T21:21:10Z

Test build #80748 has finished for PR 18960 at commit 492b756.

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

gatorsmile · 2017-08-16T23:31:37Z

retest this please

SparkQA · 2017-08-17T01:30:08Z

Test build #80754 has finished for PR 18960 at commit 492b756.

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

SparkQA · 2017-08-17T05:00:30Z

Test build #80760 has finished for PR 18960 at commit a264e3a.

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

gatorsmile · 2017-08-17T05:18:01Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/QueryPartitionSuite.scala

@@ -68,4 +68,25 @@ class QueryPartitionSuite extends QueryTest with SQLTestUtils with TestHiveSingl
      sql("DROP TABLE IF EXISTS createAndInsertTest")
    }
  }
+
+  test("SPARK-21739: Cast expression should initialize timezoneId " +


This test can pass without the change in TableReader.scala. We need another test case.

Oh, it should select the TimestampType column. Thanks for reminder, I will fix it.

gatorsmile · 2017-08-17T05:18:33Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/QueryPartitionSuite.scala

+    checkAnswer(sql("select value from table_with_timestamp_partition " +
+      "where ts = '2010-01-02 00:00:00.000'"), Row(2))
+
+    sql("DROP TABLE IF EXISTS table_with_timestamp_partition")


use WithTable. You can check how we do it in the other test cases

DonnyZone · 2017-08-17T07:53:33Z

Test case updated.

DonnyZone · 2017-08-17T08:01:59Z

Moreover, how about using CastSupport.cast, shall I initialize a DataSourceAnalysis or DataSourceStrategy ?

SparkQA · 2017-08-17T09:39:53Z

Test build #80779 has finished for PR 18960 at commit 5a04ddc.

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

SparkQA · 2017-08-17T14:18:27Z

Test build #80786 has finished for PR 18960 at commit 93581bb.

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

gatorsmile · 2017-08-17T23:32:39Z

Could you check the changes I made below? 081d57a

Thanks!

SparkQA · 2017-08-18T04:18:33Z

Test build #80818 has finished for PR 18960 at commit 5b051dd.

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

gatorsmile · 2017-08-18T05:36:27Z

LGTM

gatorsmile · 2017-08-18T05:38:00Z

Thanks! Merging to master/2.2

…it is called statically to convert something into TimestampType ## What changes were proposed in this pull request? https://issues.apache.org/jira/projects/SPARK/issues/SPARK-21739 This issue is caused by introducing TimeZoneAwareExpression. When the **Cast** expression converts something into TimestampType, it should be resolved with setting `timezoneId`. In general, it is resolved in LogicalPlan phase. However, there are still some places that use Cast expression statically to convert datatypes without setting `timezoneId`. In such cases, `NoSuchElementException: None.get` will be thrown for TimestampType. This PR is proposed to fix the issue. We have checked the whole project and found two such usages(i.e., in`TableReader` and `HiveTableScanExec`). ## How was this patch tested? unit test Author: donnyzone <wellfengzhu@gmail.com> Closes #18960 from DonnyZone/spark-21739. (cherry picked from commit 310454b) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

…it is called statically to convert something into TimestampType ## What changes were proposed in this pull request? https://issues.apache.org/jira/projects/SPARK/issues/SPARK-21739 This issue is caused by introducing TimeZoneAwareExpression. When the **Cast** expression converts something into TimestampType, it should be resolved with setting `timezoneId`. In general, it is resolved in LogicalPlan phase. However, there are still some places that use Cast expression statically to convert datatypes without setting `timezoneId`. In such cases, `NoSuchElementException: None.get` will be thrown for TimestampType. This PR is proposed to fix the issue. We have checked the whole project and found two such usages(i.e., in`TableReader` and `HiveTableScanExec`). ## How was this patch tested? unit test Author: donnyzone <wellfengzhu@gmail.com> Closes apache#18960 from DonnyZone/spark-21739. (cherry picked from commit 310454b) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

DonnyZone added 3 commits August 16, 2017 15:53

spark-21739

2537c1e

fix code style

86331e3

correct answer

492b756

gatorsmile reviewed Aug 16, 2017

View reviewed changes

Obtain SQLConf from sparksession

a264e3a

gatorsmile reviewed Aug 17, 2017

View reviewed changes

fix test case queries

5a04ddc

remove a blank line

93581bb

refractor implementation

5b051dd

asfgit closed this in 310454b Aug 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21739][SQL]Cast expression should initialize timezoneId when it is called statically to convert something into TimestampType #18960

[SPARK-21739][SQL]Cast expression should initialize timezoneId when it is called statically to convert something into TimestampType #18960

DonnyZone commented Aug 16, 2017

DonnyZone commented Aug 16, 2017

gatorsmile commented Aug 16, 2017

gatorsmile commented Aug 16, 2017

gatorsmile Aug 16, 2017

DonnyZone Aug 17, 2017

DonnyZone Aug 17, 2017

gatorsmile Aug 16, 2017

gatorsmile Aug 16, 2017

DonnyZone Aug 17, 2017

SparkQA commented Aug 16, 2017

gatorsmile commented Aug 16, 2017

SparkQA commented Aug 17, 2017

SparkQA commented Aug 17, 2017

gatorsmile Aug 17, 2017

DonnyZone Aug 17, 2017

gatorsmile Aug 17, 2017

DonnyZone commented Aug 17, 2017

DonnyZone commented Aug 17, 2017

SparkQA commented Aug 17, 2017

SparkQA commented Aug 17, 2017

gatorsmile commented Aug 17, 2017 •

edited

Loading

SparkQA commented Aug 18, 2017

gatorsmile commented Aug 18, 2017

gatorsmile commented Aug 18, 2017

[SPARK-21739][SQL]Cast expression should initialize timezoneId when it is called statically to convert something into TimestampType #18960

[SPARK-21739][SQL]Cast expression should initialize timezoneId when it is called statically to convert something into TimestampType #18960

Conversation

DonnyZone commented Aug 16, 2017

What changes were proposed in this pull request?

How was this patch tested?

DonnyZone commented Aug 16, 2017

gatorsmile commented Aug 16, 2017

gatorsmile commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 16, 2017

gatorsmile commented Aug 16, 2017

SparkQA commented Aug 17, 2017

SparkQA commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonnyZone commented Aug 17, 2017

DonnyZone commented Aug 17, 2017

SparkQA commented Aug 17, 2017

SparkQA commented Aug 17, 2017

gatorsmile commented Aug 17, 2017 • edited Loading

SparkQA commented Aug 18, 2017

gatorsmile commented Aug 18, 2017

gatorsmile commented Aug 18, 2017

gatorsmile commented Aug 17, 2017 •

edited

Loading