fix timestamp bug#3542
Conversation
Signed-off-by: xinyual <xinyual@amazon.com>
xinyual
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az and
ykmr1224
as code owners
|
|
||
| @Test | ||
| public void testTimestampWithTimeInput() { | ||
| String utcTomorrow = LocalDate.now().plusDays(1).toString(); |
There was a problem hiding this comment.
There may be an edge case this test should be flakey when it was ran at the time point crossing a day.
I think it should use a fixed time via Hook.CURRENT_TIMESTAMP once changing to use the time properties in Calcite.
There was a problem hiding this comment.
Currently we don't use the latest refactor framework so Hook wouldn't work. But after changing it, I will use Hook in this IT.
There was a problem hiding this comment.
Please add a TODO here.
There was a problem hiding this comment.
timestamp(time('12:00:00'), time('12:00:00'))
what is meaning of time+time? should we limit the first argument be date only?
There was a problem hiding this comment.
In v2, we will implicitly convert the time into timestamp. The logic of v2 timestamp is transfer two arguments into timestamp (if we have two.), then extract time part from the second timestamp and then add it to the first timestamp.
|
CI failed for java21, is this a known failure? |
Should be addressed by this fix: #3543 |
|
Could you open an issue? |
| * We need to write our own since we are actually implement timestamp add here | ||
| * (STRING/DATE/TIME/DATETIME/TIMESTAMP) -> TIMESTAMP (STRING/DATE/TIME/DATETIME/TIMESTAMP, | ||
| * STRING/DATE/TIME/DATETIME/TIMESTAMP) -> TIMESTAMP | ||
| * STRING/DATE/TIME/DATETIME/TIMESTAMP) -> TIMESTAMP It's implicitly transferred into timestamp, so |
There was a problem hiding this comment.
please format this java doc for more readable
Signed-off-by: xinyual <xinyual@amazon.com>
Done. #3545 |
* fix timestamp bug Signed-off-by: xinyual <xinyual@amazon.com> * add TODO Signed-off-by: xinyual <xinyual@amazon.com> * change doc Signed-off-by: xinyual <xinyual@amazon.com> --------- Signed-off-by: xinyual <xinyual@amazon.com>
4 participants
Description
This pr fix the timestamp bug when take time argument. It uses the current function properties. We now follow the same logic as
sql/core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunctions.java
Line 880 in e23a61d
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
#3545
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.