-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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: Enhance timestamp escaped literal parsing #52097
Conversation
Allow also whitespace ` ` (together with `T`) as a separator between date and time parts of the timestamp string. E.g.: ``` 2020-02-08 12.10.45 ``` or ``` 2020-02-08T12.10.45 ``` Fixes: elastic#46049
Pinging @elastic/es-search (:Search/SQL) |
Folks, do you think we could treat it as a bug fix and backport it to previous versions? |
@@ -105,7 +111,11 @@ public static ZonedDateTime asDateTime(String dateFormat) { | |||
} | |||
|
|||
public static ZonedDateTime ofEscapedLiteral(String dateFormat) { | |||
return ZonedDateTime.parse(dateFormat, DATE_TIME_ESCAPED_LITERAL_FORMATTER.withZone(UTC)); | |||
try { |
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.
This could be improved by checking whether there's a T
separator at its dedicated position (10) in order to select the right parser. This should avoid the fallback in case of an exception.
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 position is not dedicated though as the year can be more than 4 digits.
But I can use lastIndexOf('-')
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.
P.S. The referred issue seems to be incorrect.
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.
Any other formats weren't supposed to be handled by a more general functionality in which a format could be specified to convert a date to that format?
Like a TO_DATE
like function.
This is just to properly support the ODBC escaped timestamp literals: https://docs.microsoft.com/en-us/sql/odbc/microsoft/jet-date-time-and-timestamp-literals?view=sql-server-ver15 We can still enhance the overall functionality by using introducing a TO_DATE function where the user can specify a custom format. |
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.
Left some comments
boolean hasSecs = randomBoolean(); | ||
String secs = ""; | ||
if (hasSecs) { | ||
secs = ":55"; | ||
} | ||
|
||
String fractionalSecs = ""; | ||
if (hasSecs) { | ||
fractionalSecs = randomFrom("", ".1", ".12", ".123", ".1234", ".12345", ".123456", | ||
".1234567", ".12345678", ".123456789"); | ||
} | ||
return secs + fractionalSecs; |
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.
I think this whole code could be simplified (and still readable and not cluttered) as:
if (randomBoolean()) {
return ":55" + randomFrom("", ".1", ".12", ".123", ".1234", ".12345", ".123456",
".1234567", ".12345678", ".123456789");
}
return "";
int length = randomIntBetween(4, 9); | ||
|
||
if (randomBoolean()) { | ||
sb.append('-'); |
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.
I don't understand this. Why is the date starting with -
?
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.
To test negative Year.
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
Allow also whitespace
(together with
T
) as a separator betweendate and time parts of the timestamp string. E.g.:
or
Fixes: #46069