-
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
from_unixtime might consider handle week year format #11004
Comments
@shiyu-bytedance seems like this is already correctly implemented in format_datetime()
|
@pedroerp thanks. I'm surprised because I'd be not sure how this test passes.
|
I'm puzzled now as well; @amitkdutta any chance you know how this works in Prestissimo today? |
@amitkdutta ping |
@shiyu-bytedance ignore my comment above. You are right, it is not supported today. My test above ended up being executed in the Java stack :( Would you like to add support for it? |
@pedroerp Yes, I'd like to add support for it. However, to manage expectation, this issue, as we internally at Bytedance realized recently, is more involved than meets the eye. Hinnant's library implements ISO8601 strictly; however, Presto/Spark uses SimpleDateFormatter(from . These two libraries do not agree 100% on how to format certain edge cases; so for 100% compatibility, before diving straight into coding, I think it'd be advisable to think about how to tackle the issue of velox behaving identical to presto and spark (who are the main users of velox as an acceleration library), which may require net new code that bridges the gap instead of just invoking Hinnant's library. |
Thanks for clarifying. If Presto is not following ISO8601 (which I suppose it should), maybe this is something we should bring up with the Presto community and get fixed? Have you tried opening an issue to prestodb? |
Description
Currently, DateTimeFormatter treats Y and y the same.
Strictly speaking, this is incorrect because Y should only be used for week year format. (see https://errorprone.info/bugpattern/MisusedWeekYear#:~:text=%E2%80%9CWeek%20year%E2%80%9D%20is%20intended%20to,year%20specifier%20%E2%80%9Cyyyy%E2%80%9D%20instead.)
Velox datetime functions currently do not support the week year style of formatting(https://en.wikipedia.org/wiki/ISO_8601#Week_dates), we might consider adding support for it.
Additional pointer:
cc @pedroerp
The text was updated successfully, but these errors were encountered: