Skip to content
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: protocol returns ISO 8601 String formatted dates instead of Long for JDBC/ODBC requests #36800

Merged
merged 3 commits into from Dec 19, 2018

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Dec 18, 2018

This PR changes the protocol for JDBC/ODBC requests in case of date columns, returning their values as String for ZonedDateTime type of date fields. This changes the current behavior of returning long values representing UTC millis since epoch.
There are still some situations (Histograms) where the returned value is not a ZonedDateTime instance, but a Long representing UTC millis since epoch. For those cases, the behavior remained unchanged.

This addresses #36756.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@astefan
Copy link
Contributor Author

astefan commented Dec 18, 2018

@elasticmachine run default distro tests

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -20,6 +23,8 @@
public static final String SQL_TRACE = "org.elasticsearch.xpack.sql:TRACE";

public static final String JDBC_TIMEZONE = "timezone";

public static ZoneId UTC = ZoneId.of("Z");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why org.es.xpack.sql.util.DateUtils is not used instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not visible in the jdbc project.

import static java.time.temporal.ChronoField.MINUTE_OF_HOUR;
import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;

final class DateUtils {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to JdbcDateUtils to avoid name clashes with the class in the server.
Also add a comment on why this class is here (lack of visibility) since several things from there can be found here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

import static java.time.temporal.ChronoField.MINUTE_OF_HOUR;
import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;

final class DateUtils {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

if (val == null) {
return null;
}
return DateUtils.asDateTimeField(val, DateUtils::asMillisSinceEpoch, (longValue) -> longValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just FYI, you can use Function.identity() for t->t cases

@astefan
Copy link
Contributor Author

astefan commented Dec 19, 2018

run the gradle build tests 1

1 similar comment
@astefan
Copy link
Contributor Author

astefan commented Dec 19, 2018

run the gradle build tests 1

@jasontedor jasontedor added v6.7.0 and removed v6.6.0 labels Dec 19, 2018
@astefan astefan merged commit d31eaf7 into elastic:master Dec 19, 2018
astefan added a commit that referenced this pull request Dec 19, 2018
… for JDBC/ODBC requests (#36800)

* Change the way the protocol returns date fields from Long values in case
of JDBC/ODBC, to ISO 8601 with millis String.

(cherry picked from commit d31eaf7)
astefan added a commit that referenced this pull request Dec 20, 2018
… for JDBC/ODBC requests (#36800)

* Change the way the protocol returns date fields from Long values in case
of JDBC/ODBC, to ISO 8601 with millis String.

(cherry picked from commit d31eaf7)
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants