-
Notifications
You must be signed in to change notification settings - Fork 302
[FEA] Support yyyyMMdd under CORRECTED policy [databricks] #15820
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
base: main
Are you sure you want to change the base?
Changes from all commits
5e0ccde
1e57413
18bd197
d6b7179
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ import com.nvidia.spark.rapids.RapidsPluginImplicits._ | |
| import com.nvidia.spark.rapids.jni.{Arithmetic, CastException, CastStrings, DateTimeUtils, | ||
| GpuTimeZoneDB} | ||
| import com.nvidia.spark.rapids.shims.{NullIntolerantShim, ShimBinaryExpression, ShimExpression, | ||
| TruncTimestampShims} | ||
| TruncTimestampShims, YearParseUtil} | ||
|
|
||
| import org.apache.spark.sql.catalyst.expressions.{BinaryExpression, ExpectsInputTypes, Expression, FromUnixTime, FromUTCTimestamp, ImplicitCastInputTypes, MonthsBetween, TimeZoneAwareExpression, ToUTCTimestamp, TruncDate, TruncTimestamp} | ||
| import org.apache.spark.sql.catalyst.util.DateTimeConstants | ||
|
|
@@ -417,6 +417,10 @@ abstract class UnixTimeExprMeta[A <: BinaryExpression with TimeZoneAwareExpressi | |
| sparkFormat, | ||
| expr.left.dataType == DataTypes.StringType, | ||
| allowLegacyFormattingOnlyFormats = allowLegacyFormattingOnlyFormats) | ||
| // The fused parser only accepts an unsigned four-digit year for this packed format. | ||
| if (expr.left.dataType == DataTypes.StringType && sparkFormat == "yyyyMMdd") { | ||
| YearParseUtil.tagParseStringAsDate(conf, this) | ||
| } | ||
| case None => | ||
| willNotWorkOnGpu("format has to be a string literal") | ||
| } | ||
|
|
@@ -599,9 +603,9 @@ object ExceptionTimeParserPolicy extends TimeParserPolicy | |
| object CorrectedTimeParserPolicy extends TimeParserPolicy | ||
|
|
||
| object GpuToTimestamp { | ||
| // We are compatible with Spark for these formats when the timeParserPolicy is CORRECTED | ||
| // or EXCEPTION. It is possible that other formats may be supported but these are the only | ||
| // ones that we have tests for. | ||
| // We are compatible with Spark for these formats when the timeParserPolicy is CORRECTED. | ||
| // It is possible that other formats may be supported but these are the only ones that we | ||
| // have tests for. | ||
| val CORRECTED_COMPATIBLE_FORMATS = Set( | ||
| "yyyy-MM-dd", | ||
| "yyyy/MM/dd", | ||
|
|
@@ -617,9 +621,14 @@ object GpuToTimestamp { | |
| "MM-yyyy", | ||
| "MM/dd/yyyy", | ||
| "MM-dd-yyyy", | ||
| "yyyyMMdd", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a separate note this list is also used by
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified and covered in 18bd197. yyyyMMdd is now included in the direct date_format parity matrix for both Date and Timestamp inputs, including the runtime-fallback and timezone-rule cases. The focused Spark 3.5.7 GPU run selected 25 yyyyMMdd cases: 24 passed and the Spark-4-only case skipped, with no failures or errors. |
||
| "MMyyyy" | ||
| ) | ||
|
|
||
| // EXCEPTION first tries CORRECTED parsing and then probes LEGACY parsing on failure. Formats | ||
| // in this set must therefore match Spark under both parsers, including success/failure behavior. | ||
| val EXCEPTION_COMPATIBLE_FORMATS = CORRECTED_COMPATIBLE_FORMATS - "yyyyMMdd" | ||
|
|
||
| // We are compatible with Spark for these formats when the timeParserPolicy is LEGACY. It | ||
| // is possible that other formats may be supported but these are the only ones that we have | ||
| // tests for. | ||
|
|
@@ -666,12 +675,13 @@ object GpuToTimestamp { | |
| } | ||
|
|
||
| // True iff the fused JNI parser handles this (sparkFormat, policy) combination. | ||
| // Today the JNI accepts every entry in CORRECTED_COMPATIBLE_FORMATS / LEGACY_COMPATIBLE_FORMATS. | ||
| private def isSimpleSparkFormat(sparkFormat: String, isLegacy: Boolean): Boolean = { | ||
| if (isLegacy) { | ||
| LEGACY_COMPATIBLE_FORMATS.contains(sparkFormat) | ||
| } else { | ||
| CORRECTED_COMPATIBLE_FORMATS.contains(sparkFormat) | ||
| private def isSimpleSparkFormat( | ||
| sparkFormat: String, | ||
| timeParserPolicy: TimeParserPolicy): Boolean = { | ||
| timeParserPolicy match { | ||
| case LegacyTimeParserPolicy => LEGACY_COMPATIBLE_FORMATS.contains(sparkFormat) | ||
| case ExceptionTimeParserPolicy => EXCEPTION_COMPATIBLE_FORMATS.contains(sparkFormat) | ||
| case CorrectedTimeParserPolicy => CORRECTED_COMPATIBLE_FORMATS.contains(sparkFormat) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -717,7 +727,12 @@ object GpuToTimestamp { | |
| exceptionPolicy: Boolean): ColumnVector = { | ||
|
|
||
| // `tsVector` will be closed in replaceSpecialDates | ||
| val tsVector = if (isSimpleSparkFormat(sparkFormat, isLegacy = false)) { | ||
| val timeParserPolicy = if (exceptionPolicy) { | ||
| ExceptionTimeParserPolicy | ||
| } else { | ||
| CorrectedTimeParserPolicy | ||
| } | ||
| val tsVector = if (isSimpleSparkFormat(sparkFormat, timeParserPolicy)) { | ||
| // Fused kernel skips the regex+length+cuDF-asTimestamp chain. | ||
| val parsed = try { | ||
| val parserPolicy = if (exceptionPolicy) { | ||
|
|
@@ -777,7 +792,7 @@ object GpuToTimestamp { | |
| def parseStringAsTimestampWithLegacyParserPolicy( | ||
| lhs: GpuColumnVector, | ||
| sparkFormat: String): ColumnVector = { | ||
| if (!isSimpleSparkFormat(sparkFormat, isLegacy = true)) { | ||
| if (!isSimpleSparkFormat(sparkFormat, LegacyTimeParserPolicy)) { | ||
| throw new IllegalStateException(s"Unsupported format $sparkFormat") | ||
| } | ||
| CastStrings.parseTimestampWithFormat(lhs.getBase, sparkFormat, true) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list claims to be for both CORRECTED and EXCEPTION, and then there is a separate list for LEGACY, but I think that is misleading. EXCEPTION is supposed to:
Per step 2 that means being supported under EXCEPTION also requires LEGACY to produce valid success/failure semantics. E.g. AI came up with the combination of
2024101toyyyyMMddunder EXCEPTION, which should fail (Spark LEGACY accepts it) but we succeed (the JNI throws on it in LEGACY). I think we need three compatibility lists: CORRECTED, EXCEPTION, and LEGACY, where atmyyyyMMddis not EXCEPTION compatible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed in 18bd197 by separating EXCEPTION_COMPATIBLE_FORMATS from CORRECTED_COMPATIBLE_FORMATS and selecting the policy-specific set during both tagging and execution. yyyyMMdd now falls back to CPU under EXCEPTION, so Spark preserves the CORRECTED/LEGACY disagreement behavior. I added coverage that asserts GetTimestamp fallback for a normal yyyyMMdd value and the expected error for 2024101. The focused Spark 3.5.7 GPU run passed.