[SPARK-29838][SQL] PostgreSQL dialect: cast to timestamp by amanomer · Pull Request #26472 · apache/spark

amanomer · 2019-11-11T17:50:57Z

What changes were proposed in this pull request?

To make SparkSQL's cast as timestamp behavior consistent with PostgreSQL when spark.sql.dialect is configured as PostgreSQL.

Why are the changes needed?

SparkSQL and PostgreSQL have a lot different cast behavior between types by default. We should make SparkSQL's cast behavior be consistent with PostgreSQL when spark.sql.dialect is configured as PostgreSQL.

Does this PR introduce any user-facing change?

Yes. If user switches to PostgreSQL dialect now, they will

get an AnalysisException when they input any data type except StringType and DateType.

How was this patch tested?

Added test cases.

amanomer · 2019-11-14T12:28:08Z

cc @maropu @cloud-fan

amanomer · 2019-11-15T03:58:49Z

@maropu updated as per review comments. withTimeZone() is being repeated because cannot override it in abstract class PostgreCastBase.

amanomer · 2019-11-15T13:29:29Z

@maropu Can you review the latest changes?

amanomer · 2019-11-15T15:28:37Z

@maropu Kindly review. I have updated as per your suggestion.

Ngone51 · 2019-11-16T15:21:48Z

+ if (conf.usePostgreSQLDialect) {
+ plan.transformExpressions {
+ case Cast(child, dataType, timeZoneId)
+ if dataType == TimestampType =>


We can leave cast timestamp to timestamp case to Optimizer to do optimization.

Oh, sorry. Here, I mean that we should change the if condition to: if child.dataType != TimestampType && dataType == TimestampType =>. Because Optimizer, currently, can not optimize Pg cast.

Got this. I will update this.

Ngone51 · 2019-11-16T15:32:26Z

- override def sql: String = s"CAST(${child.sql} AS ${dataType.sql})"
+ override def castToTimestamp(from: DataType): Any => Any = from match {
+ case StringType =>
+ buildCast[UTF8String](_, utfs => DateTimeUtils.stringToTimestamp(utfs, zoneId)


I believe that postgre could correctly parse string 19700101, 1970/01/01, January 1 04:05:06 1970 PST while spark can't. So, I think that we may also need to support it in PostgreCastToTimestamp.

Thanks for your suggestion. I will check this.

postgres# select cast('19700101' as timestamp); 01.01.1970 00:00:00
postgres# select cast('1970/01/01' as timestamp); 01.01.1970 00:00:00
postgres# select cast('January 1 04:05:06 1970 PST' as timestamp); 01.01.1970 04:05:06
Spark results with NULL for all of them.

@maropu kindly review latest changes and give your feedback on supporting above queries.
Do we need to support them in this PR? If yes, we need to list all formats for timestamps which postgres supports but spark don't.

I personally think that the support above is not a main issue of this pr, so better to separate the two work: the timestamp cast support and the timestamp format support for the pg dialect.

amanomer · 2019-11-19T06:41:15Z

cc @cloud-fan @srowen

amanomer · 2019-11-20T04:50:16Z

@maropu Kindly review this. Other PRs are depending on this.

maropu · 2019-11-20T11:38:46Z

+ val expectedResult = s"cannot cast type ${value.getClass} to timestamp"
+ assert(actualResult.contains(expectedResult))
+ }
+ }


Can you move these tests to SQLQueryTestSuite,e.g., input/postgreSQL/cast.sql?

I have moved these test cases. cast.sql.out needs to be updated.

Need to delete this test case.

maropu · 2019-11-20T11:49:12Z

+}

- override def dataType: DataType = BooleanType
+case class PostgreCastToTimestamp(child: Expression, timeZoneId: Option[String])


btw, we need to define a new rule and a new cast expr for each Pg cast pattern? I mean we cannot define all the Pg cast patterns in a single rule and a cast expr? cc: @cloud-fan @Ngone51

I think we can and should combine them into a single one(both rule and expression) when more types get in. Just like the original Cast does. But I'm not sure where shall we start. Maybe, this one ?

yea, I personally think so. @cloud-fan

amanomer · 2019-11-21T08:57:07Z

Jenkins, test this please.

amanomer · 2019-11-25T04:43:57Z

cc @maropu @Ngone51

Ngone51 · 2019-11-25T08:47:12Z

Hi @amanomer , have you addressed #26472 (comment) ?

amanomer · 2019-11-26T05:46:27Z

@Ngone51 I have updated this PR as per your reviews. Kindly review.

cc @maropu @cloud-fan

maropu · 2019-11-26T13:09:51Z

Can you merge the current two rule into one as mentioned above? #26472 (comment)

amanomer · 2019-11-26T17:32:06Z

Yes. I will merge these two rules that are, CastToTimestamp and CastToBoolean

…eCast

amanomer · 2019-11-29T04:14:49Z

@maropu Please check latest changes. Thanks

maropu · 2019-11-29T04:24:43Z

IIUC we the community are planning to hold off the Pg dialect PRs for a while. (until 3.0 released?). plz check http://apache-spark-developers-list.1001551.n3.nabble.com/DISCUSS-PostgreSQL-dialect-td28417.html cc: @cloud-fan

dongjoon-hyun · 2019-12-15T02:33:28Z

Thank you for contribution, @amanomer . As you know, unfortunately, we decided to remove PostgreSQL dialect via SPARK-30125 (#26763). Sorry about that. I'll close this PR, too.

dongjoon-hyun added the SQL label Nov 11, 2019

amanomer changed the title ~~[WIP][SPARK-29838][SQL] PostgreSQL dialect: cast to timestamp~~[SPARK-29838][SQL] PostgreSQL dialect: cast to timestampNov 14, 2019

maropu reviewed Nov 14, 2019
View reviewed changes

Comment thread...main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToTimestamp.scala Outdated

maropu reviewed Nov 14, 2019
View reviewed changes

Comment thread...main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToTimestamp.scala Outdated

amanomer force-pushed the 29838 branch from 72355dc to a353311Compare November 14, 2019 06:20

amanomer added 4 commits November 14, 2019 17:00

Initial commit
d705adc

Added test cases
8b56ae9

Abstract class PostgreCastBase
00ffde3

Added PostgreCastBase
6a93060

amanomer force-pushed the 29838 branch from 1ef5b1d to 6a93060Compare November 14, 2019 12:22

maropu reviewed Nov 15, 2019
View reviewed changes

Comment thread...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastBase.scala Outdated

maropu reviewed Nov 15, 2019
View reviewed changes

Comment thread...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastBase.scala Outdated

maropu reviewed Nov 15, 2019
View reviewed changes

Comment thread...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastBase.scala

This was referenced Nov 15, 2019
[WIP][SPARK-29845][SQL]PostgreSQL dialect: cast to decimal #26518
Closed
[SPARK-29840][SQL] PostgreSQL dialect: cast to integer #26481
Closed

amanomer requested a review from maropu November 15, 2019 04:55

maropu reviewed Nov 15, 2019
View reviewed changes

Comment thread...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastBase.scala Outdated

Updated PostgreCastBase
1024d37

amanomer force-pushed the 29838 branch from 58ad9d7 to 1024d37Compare November 15, 2019 15:26

Ngone51 reviewed Nov 16, 2019
View reviewed changes

amanomer requested a review from maropu November 20, 2019 05:56

maropu reviewed Nov 20, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/PostgreSQLDialect.scala Outdated

maropu reviewed Nov 20, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/PostgreSQLDialect.scala Outdated

maropu reviewed Nov 20, 2019
View reviewed changes

Moved test cases to postgreSQL/cast.sql
700143d

Test cases updated
111d673

amanomer requested a review from maropu November 23, 2019 02:53

Ngone51 reviewed Nov 25, 2019
View reviewed changes

Comment thread...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/CastSuite.scala Outdated

1 -> 1.toInt ; child.dataType != TimestampType
4fc3d71

Rules PostgreCastToBoolean, PostgreCastToTimestamp combined in Postgr…
08c71f1
…eCast

dongjoon-hyun closed this Dec 15, 2019

Uh oh!

Conversation

amanomer commented Nov 11, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

Uh oh!

amanomer commented Nov 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amanomer commented Nov 15, 2019

Uh oh!

amanomer commented Nov 15, 2019

Uh oh!

Uh oh!

amanomer commented Nov 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ngone51Nov 22, 2019• 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!

amanomer commented Nov 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amanomer commented Nov 20, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

amanomer commented Nov 21, 2019

Uh oh!

amanomer commented Nov 25, 2019

Uh oh!

Ngone51 commented Nov 25, 2019

Uh oh!

Uh oh!

amanomer commented Nov 26, 2019

Uh oh!

maropu commented Nov 26, 2019

Uh oh!

amanomer commented Nov 26, 2019

Uh oh!

amanomer commented Nov 29, 2019

Uh oh!

maropu commented Nov 29, 2019

Uh oh!

dongjoon-hyun commented Dec 15, 2019

Uh oh!

Reviewers

Ngone51Nov 22, 2019•
edited
Loading

amanomer commented Nov 19, 2019•
edited
Loading

amanomer commented Nov 20, 2019•
edited
Loading