Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support CPU path for from_utc_timestamp function with timezone #9689

Merged
merged 8 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions integration_tests/src/main/python/date_time_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,14 @@ def test_from_utc_timestamp_unsupported_timezone_fallback(data_gen, time_zone):
'FromUTCTimestamp')


@pytest.mark.parametrize('time_zone', ["UTC", "Asia/Shanghai"], ids=idfn)
@pytest.mark.parametrize('data_gen', [timestamp_gen], ids=idfn)
def test_from_utc_timestamp_supported_timezones(data_gen, time_zone):
# Remove spark.rapids.test.CPU.timezone configuration when GPU kernel is ready to really test on GPU
assert_gpu_and_cpu_are_equal_collect(
lambda spark: unary_op_df(spark, data_gen).select(f.from_utc_timestamp(f.col('a'), time_zone)), conf = {"spark.rapids.test.CPU.timezone": "true"})


@allow_non_gpu('ProjectExec')
@pytest.mark.parametrize('data_gen', [timestamp_gen], ids=idfn)
def test_unsupported_fallback_from_utc_timestamp(data_gen):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2055,6 +2055,12 @@ object RapidsConf {
.booleanConf
.createOptional

val TEST_USE_TIMEZONE_CPU_BACKEND = conf("spark.rapids.test.CPU.timezone")
.doc("Only for tests: verify for timezone related functions")
.internal()
.booleanConf
.createOptional

private def printSectionHeader(category: String): Unit =
println(s"\n### $category")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,37 @@
* limitations under the License.
*/

package com.nvidia.spark.rapids.timezone
package org.apache.spark.sql.rapids

import java.time.ZoneId

import ai.rapids.cudf.{ColumnVector, DType, HostColumnVector}
import com.nvidia.spark.rapids.Arm.withResource
import com.nvidia.spark.rapids.RapidsConf.TEST_USE_TIMEZONE_CPU_BACKEND

import org.apache.spark.SparkEnv
import org.apache.spark.sql.catalyst.util.DateTimeUtils

object TimeZoneDB {

// Copied from Spark. Used to format time zone ID string with (+|-)h:mm and (+|-)hh:m
def getZoneId(timezoneId: String): ZoneId = {
val formattedZoneId = timezoneId
// To support the (+|-)h:mm format because it was supported before Spark 3.0.
.replaceFirst("(\\+|\\-)(\\d):", "$10$2:")
// To support the (+|-)hh:m format because it was supported before Spark 3.0.
.replaceFirst("(\\+|\\-)(\\d\\d):(\\d)$", "$1$2:0$3")
DateTimeUtils.getZoneId(formattedZoneId)
}

// Support fixed offset or no transition rule case
def isSupportedTimezone(timezoneId: String): Boolean = {
val rules = getZoneId(timezoneId).getRules
// CPU backend is just for test purpose
SparkEnv.get.conf.getBoolean(TEST_USE_TIMEZONE_CPU_BACKEND.key, false) ||
winningsix marked this conversation as resolved.
Show resolved Hide resolved
(rules.isFixedOffset || rules.getTransitionRules.isEmpty)
}

def cacheDatabase(): Unit = {}

/**
Expand All @@ -42,10 +62,14 @@ object TimeZoneDB {
withResource(HostColumnVector.builder(DType.TIMESTAMP_MICROSECONDS, rowCount)) { builder =>
var currRow = 0
while (currRow < rowCount) {
val origin = input.getLong(currRow)
// Spark implementation
val dist = DateTimeUtils.toUTCTime(origin, zoneStr)
builder.append(dist)
if (input.isNull(currRow)) {
builder.appendNull()
} else {
val origin = input.getLong(currRow)
// Spark implementation
val dist = DateTimeUtils.toUTCTime(origin, zoneStr)
builder.append(dist)
}
currRow += 1
}
withResource(builder.build()) { b =>
Expand All @@ -72,10 +96,14 @@ object TimeZoneDB {
withResource(HostColumnVector.builder(DType.TIMESTAMP_MICROSECONDS, rowCount)) { builder =>
var currRow = 0
while (currRow < rowCount) {
val origin = input.getLong(currRow)
// Spark implementation
val dist = DateTimeUtils.fromUTCTime(origin, zoneStr)
builder.append(dist)
if(input.isNull(currRow)) {
builder.appendNull()
} else {
val origin = input.getLong(currRow)
// Spark implementation
val dist = DateTimeUtils.fromUTCTime(origin, zoneStr)
builder.append(dist)
}
currRow += 1
}
withResource(builder.build()) { b =>
Expand All @@ -97,10 +125,14 @@ object TimeZoneDB {
withResource(HostColumnVector.builder(DType.TIMESTAMP_DAYS, rowCount)) { builder =>
var currRow = 0
while (currRow < rowCount) {
val origin = input.getLong(currRow)
// Spark implementation
val dist = DateTimeUtils.microsToDays(origin, currentTimeZone)
builder.append(dist)
if (input.isNull(currRow)) {
builder.appendNull()
} else {
val origin = input.getLong(currRow)
// Spark implementation
val dist = DateTimeUtils.microsToDays(origin, currentTimeZone)
builder.append(dist)
}
currRow += 1
}
withResource(builder.build()) { b =>
Expand All @@ -124,10 +156,14 @@ object TimeZoneDB {
withResource(HostColumnVector.builder(DType.INT64, rowCount)) { builder =>
var currRow = 0
while (currRow < rowCount) {
val origin = input.getInt(currRow)
// Spark implementation
val dist = DateTimeUtils.daysToMicros(origin, desiredTimeZone)
builder.append(dist)
if (input.isNull(currRow)) {
builder.appendNull()
} else {
val origin = input.getInt(currRow)
// Spark implementation
val dist = DateTimeUtils.daysToMicros(origin, desiredTimeZone)
builder.append(dist)
}
currRow += 1
}
withResource(builder.build()) { b =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1044,29 +1044,28 @@ class FromUTCTimestampExprMeta(
rule: DataFromReplacementRule)
extends BinaryExprMeta[FromUTCTimestamp](expr, conf, parent, rule) {

private[this] var timezoneId: ZoneId = null

override def tagExprForGpu(): Unit = {
extractStringLit(expr.right) match {
case None =>
willNotWorkOnGpu("timezone input must be a literal string")
case Some(timezoneShortID) =>
if (timezoneShortID != null) {
val utc = ZoneId.of("UTC").normalized
// This is copied from Spark, to convert `(+|-)h:mm` into `(+|-)0h:mm`.
val timezone = ZoneId.of(timezoneShortID.replaceFirst("(\\+|\\-)(\\d):", "$10$2:"),
ZoneId.SHORT_IDS).normalized

if (timezone != utc) {
willNotWorkOnGpu("only timezones equivalent to UTC are supported")
if (TimeZoneDB.isSupportedTimezone(timezoneShortID)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal Spark config should actually be wired into here. The logic should be:

If the timezone is UTC -> Always stay on the GPU (no-op)
If the timezone is not UTC -> if the config is set to true, use the CPU POC as long as the timezone is supported, otherwise fallback to CPU

Copy link
Collaborator Author

@winningsix winningsix Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal Spark config (spark.rapids.test.CPU.timezone) was wired inside isSupportedTimezone method.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it's better to wire the config directly here. I find that putting it in the logic of isSupportedTimezone a bit confusing to understand, even though it might make the migration process simpler in a way. It's not actually hard to remove the config when we migrate to GPU timezone DB. Plus I'm not sure depending on the review if that will make it in 23.12, so I think we should stay safe with this code path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Please take a further look.

timezoneId = TimeZoneDB.getZoneId(timezoneShortID)
} else {
willNotWorkOnGpu(s"Not supported timezone type $timezoneShortID.")
}
}
}
}

override def convertToGpu(timestamp: Expression, timezone: Expression): GpuExpression =
GpuFromUTCTimestamp(timestamp, timezone)
GpuFromUTCTimestamp(timestamp, timezone, timezoneId)
}

case class GpuFromUTCTimestamp(timestamp: Expression, timezone: Expression)
case class GpuFromUTCTimestamp(timestamp: Expression, timezone: Expression, zoneId: ZoneId)
extends GpuBinaryExpressionArgsAnyScalar
with ImplicitCastInputTypes
with NullIntolerant {
Expand All @@ -1078,8 +1077,7 @@ case class GpuFromUTCTimestamp(timestamp: Expression, timezone: Expression)

override def doColumnar(lhs: GpuColumnVector, rhs: GpuScalar): ColumnVector = {
if (rhs.getBase.isValid) {
// Just a no-op.
lhs.getBase.incRefCount()
TimeZoneDB.fromUtcTimestampToTimestamp(lhs.getBase, zoneId)
winningsix marked this conversation as resolved.
Show resolved Hide resolved
} else {
// All-null output column.
GpuColumnVector.columnVectorFromNull(lhs.getRowCount.toInt, dataType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.nvidia.spark.rapids.SparkQueryCompareTestSuite

import org.apache.spark.SparkConf
import org.apache.spark.sql.{DataFrame, Row, SparkSession}
import org.apache.spark.sql.rapids.TimeZoneDB
import org.apache.spark.sql.types._

class TimeZoneSuite extends SparkQueryCompareTestSuite {
Expand Down