Updates Datum to use java.time for a sane datetime #1691
Conversation
RCHowell
changed the title
CROSS-ENGINE-REPORT ❌
Testing Details
Result Details
Now FAILING Tests ❌The following 2 test(s) were previously PASSING in BASE but are now FAILING in TARGET: Click here to see
Now IGNORED Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. Now Passing Tests180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-4EFC211). Before merging, confirm they are intended to pass. The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. CROSS-COMMIT-REPORT ✅
Testing DetailsResult Details
Now IGNORED Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. |
partiql-spi/src/main/java/org/partiql/spi/value/DatumTimestamp.java
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/java/org/partiql/spi/value/DatumTimestampz.java
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnBetween.kt
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnUtcnow.kt
Outdated
Show resolved
Hide resolved
| // calculate second/nanos | ||
| val ds = second.bigDecimal | ||
| val seconds: Int = ds.toInt() | ||
| val nanoOfSecond: Int = ds.remainder(BigDecimal.ONE).movePointRight(9).toInt() |
There was a problem hiding this comment.
nit: this is used in a few other places. perhaps could be some top-level variable/const?
|
|
||
| @Test | ||
| @Disabled("The planner does not return the v1 plans right now. See assertExpression.") // TODO | ||
| fun testRandomDates() { |
There was a problem hiding this comment.
Why is this test getting removed?
partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt
Outdated
Show resolved
Hide resolved
| @@ -18,6 +18,22 @@ STRICT:::divDecimalInt | |||
| PERMISSIVE:::subtractionOutOfAllowedPrecision | |||
| STRICT:::subtractionOutOfAllowedPrecision | |||
|
|
|||
| // TODO: Datetime methods for EXTRACT for TIME WITH TIMEZONE and TIMESTAMP WITH TIMEZONE | |||
There was a problem hiding this comment.
Are these the conformance tests that were failing w/ this change?
There was a problem hiding this comment.
Yes, some issue introduced with lowering the Ion timestamp.
There was a problem hiding this comment.
I'll cut a separate issue to track these new failing conformance tests -- #1699.
| import java.util.regex.Matcher | ||
| import java.util.regex.Pattern | ||
|
|
||
| internal object DateTimeUtils { |
There was a problem hiding this comment.
(from offline review) this file's logic was kept largely the same as before. We should cleanup the validation + parsing logic -- #1699
| @@ -18,6 +18,22 @@ STRICT:::divDecimalInt | |||
| PERMISSIVE:::subtractionOutOfAllowedPrecision | |||
| STRICT:::subtractionOutOfAllowedPrecision | |||
|
|
|||
| // TODO: Datetime methods for EXTRACT for TIME WITH TIMEZONE and TIMESTAMP WITH TIMEZONE | |||
There was a problem hiding this comment.
I'll cut a separate issue to track these new failing conformance tests -- #1699.
Description
This PR updates Datum to use java.time.
See #1654
Other Information
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
< If YES, which ones and why? >
< In addition, please also mention any other alternatives you've considered and the reason they've been discarded >
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.