fix: surface next_day and make_date ANSI errors as Spark exceptions - #5167
fix: surface next_day and make_date ANSI errors as Spark exceptions#5167peterxcli wants to merge 1 commit into
Conversation
andygrove
left a comment
There was a problem hiding this comment.
Thanks for this, and thanks for the thorough PR description. I checked the shim mapping against Spark 3.4.3, 3.5.8, 4.0.1, 4.1.0, and master, and the version handling is correct:
NextDayunder ANSI on 3.4/3.5 throws a plainIllegalArgumentExceptionfromDateTimeUtils.getDayOfWeekFromStringand wraps it inQueryExecutionErrors.ansiIllegalArgumentError(_LEGACY_ERROR_TEMP_2000). On 4.0+ it throwsSparkIllegalArgumentExceptionwithILLEGAL_DAY_OF_WEEKand astringparameter. Both shim branches match exactly, including the reconstructed 3.x message text.MakeDateunder ANSI usesansiDateTimeErroron 3.4/3.5 andansiDateTimeArgumentOutOfRangeon 4.x. Delegating to the sameQueryExecutionErrorshelper Spark itself calls is the right call, because the error class behind that helper has moved three times (DATETIME_FIELD_OUT_OF_BOUNDSon 4.0,.WITH_SUGGESTIONon 4.1,.WITHOUT_SUGGESTIONon master) and the shim tracks it for free.- Neither Spark error carries a
QueryContext, and the shim correctly ignores the one Comet threads through.
I also went through every branch of invalid_date_message against LocalDate.of. It is a careful reproduction of the java.time wording, including the uppercase month enum name in Invalid date 'APRIL 31' and the differently cased Invalid date 'February 29' as '2023' is not a leap year. One field looks uncovered, which is my main comment below.
A few things I would like to see addressed.
invalid_date_message is missing the Year field
LocalDate.of validates YEAR before MONTH_OF_YEAR, and the valid range is -999999999 to 999999999. So make_date(1000000000, 1, 1) gives Spark:
Invalid value for Year (valid values -999999999 - 999999999): 1000000000
whereas invalid_date_message passes the month and day checks and falls through to Invalid date 'JANUARY 1'. Before this PR that string was buried inside a CometNativeException, but now it becomes the rangeMessage parameter of a real DATETIME_FIELD_OUT_OF_BOUNDS error, so it is the message the user actually reads. Would you mind adding a Year branch as the first check, so the field ordering matches java.time as well?
make_date is now advertised as unqualified Compatible()
CometMakeDate has no getSupportLevel override, so removing getCompatibleNotes() leaves the expression with no caveats at all. I think there is still a divergence hiding there. NaiveDate::from_ymd_opt is bounded by chrono's MAX_YEAR, which is (i32::MAX >> 13) - 1, or 262142. That is far narrower than java.time's 999999999. So make_date(300000, 6, 15) is a perfectly valid date to Spark, but Comet's make_date returns None for it. That means NULL with ANSI off, and with this PR it becomes a well formed SparkDateTimeException claiming the date is out of bounds when Spark would have returned a value.
That part is pre-existing and I am not asking you to fix chrono's range here. Could you confirm the behavior and file a tracking issue, then link it either from a getSupportLevel note or from the compatibility index? My concern is that this PR makes the wrong answer look authoritative while removing the last remaining caveat on the expression.
The new test does not assert the message
next_day and make_date ANSI errors match Spark exceptions checks the exception class, error class, and SQLSTATE, but not the message. The message is the one dimension where I found an actual gap, so it would be worth closing. Comparing getMessageParameters would pin rangeMessage and string directly and would catch the year case above. A third query with a year outside java.time's range would exercise it.
The assert(!causeChain(cometFailure).exists(_.isInstanceOf[CometNativeException])) guard is a good touch. It is what makes this a real regression test rather than a coincidence.
Consider moving the assertion helpers into CometTestBase
causeChain and deepestSparkThrowable are the reusable part of this test, and the two remaining bullets in the same compatibility index section (5072 and 4967) are the same class of problem for other datetime and arithmetic functions. Could they move into CometTestBase as something like checkSparkExceptionMatches(df)? Otherwise the next ANSI parity fix will copy them and the two copies will drift.
The corrected note in make_date_ansi.sql will go stale again
It says DATETIME_FIELD_OUT_OF_BOUNDS.WITH_SUGGESTION on 4.1+, but ansiDateTimeArgumentOutOfRange on Spark master switched to DATETIME_FIELD_OUT_OF_BOUNDS.WITHOUT_SUGGESTION, and CI runs 4.2. Since the shim delegates to the helper rather than hardcoding a class, the behavior is version agnostic. Could the comment say the same? Something like "the exact subclass varies by version (DATETIME_FIELD_OUT_OF_BOUNDS on 4.0, .WITH_SUGGESTION on 4.1, .WITHOUT_SUGGESTION on 4.2+)" would stop it going stale on the next Spark bump.
|
@andygrove thanks for the review, push review change and filed two issue per your review!
Added Java
The test now compares exception class, error class, SQL state,
Filed #5208 and restored
Found five equivalent cause-chain traversals across four suites. Filed #5223) to centralize
Reworded the SQL test comment to state that the subclass and message parameters vary by Spark version, without pinning a specific version boundary. |
Which issue does this PR close?
Closes #5073.
Rationale for this change
Under ANSI mode, native
next_dayandmake_datereturn generic DataFusion execution errors. Those bypass Comet's structuredSparkErrorconversion at the JNI boundary and surface asCometNativeException, losing Spark's exception class, error class, and SQLSTATE.What changes are included in this PR?
IllegalDayOfWeekandDatetimeFieldOutOfBoundsSpark errors and emit them from the native functions.CometNativeException.How are these changes tested?
make coreCometTemporalExpressionSuiteregression on Spark 3.4, 3.5, 4.0, 4.1, and 4.2:next_day and make_date ANSI errors match Spark exceptionscargo fmt --manifest-path native/Cargo.toml --all -- --check