Deprecate Joda library for Presto Teradata functions #15537
Conversation
|
|
rk13
force-pushed
the
teradata-migrate-joda
branch
from
6c8ad43
to
025073a
Compare
...ta-functions/src/main/java/com/facebook/presto/teradata/functions/TeradataDateFunctions.java
Show resolved
Hide resolved
...ctions/src/main/java/com/facebook/presto/teradata/functions/dateformat/DateFormatParser.java
Outdated
Show resolved
Hide resolved
...unctions/src/test/java/com/facebook/presto/teradata/functions/TestTeradataDateFunctions.java
Outdated
Show resolved
Hide resolved
...ta-functions/src/main/java/com/facebook/presto/teradata/functions/TeradataDateFunctions.java
Outdated
Show resolved
Hide resolved
rk13
force-pushed
the
teradata-migrate-joda
branch
from
025073a
to
2b89c16
Compare
rk13
force-pushed
the
teradata-migrate-joda
branch
2 times, most recently
from
3e88847
to
db480f3
Compare
|
Rebased PR branch on the latest master |
rk13
force-pushed
the
teradata-migrate-joda
branch
from
db480f3
to
b5aee06
Compare
...ctions/src/main/java/com/facebook/presto/teradata/functions/dateformat/DateFormatParser.java
Outdated
Show resolved
Hide resolved
| if (formatContainsHourOfAMPM) { | ||
| // At the moment format does not allow to include AM/PM token, thus it was never possible to specify PM hours using 'HH' token in format | ||
| // Keep existing behaviour by defaulting to 0(AM) for AMPM_OF_DAY if format string contains 'HH' | ||
| builder.parseDefaulting(ChronoField.HOUR_OF_AMPM, 0) |
There was a problem hiding this comment.
Could we set all three of the parseDefaultings regardless of whether the format contains hour of AMPM (if a format doesn't have HOUR_OF_AMPM, then this won't be relevant, and similarly for HOUR_OF_DAY if it does have HOUR_OF_AMPM)?
There was a problem hiding this comment.
Unfortunately, not.
Conflicts can happen if both HOUR_OF_DAY and HOUR_OF_AMPM values provided (e.g. one with parseDefaultings and another in the input).
Example: to_timestamp('1988/04/08 2','yyyy/mm/dd hh') will trigger "Conflict found: HourOfDay 0 differs from HourOfDay 2" error if we set all three of the parseDefaultings
...ctions/src/main/java/com/facebook/presto/teradata/functions/dateformat/DateFormatParser.java
Outdated
Show resolved
Hide resolved
...ta-functions/src/main/java/com/facebook/presto/teradata/functions/TeradataDateFunctions.java
Outdated
Show resolved
Hide resolved
...ta-functions/src/main/java/com/facebook/presto/teradata/functions/TeradataDateFunctions.java
Outdated
Show resolved
Hide resolved
| // Append default values(0) for time fields(HH24, HH, MI, SS) because JSR-310 does not accept bare Date value as DateTime | ||
|
|
||
| if (formatContainsHourOfAMPM) { | ||
| // At the moment format does not allow to include AM/PM token, thus it was never possible to specify PM hours using 'HH' token in format |
There was a problem hiding this comment.
Is there any reason we couldn't or shouldn't support an AM/PM token?
There was a problem hiding this comment.
No, I do not know about any reason why we should not.
Teradata date format arguments support AM/PM actually (https://www.docs.teradata.com/r/~_sY_PYVxZzTnqKq45UXkQ/DlNXnvtMQTAeWXYLHvEarQ).
Original grammar used to tokenise input should be extended in this case
https://github.com/prestodb/presto/blob/master/presto-teradata-functions/src/main/antlr4/com/facebook/presto/teradata/functions/DateFormat.g4
It is better to be implemented in separate PR as a new feature I think.
...ctions/src/main/java/com/facebook/presto/teradata/functions/dateformat/DateFormatParser.java
Outdated
Show resolved
Hide resolved
rk13
force-pushed
the
teradata-migrate-joda
branch
2 times, most recently
from
63a5fa3
to
3779262
Compare
|
@rschlussel Can you please take another look at this PR. |
rk13
force-pushed
the
teradata-migrate-joda
branch
from
3779262
to
e8dddaa
Compare
|
Rebased on the latest master |
Test plan - (Please fill in how you tested your changes)
Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. Don't forget to follow our attribution guidelines for any code copied from other projects.
Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.