[SPARK-57846][SQL] Add the try_make_time function#56932
Conversation
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
1 blocking, 0 non-blocking, 1 nits.
Clean, faithful mirror of the try_* datetime-constructor pattern (TryEval(MakeTime(...)), like try_to_time). Implementation, registry, API surface, docs, and end-to-end tests are all correct and consistent. One blocking versioning fix.
Correctness (1)
timeExpressions.scala:645,functions.scala:904,builtin.py:25589:@since/versionaddedis4.1.0, but v4.1.0 is already released and does not contain this new function — should bebranch-4.x's4.3.0. See inline.
Nits: 1 minor item (see inline comments).
| ) | ||
| } | ||
|
|
||
| test("creating values of TimeType via try_make_time") { |
There was a problem hiding this comment.
Nit (non-blocking): this test builds TryEval(MakeTime(...)) directly, so despite its title it doesn't instantiate the new TryMakeTime expression or TryMakeTimeExpressionBuilder. The class/builder are covered end-to-end by TimeFunctionsSuiteBase and the SQL golden, so coverage isn't missing — but constructing new TryMakeTime(...) here would make the unit test match its title and directly cover the new expression's plumbing (including the builder's arg-count error path, which nothing currently exercises).
|
@MaxGekk Addressed the review comments: added a TryMakeTime wrapper so the display name is try_make_time, added the PySpark binding, fixed the doctest, and set the version to 4.3.0. Could you take another look when you get a chance? |
…per for try_make_time
…st TryMakeTime directly
e81b677 to
6209b71
Compare
| if (numArgs == 3) { | ||
| new TryMakeTime(expressions(0), expressions(1), expressions(2)) | ||
| } else { | ||
| throw QueryCompilationErrors.wrongNumArgsError(funcName, Seq(3), numArgs) |
There was a problem hiding this comment.
Yes - TimeExpressionsSuite covers it in "creating values of TimeType via try_make_time"; the invalid-input case goes through TryEval and returns null.
| val invalidSchema = StructType(Seq( | ||
| StructField("hour", IntegerType, nullable = false), | ||
| StructField("minute", IntegerType, nullable = false), | ||
| StructField("second", DecimalType(16, 6), nullable = false) | ||
| )) |
There was a problem hiding this comment.
This looks like a duplicate code block, please just re-use schema.
There was a problem hiding this comment.
Done - dropped the duplicate schema and reused the existing schema val.
| val invalidResult = invalidDf.selectExpr("try_make_time(hour, minute, second)") | ||
| checkAnswer(invalidResult, Seq(Row(null), Row(null), Row(null))) | ||
| } | ||
|
|
There was a problem hiding this comment.
Null-input coverage for the DataFrame path lives only in the Catalyst unit test; the TimeFunctionsSuiteBase test covers valid + invalid, but not NULL columns.
There was a problem hiding this comment.
Added a NULL-column case through the DataFrame API that asserts try_make_time returns null.
| "01:02:03.4", | ||
| "23:59:59.999999" | ||
| ).toDF("timeString").select(col("timeString").cast("time")) | ||
| checkAnswer(result1, expected) |
There was a problem hiding this comment.
Note about equivalence assertion strength: the integration test compares valid output to a hardcoded string cast to time. DateFunctionsSuite.try_make_timestamp instead asserts equivalence against make_timestamp directly (checkAnswer(tryResult, makeResult) under ANSI_ENABLED=false). Doing the same here (valid rows equal make_time) would more directly express the "identical on valid input" contract.
There was a problem hiding this comment.
Switched the valid-input assertion to checkAnswer against make_time directly, matching the try_make_timestamp test.
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
I left a few more comments, but mostly looks good.
…e_time assertions
3 participants
What changes were proposed in this pull request?
Adds the
try_make_time(hour, minute, second)function, the error-safe counterpart tomake_time: it returns the same result on valid input andNULL(instead of throwing) on invalid input. It is implemented asTryEval(MakeTime(...)), mirroringtry_make_timestamp. Registered in the function registry, exposed viafunctions.scala, and documented (ANSI-compliance try-functions list +sql-expression-schema).Why are the changes needed?
make_timethrows on invalid components (e.g.hour = 25). Users want a null-returning variant consistent with the othertry_*datetime constructors (try_make_timestamp, etc.).Does this PR introduce any user-facing change?
Yes - a new function
try_make_time.How was this patch tested?
Unit tests (
TimeExpressionsSuite), integration (TimeFunctionsSuiteBase), SQL-level (time.sql), and Connect plan-generation goldens: valid input equalsmake_time, invalid input returnsNULL.Was this patch authored or co-authored using generative AI tooling?
Authored with assistance by Claude Opus 4.8.