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 |
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
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
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
For the method with a default value, we should have try catch, and return default in the catch; for the method without the default value, we should just throw the exception out
There was a problem hiding this comment.
We should directly throw exception instead of returning 0. Using implicit default value can case unexpected behavior
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
3 participants
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