fix: Support legacy date formatter for Spark date_format function #15535
Conversation
zml1206
requested review from
jinchengchenghh,
rui-mo and
zhli1142015
as code owners
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
|
| case 'S': | ||
| builder.appendFractionOfSecond(count); | ||
| break; | ||
| case 'u': |
There was a problem hiding this comment.
I'm not sure why this function is in functions/lib, looks like this function is only used in Spark. CC @rui-mo
And this behavior is not supported in Presto because it always uses joda time.
If we move it to Spark folder, we can safely change it to spark specified behavior.
presto:default> SELECT format_datetime(TIMESTAMP '1970-01-01', 'u');
Query 20251118_104605_00011_2tw34 failed: Illegal pattern component: u
There was a problem hiding this comment.
The historical reasons are unclear, it may be for the purpose of centralizing all formatters.
There was a problem hiding this comment.
I'm not sure why this function is in functions/lib, looks like this function is only used in Spark
Hi @NEUpanning, do you have any input on this? I only found #10354 but it doesn’t explain why the simple formatter cannot be Spark-specific.
There was a problem hiding this comment.
I thought formatters which could be reused should in functions/lib when I saw mysql formatter is only used by presto yet remains in functions/lib.
| break; | ||
| case 'u': | ||
| builder.appendDayOfWeek1Based(count); | ||
| break; |
There was a problem hiding this comment.
Keep alphabetical order
There was a problem hiding this comment.
Please don't move it, they are all appendDayOfWeekxxx
There was a problem hiding this comment.
It wasn't removed, it was just moved to the bottom.
There was a problem hiding this comment.
Maybe it placed here because the next is appendDayOfWeekText, both way can be acceptable, integrate with appendDayOfWeekxxx or alphabetical order
| if (!isConstFormat_) { | ||
| auto formatter = detail::initializeFormatter( | ||
| std::string_view(formatString), legacyFormatter_); | ||
| if (formatter) { |
There was a problem hiding this comment.
if (!formatter) {
return false;
}
formatter_ = formatter;
maxResultSize_ = formatter_->maxResultSize(sessionTimeZone_);
rui-mo
added
the
ready-to-merge
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this in D88172777. |
|
The Spark fuzzer failure seems unrelated, do you know why? @rui-mo |
5 participants
Support legacy date formatter for Spark date_format function.