-
Notifications
You must be signed in to change notification settings - Fork 25k
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
SQL: Fix display size for DATE/DATETIME #40669
Conversation
A full format for a DATETIME would be: `2019-03-30T10:20:30.123456789+10:00` (example when asking for `current_timestamp(9)`) which is 35 chars long. For DATE a full format would be: `2019-03-30T00:00:00.000+10:00` which is 29 chars long.
Pinging @elastic/es-search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left a comment about the protocol tests.
@@ -67,11 +67,16 @@ public void testTextualType() throws IOException { | |||
|
|||
public void testDateTimes() throws IOException { | |||
assertQuery("SELECT CAST('2019-01-14T12:29:25.000Z' AS DATETIME)", "CAST('2019-01-14T12:29:25.000Z' AS DATETIME)", | |||
"datetime", "2019-01-14T12:29:25.000Z", 24); | |||
"datetime", "2019-01-14T12:29:25.000Z", 35); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the precision changed, please change the tests' values or add new tests (preferred) to reflect the increased precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this revealed another bug: #40692
so currently I cannot have easily a test with custom timezone.
Currently we only display down to millis.
/cc @bpintea We keep the precision of second fractional digits to 3 as currently we never return more than that. Added a comment to docs regarding |
@@ -44,11 +44,11 @@ | |||
OBJECT( "object", JDBCType.STRUCT, -1, 0, 0, false, false, false), | |||
NESTED( "nested", JDBCType.STRUCT, -1, 0, 0, false, false, false), | |||
BINARY( "binary", JDBCType.VARBINARY, -1, Integer.MAX_VALUE, Integer.MAX_VALUE, false, false, false), | |||
DATE( JDBCType.DATE, Long.BYTES, 24, 24, false, false, true), | |||
DATE( JDBCType.DATE, Long.BYTES, 3, 29, false, false, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The precision here (3) and the comment below (about display size and precision) are incompatible I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 3
shows how many second fractional digits are used. currently millis -> 3. This was fixed here: 1557d77#diff-a3aa9a4b764c281befa4ee231082c298R51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Thanks. Then the code comment is inaccurate or not necessary?
- The precision is not 23 + 1 anymore
- the timezone is not Z only anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. Thx will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I left a comment.
A full format for a DATETIME would be: `2019-03-30T10:20:30.123+10:00` which is 29 chars long. For DATE a full format would be: `2019-03-30T00:00:00.000+10:00` which is also 29 chars long. (cherry picked from commit 6be8396)
A full format for a DATETIME would be: `2019-03-30T10:20:30.123+10:00` which is 29 chars long. For DATE a full format would be: `2019-03-30T00:00:00.000+10:00` which is also 29 chars long. (cherry picked from commit 6be8396)
A full format for a DATETIME would be: `2019-03-30T10:20:30.123+10:00` which is 29 chars long. For DATE a full format would be: `2019-03-30T00:00:00.000+10:00` which is also 29 chars long. (cherry picked from commit 6be8396)
A full format for a DATETIME would be: `2019-03-30T10:20:30.123+10:00` which is 29 chars long. For DATE a full format would be: `2019-03-30T00:00:00.000+10:00` which is also 29 chars long. (cherry picked from commit 6be8396)
…leniency * elastic/master: SQL: Fix deserialisation issue of TimeProcessor (elastic#40776) Improve GCS docs for using keystore (elastic#40605) Add Restore Operation to SnapshotResiliencyTests (elastic#40634) Small refactorings to analysis components (elastic#40745) SQL: Fix display size for DATE/DATETIME (elastic#40669) add HLRC protocol tests for transform state and stats (elastic#40766) Inline TransportReplAction#registerRequestHandlers (elastic#40762) remove experimental label from search_as_you_type documentation (elastic#40744) Remove some abstractions from `TransportReplicationAction` (elastic#40706) Upgrade to latest build scan plugin (elastic#40702) Use default memory lock setting in testing (elastic#40730) Add Bulk Delete Api to BlobStore (elastic#40322) Remove yaml skips older than 7.0 (elastic#40183) Docs: Move id in the java-api (elastic#40748)
A full format for a DATETIME would be: `2019-03-30T10:20:30.123+10:00` which is 29 chars long. For DATE a full format would be: `2019-03-30T00:00:00.000+10:00` which is also 29 chars long.
A full format for a DATETIME would be:
2019-03-30T10:20:30.123456789+10:00
(example when asking forcurrent_timestamp(9)
) which is 35 chars long.For DATE a full format would be:
2019-03-30T00:00:00.000+10:00
which is 29 chars long.