-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Invalid format returns null for legacy date formatter #11131
base: main
Are you sure you want to change the base?
Invalid format returns null for legacy date formatter #11131
Conversation
✅ Deploy Preview for meta-velox canceled.
|
@rui-mo Could you help to review this PR please? Thanks. |
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.
Thanks.
kDefaultFormat_, | ||
config.sparkLegacyDateFormatter() ? DateTimeFormatterType::STRICT_SIMPLE | ||
: DateTimeFormatterType::JODA); | ||
// Default format should always be valid. | ||
VELOX_CHECK_EQ(formatter.hasError(), false); |
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.
VELOX_CHECK(!formatter.hasError());
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.
Thanks. Updated.
invalidFormat_ = true; | ||
} else { | ||
this->format_ = formatter.value(); | ||
} | ||
} catch (const VeloxUserError&) { |
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.
I wound be nice to make buildJodaDateTimeFormatter
return Expected so getDateTimeFormatter
does not throw exception and the try-catch clause can be removed.
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.
Thanks for your suggestion. Done.
std::string_view(format.data(), format.size()), | ||
legacyFormatter_ ? DateTimeFormatterType::STRICT_SIMPLE | ||
: DateTimeFormatterType::JODA); | ||
if (formatter.hasError()) { | ||
return false; | ||
} |
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.
Exception from 'buildJodaDateTimeFormatter' is not handled, so it shall work here if 'buildJodaDateTimeFormatter' returns Expected.
Invalid format returns null for Spark functions that use legacy(Simple) date formatter to align with SparkSQL behavior. Included functions are : 'unix_timestamp', 'from_unixtime' and 'get_timestamp'.
Relates #10354