[SPARK-3173][SQL] Timestamp support in the parser

[byF]

If you have a table with TIMESTAMP column, that column can't be used in WHERE clause properly - it is not evaluated properly. More

Motivation: http://www.aproint.com/aggregation-with-spark-sql/

• modify SqlParser so it supports casting to TIMESTAMP (workaround for item 2)
• the string literal should be converted into Timestamp if the column is Timestamp.

[AmplabJenkins]

Can one of the admins verify this patch?

[chuxi]

if add it in Filter part (where ~~), maybe you need to modify the logic part of EqualTo and c2 functions in file predicates.scala & Expression.scala

[byF]

dataType parser is used only in for a CAST expression; I checked the logic of EqualTo and c2, it works out of box, f.e.:

WHERE timestamp >= CAST('2012-07-16 00:00:00' AS TIMESTAMP)
             AND timestamp <= CAST('2012-07-16 01:00:00' AS TIMESTAMP)

Can you think of any corner cases I should test?

[chuxi]

I think CAST is the better choice(Compared with the NO CAST method). It is implemented in the

case class Cast(child: Expression, dataType: DataType) extends UnaryExpression

with a lot of dataTypes, including timestampType

Besides, if you want to implement "modify Comparable expression evaluation so the the explicit casting is not necessary", you need to tell a String apart whether it is a TimeStamp format. And then modify the last line code:

protected lazy val literal: Parser[Literal] =
    numericLit ^^ {
      case i if i.toLong > Int.MaxValue => Literal(i.toLong)
      case i => Literal(i.toInt)
    } |
    NULL ^^^ Literal(null, NullType) |
    floatLit ^^ {case f => Literal(f.toDouble) } |
    stringLit ^^ {case s => Literal(s, StringType) }

Literal supports TimeStamp too.

stringLit ^^ { case s => try Literal(Timestamp.valueOf(s), Timestamp) catch Literal(s, StringType) }

[chutium]

is this PR for SPARK-3065 or SPARK-3173 ?

[marmbrus]

Thanks for working on this!

Can you modify the PR title to point to SPARK-3173 instead? Also please add a test case in SQLQuerySuite.

@chuxi I believe this PR is just extending the parser you can can cast string to timestamps. I do not think that we automatically want to do this anytime a string could be interpreted as a timestamp.

[byF]

I'll add the test case once I get to my office today.

Exactly, the parser supported only CAST(... AS STRING). That's the problem for me. However, it would be nice if I could do:

WHERE timestamp >= '2012-07-16 00:00:00'
             AND timestamp <= '2012-07-16 01:00:00'

instead of

WHERE timestamp >= CAST('2012-07-16 00:00:00' AS TIMESTAMP)
             AND timestamp <= CAST('2012-07-16 01:00:00' AS TIMESTAMP)

Parsing this kind of expressions happens in SqlParser.comparisonExpression; I consider modifying predicates.scala in a way that would say:

If a queried expression has the TimestampType, replace the value expression with Cast.

The way the rest of the code is written it should probably work out of box. What do you think?

[marmbrus]

Instead of doing this by modifying the predicate logic, I think that we should just add a rule in HiveTypeCoercion that finds BinaryPredicates where one side is a string and the other side is a timestamp and adds a Cast. Perhaps in PromoteStrings?

[chuxi]

@marmbrus, I agree with you. Use CAST and so we can avoid some tough design. I know little about hive and do you mean in HiveTypeCoercion there is a CAST problem? I will try to follow the code.

[byF]

@marmbrus PromoteStrings makes perfect sense; I've got troubles running tests on my machine, I'll push the fix once I make the test work.

[marmbrus]

Awesome! If you get stuck with tests feel free to push broken code and ping me.

[byF]

I had a problem with running the tests, eventually figured it out

The tests added and the literal conversion works.

[marmbrus]

ok to test

[SparkQA]

QA tests have started for PR 2084 at commit 491dfcf.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2084 at commit 491dfcf.

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

[chenghao-intel]

What if left is StringType and right is the TimestampType?

[byF]

Fixed in 47b27b4

[SparkQA]

QA tests have started for PR 2084 at commit 47b27b4.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2084 at commit 47b27b4.

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

[byF]

@marmbrus so the test fails in the jenkins build, however it passes okay on my machine (in Intellij). Any idea what's the reason for that?

[marmbrus]

Could it be something with timezones? In the hive test we fix the default timezones to prevent problems.

[SparkQA]

QA tests have started for PR 2084 at commit 442b59d.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2084 at commit 442b59d.

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

[marmbrus]

Thanks! I've merged this into master.