fix fromDateTime malform handling #11258
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11258 +/- ##
=============================================
+ Coverage 0.11% 14.44% +14.33%
- Complexity 0 201 +201
=============================================
Files 2229 2343 +114
Lines 119803 125698 +5895
Branches 18126 19310 +1184
=============================================
+ Hits 137 18160 +18023
+ Misses 119646 106002 -13644
- Partials 20 1536 +1516
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 901 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| try { | ||
| return dateTimeFormatter.parseMillis(dateTimeString); | ||
| } catch (Exception e) { | ||
| return 0L; |
There was a problem hiding this comment.
i have concerns on doing this by default. one alternative is to add a
fromDateTime(String dateTimeString, String pattern, long defaultValue)
and only allow returning default value when malformed dateTimeString is encountered otherwise still throw an exception. please share your thoughts
There was a problem hiding this comment.
I'd suggest doing it more explicitly by asking for an extra default value. This is similar to how we handle json path error
There was a problem hiding this comment.
yeah unfortunately the function signature with extra default value is taken by the timezone :-( so we can't add that
There was a problem hiding this comment.
Why? The default value should be long type
There was a problem hiding this comment.
function registry is using number of arguments to match scalar function with the same name, it doesn't consider type b/c v1 is not strong-typed
| @@ -491,7 +492,7 @@ public static String[] toDateTimeMV(long[] millis, String pattern, String timezo | |||
| * Converts DateTime string represented by pattern to epoch millis | |||
| */ | |||
| @ScalarFunction | |||
| public static long fromDateTime(String dateTimeString, String pattern) { | |||
| public static long fromDateTime(@Nullable String dateTimeString, String pattern) { | |||
There was a problem hiding this comment.
This is incorrect. We don't expect null values to be passed
There was a problem hiding this comment.
the dateTimeString column can have nulls right?
There was a problem hiding this comment.
If it is null, this scalar function will return null, which is expected
There was a problem hiding this comment.
good point, i think i mixed null and malformed handling , let's only target malformed string in this PR. i am not sure whether we should allow null in dateTimeString functions
| } | ||
|
|
||
| private static long parseMillis(String dateTimeString, DateTimeFormatter dateTimeFormatter) { | ||
| return parseMillis(dateTimeString, dateTimeFormatter, 0L); |
There was a problem hiding this comment.
We should directly throw exception instead of returning 0. Using implicit default value can case unexpected behavior
|
|
||
| public static long parseDateTimeStringToEpochMillis(String dateTimeString, String pattern, String timezoneId, | ||
| long defaultVal) { | ||
| DateTimeFormatter dateTimeFormatter = getDateTimeFormatter(pattern, timezoneId); |
There was a problem hiding this comment.
Need to have a try-catch over getDateTimeFormatter as well
walterddr
force-pushed
the
fix_from_datetime
branch
from
26d9cab to
d9bf40f
Compare
Allow malformed dateTime string to return default value configurable in the function signature (must explicitly state the timezone)
syntax
LIMITATION
this is a workaround of the more standard SQL
REFERENCE