Replace createTmestampType to createTimestampWithTimeZoneType in TestLongTimestampWithTimeZoneType #26490
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
465e4b2 to
ad473de
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| Arguments.of(9, Long.MIN_VALUE, Long.MAX_VALUE, 1_000), | ||
| Arguments.of(10, Long.MIN_VALUE, Long.MAX_VALUE, 100), | ||
| Arguments.of(11, Long.MIN_VALUE, Long.MAX_VALUE, 10), | ||
| Arguments.of(12, Long.MIN_VALUE, Long.MAX_VALUE, 1)); |
There was a problem hiding this comment.
The parameter name (maxValue) and the actual value don't match.
Please change long to LongTimestampWithTimeZone here instead of test methods.
There was a problem hiding this comment.
Thanks. I modified the MethodSource: testPreviousNextValueEveryPrecisionDataProvider
| @ParameterizedTest | ||
| @MethodSource("testPreviousNextValueEveryPrecisionDataProvider") | ||
| public void testPreviousValueEveryPrecision(int precision, long minValue, long maxValue, long step) | ||
| public void testPreviousValueEveryPrecision(int precision, long minValue, long maxValue, int step) |
There was a problem hiding this comment.
change to int seems not necessary here
There was a problem hiding this comment.
This change makes sense because the 2nd parameter of fromEpochMillisAndFraction is int.
|
|
||
| assertThat(type.getPreviousValue(minValue)) | ||
| long middleRangeEpochMillis = 123_456_789_000_000L; | ||
| int oneMsInPicos = 1_000_000_000; |
There was a problem hiding this comment.
rename to picosOfMilli
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
ad473de to
8591e9b
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
8591e9b to
cc0b4b1
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
cc0b4b1 to
c5d37b1
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
c5d37b1 to
db4be17
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
ee43ce4 to
d5adba7
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| return blockBuilder.buildValueBlock(); | ||
| } | ||
|
|
||
| private static final int PICOS_OF_MILLI = 1_000_000_000; |
There was a problem hiding this comment.
nit: there exists the constant in Timestamps#PICOSECONDS_PER_MILLISECOND, we could static import here
There was a problem hiding this comment.
I didn't know there is already a const for that. thanks
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
7775667 to
6df1fe9
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
karolhor
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
6df1fe9 to
5ec9d4b
Compare
|
@ebyhr @chenjian2664 is there anything more I can improve in this ticket or it's ok now? |
ebyhr
force-pushed
the
fix/26448_replace_createTimestampType
branch
from
5ec9d4b to
f54ea7d
Compare
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
Hi, is there something I can do with this PR? |
3 participants
Description
Fixes #26448
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.