[SPARK-8186] [SPARK-8187] [SQL] datetime function: date_add, date_sub #6782

adrian-wang · 2015-06-12T06:55:35Z

No description provided.

SparkQA · 2015-06-12T07:45:41Z

Test build #34758 has finished for PR 6782 at commit 90b5d2e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class DateAdd(left: Expression, right: Expression)
- case class DateSub(left: Expression, right: Expression)
- case class DateDiff(left: Expression, right: Expression)

marmbrus · 2015-06-12T08:30:35Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DatetimeFunctionsSuite.scala

+  test("date_add") {
+    checkEvaluation(
+      DateAdd(Literal(Date.valueOf("2016-02-28")), Literal(1)),
+      DateUtils.fromJavaDate(Date.valueOf("2016-02-29")), create_row(null))


Nit: Could we just pass an Row.empty instead of constructing a new one? Could that just be a default argument to checkEvaluation?

I think it will be a good idea.

adrian-wang · 2015-06-18T01:02:26Z

@chenghao-intel

SparkQA · 2015-06-18T01:10:57Z

Test build #35068 has finished for PR 6782 at commit c0703ea.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class DateAdd(startDate: Expression, days: Expression) extends Expression
- case class DateSub(startDate: Expression, days: Expression) extends Expression

SparkQA · 2015-06-18T03:20:43Z

Test build #35072 has finished for PR 6782 at commit a5522e4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class DateAdd(startDate: Expression, days: Expression) extends Expression
- case class DateSub(startDate: Expression, days: Expression) extends Expression

chenghao-intel · 2015-06-18T04:54:33Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

@@ -946,6 +946,38 @@ object functions {
  def cosh(columnName: String): Column = cosh(Column(columnName))

  /**
+   * Adds a number of days to startDate, given values for startDate and days
+   *
+   * @group datetime_funcs


Add datetime_funcs in the beginning of the functions.scala also.

chenghao-intel · 2015-06-18T05:01:44Z

LGTM except some minor issues.

SparkQA · 2015-06-18T07:20:59Z

Test build #35091 has finished for PR 6782 at commit 81628b5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class DateAdd(startDate: Expression, days: Expression) extends Expression
- case class DateSub(startDate: Expression, days: Expression) extends Expression

adrian-wang · 2015-06-18T07:25:39Z

cc @rxin
Can you review this? This pr is the first PR for datetime functions and it will created several files in upstream, hence all other datetime functions depend on the creation of these files.

rxin · 2015-06-18T07:31:36Z

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

+import org.apache.spark.sql.types._
+import org.apache.spark.unsafe.types.UTF8String
+
+case class DateAdd(startDate: Expression, days: Expression) extends Expression {


i think it'd be helpful to define the semantics here (basically what eval is doing)

rxin · 2015-06-18T07:32:29Z

Can you add the code gen version? Otherwise these expressions are pretty slow with so many branches ...

chenghao-intel · 2015-06-18T07:46:51Z

Yes,we need to move out the dynamic dataType comparison from the eval.

SparkQA · 2015-06-18T10:10:14Z

Test build #35113 has finished for PR 6782 at commit eebf41d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class DateAdd(startDate: Expression, days: Expression) extends Expression
- case class DateSub(startDate: Expression, days: Expression) extends Expression

marmbrus · 2015-06-18T20:02:16Z

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

+    }
+  }
+
+  override def dataType: DataType = StringType


Why does this return a string instead of a date?

hive doc says it returns a stirng

Should we follow hive so strictly? It makes more sense for me to have a DateType return type, so that we can apply date time functions on the result without casting to String and back again.

Actually hive support these functions before there are date type support. to be compatible with older version(older than 0.12.0, where date type are first supported) and applications based on those, keeping the same interface is helpful.

In general though we cast back to a string whenever you need to. From an efficiency stand point it seems much better to keep it a date.

/cc @rxin

+1 for DateType, unless we committed to 100% compatible with Hive.

adrian-wang · 2015-06-19T06:57:48Z

ping @rxin @marmbrus

rxin · 2015-06-19T07:09:00Z

@cloud-fan can you add this to your review queue?

cloud-fan · 2015-06-19T07:25:52Z

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

+      TypeCheckResult.TypeCheckFailure(
+        s"type of days expression in DateAdd should be int, not ${days.dataType}.")
+    } else {
+      TypeCheckResult.TypeCheckSuccess


I think we should only allow DateType and NullType, and add rules in HiveTypeCoercion to cast other supported types to DateType.

I don't like that way, as we will need to add a rule to cast both timestamp type string and date type string intto DateType. That's very intuitive.

After #6759 gets in, I probably will refract DateUtils to offer some more direct interfaces, but let's skip it now to avoid huge conflicts.

I agreed with @cloud-fan, that also helps to simplify the implementations.

To do cast in HiveTypeCoercion for about 30 different Datetime Functions? That would be nasty to maintain.

@adrian-wang Thanks for the confirm. From the HIVE test [1], I see date_add works with tinyint, smallint, int, but there is no case for "2010-01-01 01:02:03", is it not tested side behavior?

[1] https://github.com/apache/hive/blob/master/ql/src/test/queries/clientpositive/udf_date_add.q

@davies you can find more test in
https://github.com/apache/hive/blob/master/ql/src/test/queries/clientpositive/date_udf.q

for string type of time, the tests are in
https://github.com/apache/hive/blob/master/ql/src/test/queries/clientpositive/timestamp_udf.q

I see. I checked MySQL, it can cast '2012-01-01 12:21:12' to Date:

mysql> select cast('2012-01-01 12:21:12' as date); +-------------------------------------+ | cast('2012-01-01 12:21:12' as date) | +-------------------------------------+ | 2012-01-01 | +-------------------------------------+ 1 row in set (0.00 sec)

This sounds reasonable to me, but not sure making it different than Hive will break real workload or not.

Because of all kinds of wired behavior in Hive, we cannot have 100% compatibility with Hive already, see many hive tests are skipped. @marmbrus, what do yo think?

@davies agree that 100% compatibility is not a goal and mysql's behavior seems reasonable here. It would be good though to more holistically design our data handling API and make sure we aren't missing anything. For example, today we can handle queries like: WHERE timestamp > "2014-01" but only because we convert the timestamp to a string and the strings compare lexicographically as you would hope.

However, this feels like a hack to me and it would be much more efficient if we could avoid creating strings for every date and instead could covert "2014-01" into an Int internally. How does mysql handle partial dates like this?

cloud-fan · 2015-06-20T14:26:55Z

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

+        val resultDays = startDate.dataType match {
+          case StringType =>
+            DateUtils.millisToDays(DateUtils.stringToTime(
+              start.asInstanceOf[UTF8String].toString).getTime) + offset


In Cast we using DateUtils.fromJavaDate(Date.valueOf(s.toString)) to cast string to date. Which way is more efficient?

Ah I see, you handle both date and timestamp string here.

cloud-fan · 2015-06-20T14:30:31Z

DateAdd and DateSub are very similar , and logically we can just transform from one to the other by changing days to -days. Can we combine them to reduce code size?

chenghao-intel · 2015-06-24T02:36:36Z

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

+
+  override def dataType: DataType = StringType
+
+  override def toString: String = s"DateAdd($startDate, $days)"


Let's remove this as the case class will handle that pretty well.

chenghao-intel · 2015-06-24T02:37:50Z

Can you remove the WIP from the title?

davies · 2015-06-24T04:44:52Z

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

+      TypeCheckResult.TypeCheckFailure(
+        s"type of startdate expression in DateAdd should be string/timestamp/date," +
+          s" not ${startDate.dataType}")
+    } else if (days.dataType != IntegerType && days.dataType != NullType) {


Could we also support integral types here for days?

For example, we always infer integer from Python and Json as LongType.

SparkQA · 2015-06-24T06:41:16Z

Test build #35646 has finished for PR 6782 at commit 0944f03.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class DateAdd(startDate: Expression, days: Expression) extends Expression
- case class DateSub(startDate: Expression, days: Expression) extends Expression

SparkQA · 2015-06-24T08:52:37Z

Test build #35655 has finished for PR 6782 at commit 95b3b58.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class DateAdd(startDate: Expression, days: Expression) extends Expression
- case class DateSub(startDate: Expression, days: Expression) extends Expression

rxin · 2015-06-30T00:38:26Z

@adrian-wang I thought about this more -- I think we need to go back to the drawing board to specify the behavior of various date functions, return types, before we can merge these expressions.

Some open questions include:

what data type should these expressions accept?
what data type should these expressions return?
should we support intervals?
...

I will make sure the design happens next week. Meantime, can you close the expressions related to dates? We can reopen them once we flush out the design details.

adrian-wang · 2015-06-30T04:19:52Z

OK, I'll close these for now. Could you assign them all to me in case duplicated work here?

adrian-wang changed the title ~~[SPARK-8185] [SPARK-8186] [SPARK-8187] [SQL] datetime function: date_add, date_sub, datediff~~ [SPARK-8185] [SPARK-8186] [SPARK-8187] [SQL] [WIP] datetime function: date_add, date_sub, datediff Jun 12, 2015

marmbrus reviewed Jun 12, 2015
View reviewed changes

adrian-wang changed the title ~~[SPARK-8185] [SPARK-8186] [SPARK-8187] [SQL] [WIP] datetime function: date_add, date_sub, datediff~~ [SPARK-8186] [SPARK-8187] [SQL] [WIP] datetime function: date_add, date_sub Jun 18, 2015

adrian-wang force-pushed the udfdatecal branch from c0703ea to a5522e4 Compare June 18, 2015 01:22

chenghao-intel reviewed Jun 18, 2015
View reviewed changes

rxin reviewed Jun 18, 2015
View reviewed changes

marmbrus reviewed Jun 18, 2015
View reviewed changes

cloud-fan reviewed Jun 19, 2015
View reviewed changes

adrian-wang changed the title ~~[SPARK-8186] [SPARK-8187] [SQL] [WIP] datetime function: date_add, date_sub~~ [SPARK-8186] [SPARK-8187] [SQL] datetime function: date_add, date_sub Jun 19, 2015

cloud-fan reviewed Jun 20, 2015
View reviewed changes

chenghao-intel reviewed Jun 24, 2015
View reviewed changes

adrian-wang added 2 commits June 23, 2015 20:14

datetime function: date_add, date_sub, datediff

5a50edc

remove datediff because datediff use GTC time, do that in another pr

a6243f7

adrian-wang added 3 commits June 23, 2015 20:14

fix rebase

ea9fde0

chenghao's comments

f89bf4a

add codegen

2ac75f7

davies reviewed Jun 24, 2015
View reviewed changes

rebase and several update

0944f03

adrian-wang force-pushed the udfdatecal branch from eebf41d to 0944f03 Compare June 24, 2015 06:10

fix test

95b3b58

adrian-wang closed this Jun 30, 2015

adrian-wang mentioned this pull request Jul 22, 2015

[SPARK-8186][SPARK-8187][SPARK-8194][SPARK-8198][SQL] functions: date_add, date_sub, add_months, months_between, time-interval calculation #7589

Closed


		override def dataType: DataType = StringType

		override def toString: String = s"DateAdd($startDate, $days)"

[SPARK-8186] [SPARK-8187] [SQL] datetime function: date_add, date_sub #6782

[SPARK-8186] [SPARK-8187] [SQL] datetime function: date_add, date_sub #6782

Conversation

adrian-wang commented Jun 12, 2015

SparkQA commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-wang commented Jun 18, 2015

SparkQA commented Jun 18, 2015

SparkQA commented Jun 18, 2015

Choose a reason for hiding this comment

chenghao-intel commented Jun 18, 2015

SparkQA commented Jun 18, 2015

adrian-wang commented Jun 18, 2015

Choose a reason for hiding this comment

rxin commented Jun 18, 2015

chenghao-intel commented Jun 18, 2015

SparkQA commented Jun 18, 2015

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

adrian-wang commented Jun 19, 2015

rxin commented Jun 19, 2015

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

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

cloud-fan commented Jun 20, 2015

Choose a reason for hiding this comment

chenghao-intel commented Jun 24, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 24, 2015

SparkQA commented Jun 24, 2015

rxin commented Jun 30, 2015

adrian-wang commented Jun 30, 2015