-
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 2 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") | ||
| } | ||
|
|
@@ -617,6 +621,7 @@ 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" | ||
| ) | ||
|
|
||
|
|
||
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.