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.

Sign up for GitHub

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

Jump to bottom

CAST string to temporal type now calls isTimestamp #1718

Merged
merged 8 commits into from
Feb 17, 2021
Merged

CAST string to temporal type now calls isTimestamp #1718

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cb48046
CAST string to temporal types now uses isTimestamp to detect valid in…
andygrove Feb 12, 2021
53c917b
Bug fix in timestamp formats
andygrove Feb 13, 2021
0f3520f
Update copyright and add additional test cases
andygrove Feb 16, 2021
48e1c9c
Refactor to avoid additional regex call
andygrove Feb 16, 2021
2b0d86c
Remove regex for full timestamp match
andygrove Feb 16, 2021
7316576
Update comment
andygrove Feb 16, 2021
9ab1292
Add test data to demonstrate why regex was needed
andygrove Feb 16, 2021
d5e0055
Reinstate regex for full timestamp
andygrove Feb 16, 2021
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
60 changes: 43 additions & 17 deletions sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,10 @@ object GpuCast {
private val TIMESTAMP_REGEX_YYYY = "\\A\\d{4}\\Z"
jlowe marked this conversation as resolved.
Show resolved Hide resolved
private val TIMESTAMP_REGEX_YYYY_MM = "\\A\\d{4}\\-\\d{2}[ ]?\\Z"
private val TIMESTAMP_REGEX_YYYY_MM_DD = "\\A\\d{4}\\-\\d{2}\\-\\d{2}[ ]?\\Z"
private val TIMESTAMP_REGEX_FULL =
"\\A\\d{4}\\-\\d{2}\\-\\d{2}[ T]\\d{2}:\\d{2}:\\d{2}\\.\\d{6}Z\\Z"
private val TIMESTAMP_REGEX_FULL_1 =
"\\A\\d{4}\\-\\d{2}\\-\\d{2} \\d{2}:\\d{2}:\\d{2}\\.\\d{6}Z\\Z"
private val TIMESTAMP_REGEX_FULL_2 =
"\\A\\d{4}\\-\\d{2}\\-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{6}Z\\Z"
private val TIMESTAMP_REGEX_NO_DATE = "\\A[T]?(\\d{2}:\\d{2}:\\d{2}\\.\\d{6}Z)\\Z"

/**
Expand Down Expand Up @@ -637,10 +639,16 @@ case class GpuCast(
regex: String,
cudfFormat: String): ColumnVector = {

withResource(Scalar.fromNull(DType.TIMESTAMP_DAYS)) { nullScalar =>
withResource(input.matchesRe(regex)) { isMatch =>
withResource(input.asTimestampDays(cudfFormat)) { asDays =>
isMatch.ifElse(asDays, nullScalar)
val isValidDate = withResource(input.matchesRe(regex)) { isMatch =>
withResource(input.isTimestamp(cudfFormat)) { isTimestamp =>
isMatch.and(isTimestamp)
}
}

withResource(isValidDate) { isValidDate =>
withResource(input.asTimestampDays(cudfFormat)) { asDays =>
withResource(Scalar.fromNull(DType.TIMESTAMP_DAYS)) { nullScalar =>
isValidDate.ifElse(asDays, nullScalar)
}
}
}
Expand All @@ -653,10 +661,16 @@ case class GpuCast(
cudfFormat: String,
orElse: ColumnVector): ColumnVector = {

withResource(input.matchesRe(regex)) { isMatch =>
val isValidDate = withResource(input.matchesRe(regex)) { isMatch =>
withResource(input.isTimestamp(cudfFormat)) { isTimestamp =>
isMatch.and(isTimestamp)
}
}

withResource(isValidDate) { isValidDate =>
withResource(input.asTimestampDays(cudfFormat)) { asDays =>
withResource(orElse) { orElse =>
isMatch.ifElse(asDays, orElse)
isValidDate.ifElse(asDays, orElse)
}
}
}
Expand Down Expand Up @@ -721,10 +735,16 @@ case class GpuCast(
regex: String,
cudfFormat: String): ColumnVector = {

withResource(Scalar.fromNull(DType.TIMESTAMP_MICROSECONDS)) { nullScalar =>
withResource(input.matchesRe(regex)) { isMatch =>
val isValidTimestamp = withResource(input.matchesRe(regex)) { isMatch =>
withResource(input.isTimestamp(cudfFormat)) { isTimestamp =>
isMatch.and(isTimestamp)
}
}

withResource(isValidTimestamp) { isValidTimestamp =>
withResource(Scalar.fromNull(DType.TIMESTAMP_MICROSECONDS)) { nullScalar =>
withResource(input.asTimestampMicroseconds(cudfFormat)) { asDays =>
isMatch.ifElse(asDays, nullScalar)
isValidTimestamp.ifElse(asDays, nullScalar)
}
}
}
Expand All @@ -737,10 +757,15 @@ case class GpuCast(
cudfFormat: String,
orElse: ColumnVector): ColumnVector = {

withResource(input.matchesRe(regex)) { isMatch =>
val isValidTimestamp = withResource(input.matchesRe(regex)) { isMatch =>
withResource(input.isTimestamp(cudfFormat)) { isTimestamp =>
isMatch.and(isTimestamp)
}
}
withResource(isValidTimestamp) { isValidTimestamp =>
withResource(input.asTimestampMicroseconds(cudfFormat)) { asDays =>
withResource(orElse) { orElse =>
isMatch.ifElse(asDays, orElse)
isValidTimestamp.ifElse(asDays, orElse)
}
}
}
Expand Down Expand Up @@ -779,10 +804,11 @@ case class GpuCast(

// convert dates that are in valid timestamp formats
val converted =
convertTimestampOr(sanitizedInput, TIMESTAMP_REGEX_FULL, "%Y-%m-%dT%H:%M:%SZ%f",
convertTimestampOr(sanitizedInput, TIMESTAMP_REGEX_YYYY_MM_DD, "%Y-%m-%d",
convertTimestampOr(sanitizedInput, TIMESTAMP_REGEX_YYYY_MM, "%Y-%m",
convertTimestampOrNull(sanitizedInput, TIMESTAMP_REGEX_YYYY, "%Y"))))
convertTimestampOr(sanitizedInput, TIMESTAMP_REGEX_FULL_1, "%Y-%m-%d %H:%M:%S.%f",
convertTimestampOr(sanitizedInput, TIMESTAMP_REGEX_FULL_2, "%Y-%m-%dT%H:%M:%S.%f",
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to do an extra regex pass? IIRC the regex passes are really expensive, and I'm curious how it compares to the cost of isValidTimestamp and asTimestampMicrosceonds. I'm wondering if it would be overall cheaper to use the original regex and ifElse the two valid+conversion formats corresponding to that regex instead of adding another regex pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense. I'll refactor this part to avoid the additional regex call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed one commit that refactors this to use a single regex and then pushed another commit to remove the regex completely for this full timestamp case and tests pass with the current test values.

@revans2 would you have any concerns about removing regex use in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Once we have sanitized the inputs the regular expressions are redundant and should be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can probably remove even more regular expressions from the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at this some more and remembered why we have the regex here. We need to make sure there is no additional text after the sanitized date/timestamp string. I added an additional test case to demonstrate this for timestamps and reverted the change to remove the regex for this case.

I do think could remove more of these regexes by checking the length of the sanitized strings (in conjunction with calling cuDF isTimestamp) rather than using regex and have filed an issue for this: #1738

convertTimestampOr(sanitizedInput, TIMESTAMP_REGEX_YYYY_MM_DD, "%Y-%m-%d",
convertTimestampOr(sanitizedInput, TIMESTAMP_REGEX_YYYY_MM, "%Y-%m",
convertTimestampOrNull(sanitizedInput, TIMESTAMP_REGEX_YYYY, "%Y")))))

// handle special dates like "epoch", "now", etc.
val finalResult = specialDates.foldLeft(converted)((prev, specialDate) =>
Expand Down
4 changes: 4 additions & 0 deletions tests/src/test/scala/com/nvidia/spark/rapids/CastOpSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,10 @@ object CastOpSuite {
"2018-1random_text",
jlowe marked this conversation as resolved.
Show resolved Hide resolved
"2018-11-08random_text",
"2018-11-9random_text",
// date component out of range
"2020-13-01",
"2020-12-32",
"2020-02-30",
// `yyyy-[m]m-[d]dT*` in Spark 3.1+ these no longer work for AnsiCast, but did before
"2010-1-01T!@#$%",
"2010-1-02T,",
Expand Down