Stop parsing special dates for Spark 3.2+ #3439

sperlingxx · 2021-09-10T06:00:42Z

Signed-off-by: sperlingxx [email protected]

Current PR also fixes cases on casting string to date/timestamp in CastOpSuite, which introduce special dates. However, these cases won't be entirely fixed until we fix #3382 through supporting the full range date/timestamp on GPU as what SPARK-35780 does.

Signed-off-by: sperlingxx <[email protected]>

sperlingxx · 2021-09-10T06:04:40Z

build

firestarman · 2021-09-10T10:00:35Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala

      // handle special dates like "epoch", "now", etc.
-      val finalResult = specialDates.foldLeft(converted)((prev, specialDate) =>
-        specialTimestampOr(sanitizedInput, specialDate._1, specialDate._2, prev))
+      val finalResult = withResource(daysEqual(sanitizedInput, DateUtils.EPOCH)) { isEpoch =>


NIT: It would be good to have this boilerplate code into a method.

firestarman · 2021-09-10T10:02:51Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/datetimeExpressions.scala

+  def daysScalarDays(name: String): Scalar = ShimLoader.getSparkVersion match {
+    // In Spark 3.2, special datetime values such as `epoch`, `today`, `yesterday`, `tomorrow`,
+    // and `now` are supported in typed literals only
+    case version if version >= "3.2" =>


It would be better to add shims for this.

If we do keep the version check here (which I am ok with personally because it reduces complexity in the shim layers) then we should use more robust logic for the version check. We have some code in the test suite that could be moved elsewhere.

https://github.com/NVIDIA/spark-rapids/blob/branch-21.10/tests/src/test/scala/com/nvidia/spark/rapids/SparkQueryCompareTestSuite.scala#L1848-L1849

+1 to keep the version check. I would suggest to move @andygrove's implementation to ShimVerion to make shimversions comparable

The reason to use a shim over a version check is mostly databricks and other vendors that pull back "fixes" not directly related to version numbers. I doubt it will happen in this case, but similar things have happened in the past.

I think it's something we will find out via the test being broken on databricks. If we go with the ShimVersion comparison implementation, the fix is either an additional condition on databricks shim version compare or it's more involved at which point we will have to add a new shim method.

I replaced the version dispatch with a shim method.

revans2 · 2021-09-10T13:20:04Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala

      // handle special dates like "epoch", "now", etc.
-      specialDates.foldLeft(converted)((prev, specialDate) =>
-        specialDateOr(sanitizedInput, specialDate._1, specialDate._2, prev))
+      withResource(daysEqual(sanitizedInput, DateUtils.EPOCH)) { isEpoch =>


This change feels like a step backwards. Before with the folding the maximum extra memory needed on the GPU was a boolean column, and the new output column. With this change we now have 5 boolean columns and 5 temporary output columns.

Yes, it was not optimal. I reworked this part, cleaning up temporary GPU resources as early as possible.

andygrove · 2021-09-10T14:27:03Z

tests/src/test/scala/com/nvidia/spark/rapids/ParseDateTimeSuite.scala

-    val startTimeSeconds = System.currentTimeMillis()/1000L
    val cpuNowSeconds = withCpuSparkSession(now).collect().head.toSeq(1).asInstanceOf[Long]
    val gpuNowSeconds = withGpuSparkSession(now).collect().head.toSeq(1).asInstanceOf[Long]
-    assert(cpuNowSeconds >= startTimeSeconds)


What is the reason for removing these assertions? Are they no longer valid?

Yes, under spark 3.2+, the result will be zero instead of current time, since NOW is not longer being parsed in 3.2.

Then have the test check the version. and have it check that now is replaced with a 0 instead. Or just have the test skip entirely if it is 3.2+

…pecialDates_320

Signed-off-by: sperlingxx <[email protected]>

…pecialDates_320

sperlingxx · 2021-09-14T08:07:03Z

build

…pecialDates_320

sperlingxx · 2021-09-15T03:27:28Z

build

pxLi · 2021-09-15T03:44:22Z

the CI has been assigned to non-reserved instances which failed PVC driver. I will report to blossom

pxLi · 2021-09-15T03:44:29Z

build

andygrove · 2021-09-15T13:42:42Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala

+      // `converted` will be closed in replaceSpecialDates. We wrap it with closeOnExcept in case
+      // of exception before replaceSpecialDates.
+      val finalResult = closeOnExcept(converted) { timeStampVector =>
+        val specialDates = Seq(DateUtils.EPOCH, DateUtils.NOW, DateUtils.TODAY,


It seems expensive to replace the special dates with null with Spark 3.2 rather than just skip handling them at all. They will already be ignored by is_timestamp.

I will pull this PR locally today and experiment with it and come back with more detailed suggestions.

andygrove · 2021-09-15T18:23:24Z

I looked at how I would approach this and I don't think there is a need to make any shim changes here. Spark 3.2 doesn't support the special dates so it seems confusing to add related logic there. There are also no changes in Spark APIs that would cause us to need shims.

I think this issue can be resolved more simply with some version checks in a couple of places:

In DateUtils, where we return maps of special dates to literal values we can return empty maps with Spark 3.2:

def specialDatesDays: Map[String, Int] = if (spark320orLater) {
  Map.empty
} else {
  val today = currentDate()
  Map(
    EPOCH -> 0,
    NOW -> today,
    TODAY -> today,
    YESTERDAY -> (today - 1),
    TOMORROW -> (today + 1)
  )
}

(and the same for specialDatesSeconds and specialDatesMicros).

Then in org.apache.spark.sql.rapids.GpuToTimestamp#parseStringAsTimestamp we can do:

if (spark320orLater) {
  withResource(isTimestamp(lhs.getBase, sparkFormat, strfFormat)) { isTimestamp =>
    withResource(asTimestamp(lhs.getBase, strfFormat)) { converted =>
      withResource(Scalar.fromNull(dtype)) { nullValue =>
        isTimestamp.ifElse(converted, nullValue)
      }
    }
  }
} else {
 // original complex logic that handles special dates
}

…pecialDates_320

Signed-off-by: sperlingxx <[email protected]>

sperlingxx · 2021-09-16T12:56:58Z

I looked at how I would approach this and I don't think there is a need to make any shim changes here. Spark 3.2 doesn't support the special dates so it seems confusing to add related logic there. There are also no changes in Spark APIs that would cause us to need shims.

I think this issue can be resolved more simply with some version checks in a couple of places:

In DateUtils, where we return maps of special dates to literal values we can return empty maps with Spark 3.2:
def specialDatesDays: Map[String, Int] = if (spark320orLater) {
  Map.empty
} else {
  val today = currentDate()
  Map(
    EPOCH -> 0,
    NOW -> today,
    TODAY -> today,
    YESTERDAY -> (today - 1),
    TOMORROW -> (today + 1)
  )
}
(and the same for specialDatesSeconds and specialDatesMicros).

Then in org.apache.spark.sql.rapids.GpuToTimestamp#parseStringAsTimestamp we can do:
if (spark320orLater) {
  withResource(isTimestamp(lhs.getBase, sparkFormat, strfFormat)) { isTimestamp =>
    withResource(asTimestamp(lhs.getBase, strfFormat)) { converted =>
      withResource(Scalar.fromNull(dtype)) { nullValue =>
        isTimestamp.ifElse(converted, nullValue)
      }
    }
  }
} else {
 // original complex logic that handles special dates
}

I fully agree on this approach, and I've made corresponding changes. Thanks for such a detailed explanation!

sperlingxx · 2021-09-16T12:57:05Z

build

sql-plugin/src/main/scala/com/nvidia/spark/rapids/DateUtils.scala

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/datetimeExpressions.scala

revans2 · 2021-09-16T13:30:06Z

tests/src/test/scala/com/nvidia/spark/rapids/ParseDateTimeSuite.scala

-    val startTimeSeconds = System.currentTimeMillis()/1000L
    val cpuNowSeconds = withCpuSparkSession(now).collect().head.toSeq(1).asInstanceOf[Long]
    val gpuNowSeconds = withGpuSparkSession(now).collect().head.toSeq(1).asInstanceOf[Long]
-    assert(cpuNowSeconds >= startTimeSeconds)


Then have the test check the version. and have it check that now is replaced with a 0 instead. Or just have the test skip entirely if it is 3.2+

…pecialDates_320

Signed-off-by: sperlingxx <[email protected]>

sperlingxx · 2021-09-17T06:18:08Z

build

Signed-off-by: sperlingxx <[email protected]>

sperlingxx · 2021-09-17T06:33:01Z

build

tgravescs · 2021-09-17T14:42:24Z

I pulled this PR and we still have CastOpSuite test failures, was this intended to handle all of those? its referenced in description so I was assuming so

sperlingxx · 2021-09-17T15:23:08Z

I pulled this PR and we still have CastOpSuite test failures, was this intended to handle all of those? its referenced in description so I was assuming so

Hi @tgravescs, this PR can not fix some failed cases in CastOpSuite which are due to #3382. I am going to simply disable those cases for Spark 3.2+ after finishing this PR. It will be a temporary solution, since we need naive support to fix #3382 completely.

revans2 · 2021-09-17T15:32:11Z

It will be a temporary solution, since we need naive support to fix #3382 completely.

We have to fall back to the CPU for this case unless the user opts into letting us do the cast anyways. Disabling the tests is not a solution.

sperlingxx · 2021-09-17T15:44:15Z

It will be a temporary solution, since we need naive support to fix #3382 completely.

We have to fall back to the CPU for this case unless the user opts into letting us do the cast anyways. Disabling the tests is not a solution.

Oh, I thought we already had corresponding options to disable casting from string to date/timestamp by default. But I just found I was wrong: we only have the config RapidsConf.ENABLE_CAST_STRING_TO_TIMESTAMP. IIUC, we also need to add RapidsConf.ENABLE_CAST_STRING_TO_DATE ?

revans2 · 2021-09-17T15:48:51Z

We should add in RapidsConf.ENABLE_CAST_STRING_TO_DATE and only use it for 3.2.0+. We also need to make it clear in the docs for it that it is only for 3.2.0+ because of 7 digit year support. We should also update the docs for RapidsConf.ENABLE_CAST_STRING_TO_TIMESTAMP to include information about the 7 digit year support in 3.2.0+

revans2

I am not thrilled with VersionUtils moving to the plugin as I explained here. But just to get something shipped I am going to approve this and we can revisit the topic later on.

revans2 · 2021-09-17T17:15:37Z

That said we do need to fix the 7 digit year parsing as a separate issue.

revans2 · 2021-09-17T18:54:27Z

Just FYI I filed #3530 to handle the 7 digit year fallback issue. I will take a crack at it because hopefully I can get it done today.

sperlingxx added 3 commits September 10, 2021 10:55

init

33637dd

Signed-off-by: sperlingxx <[email protected]>

update

000ce39

Signed-off-by: sperlingxx <[email protected]>

update

d87804e

Signed-off-by: sperlingxx <[email protected]>

sperlingxx requested a review from andygrove September 10, 2021 06:01

sperlingxx requested a review from revans2 September 10, 2021 06:05

firestarman reviewed Sep 10, 2021

View reviewed changes

revans2 requested changes Sep 10, 2021

View reviewed changes

andygrove reviewed Sep 10, 2021

View reviewed changes

sameerz added the Spark 3.2+ label Sep 11, 2021

sameerz added this to the Sep 13 - Sep 24 milestone Sep 11, 2021

sameerz assigned sperlingxx Sep 11, 2021

sperlingxx added 3 commits September 13, 2021 18:01

Merge remote-tracking branch 'origin/branch-21.10' into stopTransferS…

ec0f802

…pecialDates_320

refactored

a8fc234

Signed-off-by: sperlingxx <[email protected]>

Merge remote-tracking branch 'origin/branch-21.10' into stopTransferS…

54a0059

…pecialDates_320

sperlingxx requested review from andygrove, firestarman, gerashegalov and revans2 September 14, 2021 08:12

Merge remote-tracking branch 'origin/branch-21.10' into stopTransferS…

926a824

…pecialDates_320

andygrove reviewed Sep 15, 2021

View reviewed changes

sperlingxx added 2 commits September 16, 2021 19:25

Merge remote-tracking branch 'origin/branch-21.10' into stopTransferS…

111620d

…pecialDates_320

fix

fec39d5

Signed-off-by: sperlingxx <[email protected]>

sperlingxx requested a review from andygrove September 16, 2021 12:53

fix

b2f35d3

Signed-off-by: sperlingxx <[email protected]>

revans2 reviewed Sep 16, 2021

View reviewed changes

sperlingxx added 3 commits September 17, 2021 10:28

Merge remote-tracking branch 'origin/branch-21.10' into stopTransferS…

28585b2

…pecialDates_320

update

790eb44

Signed-off-by: sperlingxx <[email protected]>

wrap up

76eaa44

Signed-off-by: sperlingxx <[email protected]>

update

50e85ef

Signed-off-by: sperlingxx <[email protected]>

sperlingxx requested a review from revans2 September 17, 2021 07:58

tgravescs mentioned this pull request Sep 17, 2021

[BUG] Unit test failures in Spark 3.2 shim build #3376

Closed

54 tasks

revans2 approved these changes Sep 17, 2021

View reviewed changes

revans2 merged commit 695f82b into NVIDIA:branch-21.10 Sep 17, 2021

sperlingxx deleted the stopTransferSpecialDates_320 branch December 2, 2021 02:41

Stop parsing special dates for Spark 3.2+ #3439

Stop parsing special dates for Spark 3.2+ #3439

Uh oh!

Conversation

sperlingxx commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sperlingxx commented Sep 10, 2021

Uh oh!

firestarman Sep 10, 2021 • 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!

gerashegalov Sep 10, 2021 • 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!

sperlingxx commented Sep 14, 2021

Uh oh!

sperlingxx commented Sep 15, 2021

Uh oh!

pxLi commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pxLi commented Sep 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Sep 15, 2021

Uh oh!

sperlingxx commented Sep 16, 2021

Uh oh!

sperlingxx commented Sep 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sperlingxx commented Sep 17, 2021

Uh oh!

sperlingxx commented Sep 17, 2021

Uh oh!

tgravescs commented Sep 17, 2021

Uh oh!

sperlingxx commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

revans2 commented Sep 17, 2021

Uh oh!

sperlingxx commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

revans2 commented Sep 17, 2021

Uh oh!

revans2 left a comment

Choose a reason for hiding this comment

Uh oh!

revans2 commented Sep 17, 2021

Uh oh!

sperlingxx commented Sep 10, 2021 •

edited

Loading

firestarman Sep 10, 2021 •

edited

Loading

gerashegalov Sep 10, 2021 •

edited

Loading

pxLi commented Sep 15, 2021 •

edited

Loading

sperlingxx commented Sep 17, 2021 •

edited

Loading

sperlingxx commented Sep 17, 2021 •

edited

Loading