JDBC driver prepared statement set* methods by astefan · Pull Request #31494 · elastic/elasticsearch

astefan · 2018-06-21T07:00:44Z

…ded the byte[] setter.

…_types5

costin · 2018-06-21T07:45:22Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

    @Override
    public void setBytes(int parameterIndex, byte[] x) throws SQLException {
-        throw new UnsupportedOperationException("Bytes not implemented yet");
+        setObject(parameterIndex, x);


Why isn't Types specified as in the methods above?

costin · 2018-06-21T07:48:16Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+            return;
+        }
+
+        setDate(parameterIndex, new Date(TypeConverter.convertFromCalendarToUTC(x.getTime(), cal)));


This should call setObject directly not setDate (with 2 params) which effectively is setDate(paramIndex,x,NULL).
The 2 arg method should invoke the 3 arg one not vice-versa since the former is a subset of the latter.

costin · 2018-06-21T07:48:25Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+            return;
+        }
+
+        setTime(parameterIndex, new Time(TypeConverter.convertFromCalendarToUTC(x.getTime(), cal)));


Same comments as above.

costin · 2018-06-21T07:48:35Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

    @Override
    public void setTimestamp(int parameterIndex, Timestamp x, Calendar cal) throws SQLException {
-        throw new UnsupportedOperationException("Dates not implemented yet");
+        if (cal == null) {


costin · 2018-06-21T07:50:31Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+            // this is also a way to check early for the validity of the desired sql type
+            targetJDBCType = JDBCType.valueOf(targetSqlType);
+        } catch (IllegalArgumentException e) {
+            throw new SQLException(e.getMessage());


The exception should be more precise, either SQLDataException or its parent SQLNonTransientException.
The same comment applies for the rest of the method.

costin · 2018-06-21T07:54:09Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+    }
+
+    private Calendar getDefaultCalendar() {
+        return Calendar.getInstance(cfg.timeZone(), Locale.ROOT);


This used to be a final field, why was it moved to a method?

costin · 2018-06-21T07:56:35Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

        throw new SQLFeatureNotSupportedException("Batching not supported");
    }
+
+    private boolean checkNull(int parameterIndex, Object o, int type) throws SQLException {


The method name seems incorrect since it's not a check but a set. Something like setIfNull or handleNull is more appropriate.

costin · 2018-06-21T09:39:30Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/TypeConverter.java

+        Calendar c = (Calendar) cal.clone();
+        c.setTimeInMillis(value);
+
+        ZonedDateTime convertedDateTime = ZonedDateTime


There should be a way to do the conversion without doing the raw math in there (not to mention the whole /1000, * 1000 losses approximation).

Why not use the ZoneId from the calendar to compute the offset:

ZonedDateTime zdt = Instant.ofEpochMillis(value).atZone(cal.getTimeZone().toZoneId());

or potentially use getOffset() instead of getRawOffset()

Changed the way the conversion is done

costin · 2018-06-21T09:40:18Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/TypeConverter.java

     */
    @SuppressWarnings("unchecked")
-    static <T> T convert(Object val, JDBCType columnType, Class<T> type) throws SQLException {
+    static <T> T convert(Object val, JDBCType columnType, Class<T> type) throws SQLException, ClassCastException {


CCE is never a good idea to throw out - especially since it forces the caller to handle it.
It should be handled inside convert directly.

costin · 2018-06-21T09:44:28Z

...sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatementTests.java

+        assertEquals("true", value(jps));
+        assertEquals(VARCHAR, jdbcType(jps));
+
+        SQLException sqle = expectThrows(SQLException.class, () -> jps.setObject(1, true, Types.TIMESTAMP));


This should sit in its own test (setInvalidBooleanObject), something applied to the rest of the test suite.

elasticmachine · 2018-06-21T11:12:22Z

Pinging @elastic/es-search-aggs

…rs are converted before being sent to ES.

costin · 2018-06-22T22:10:02Z

LGTM

nik9000

I left a few small things. I'm super happy about the new test though!

nik9000 · 2018-06-25T13:20:53Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+                || x instanceof Float
+                || x instanceof Double
+                || x instanceof String) 
+        {


Usually we'd stick this on the end of the last line.

nik9000 · 2018-06-25T13:27:25Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+                // converting to {@code java.util.Date} because this is the type supported by {@code XContentBuilder} for serialization
+                java.util.Date dateToSet;
+                if (x instanceof Timestamp) {
+                    dateToSet = new java.util.Date(((Timestamp) x).getTime());


That's round the fractional milliseconds. Are we sure that is right? I think it is worth a comment if we are. If not, worth thinking about some more.

@nik9000 I did it like this since ES deals with dates in milliseconds since epoch. The parameters will be serialized using a java.util.Date when being sent to ES in queries.

Right. I think maybe this is worth playing with some more and documenting any strange things that come out of it. Though I think it'd be fine to do in a followup.

nik9000 · 2018-06-25T13:29:28Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+                || x instanceof LocalDateTime
+                || x instanceof Time
+                || x instanceof java.util.Date) {
+            if (targetJDBCType == JDBCType.TIMESTAMP ) {


Could you do these with a single instanceof? rather than check each one twice? Maybe call setParam(parameterIndex, dateToSet, Types.TIMESTAMP); on the result each time and return?

@nik9000 Haven't found a better/easier-to-follow way of refactoring this block of code mostly due to the inner if-else for the target jdbc type. I have gotten rid of the else if bunch though.

nik9000 · 2018-06-25T13:30:55Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

+        }
+    }
+
+    private void checkKnownUnsupportedTypes(Object x) throws SQLFeatureNotSupportedException {


I feel like it'd be cleaner to have this be the result of falling off the end of an if statement chain.

nik9000 · 2018-06-25T13:32:55Z

...ugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java

        throw new SQLFeatureNotSupportedException("Batching not supported");
    }
+
+    private boolean setIfNull(int parameterIndex, Object o, int type) throws SQLException {


I think it might be easier to read the code without this method to be honest. It saves a line every time you call it makes me go "what is going on here?" every time I see it. Not sure.

nik9000 · 2018-06-25T13:33:56Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/TypeConverter.java

+        // according to B-4 table from the jdbc4.2 spec
+        javaToJDBC.put(Calendar.class, JDBCType.TIMESTAMP);
+        javaToJDBC.put(java.util.Date.class, JDBCType.TIMESTAMP);
+        javaToJDBC.put(LocalDateTime.class, JDBCType.TIMESTAMP);


It'd be nice if the map were unmodifiable.

nik9000 · 2018-06-25T13:35:57Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/TypeConverter.java

+
+
+    static JDBCType fromJavaToJDBC(Class<?> clazz) throws SQLException {
+        for (Class<?> key : javaToJDBC.keySet()) {


I'd prefer iterating over the map entries here.

nik9000 · 2018-06-25T13:36:58Z

...sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatementTests.java

+        assertEquals("Numeric " + randomLongNotShort + " out of range", sqle.getMessage());
+    }
+
+    public void testSettingFloatValues() throws SQLException {


I love these tests!

astefan · 2018-06-26T11:50:46Z

@nik9000 thank you for the review and the valuable comments. I've addressed most of them.

nik9000

LGTM

Added setObject functionality and tests for it

astefan · 2018-06-27T08:11:35Z

Backported to 6.x as well.

* elastic/master: (57 commits) HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (elastic#31215) Remove legacy MetaDataStateFormat (elastic#31603) Add explain API to high-level REST client (elastic#31387) Preserve thread context when connecting to remote cluster (elastic#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (elastic#31494) [TEST] call yaml client close method from test suite (elastic#31591) ingest: Add ignore_missing property to foreach filter (elastic#22147) (elastic#31578) Fix a formatting issue in the docvalue_fields documentation. (elastic#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (elastic#31575) Docs: Clarify sensitive fields watcher encryption (elastic#31551) Watcher: Remove never executed code (elastic#31135) Add support for switching distribution for all integration tests (elastic#30874) Improve robustness of geo shape parser for malformed shapes (elastic#31449) QA: Create xpack yaml features (elastic#31403) Improve test times for tests using `RandomObjects::addFields` (elastic#31556) ...

* master: Docs: Remove duplicate test setup Print output when the name checker IT fails (#31660) Fix syntax errors in get-snapshots docs (#31656) Docs: Fix description of percentile ranks example example (#31652) Add MultiSearchTemplate support to High Level Rest client (#30836) Add test for low-level client round-robin behaviour (#31616) SQL: Refactor package names of sql-proto and sql-shared-proto projects (#31622) Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389) Correct integTest enable logic (#31646) Fix missing get-snapshots docs reference #31645 Do not check for Azure container existence (#31617) Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580) Upgrade gradle wrapper to 4.8 (#31525) Only set vm.max_map_count if greater than default (#31512) Add Get Snapshots High Level REST API (#31537) QA: Merge query-builder-bwc to restart test (#30979) Update reindex.asciidoc (#31626) Docs: Skip xpack snippet tests if no xpack (#31619) mute CreateSnapshotRequestTests HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (#31215) Remove legacy MetaDataStateFormat (#31603) Add explain API to high-level REST client (#31387) Preserve thread context when connecting to remote cluster (#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (#31494) [TEST] call yaml client close method from test suite (#31591)

* 6.x: Docs: Remove duplicate test setup Docs: Fix description of percentile ranks example example (#31652) Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389) Do not check for Azure container existence (#31617) Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580) Remove item listed in 6.3 notes that's not in 6.3 (#31623) remove unused import Upgrade gradle wrapper to 4.8 (#31525) Only set vm.max_map_count if greater than default (#31512) QA: Merge query-builder-bwc to restart test (#30979) Docs: Skip xpack snippet tests if no xpack (#31619) [TEST] Fix RemoteClusterConnectionTests Remove legacy MetaDataStateFormat (#31603) [TEST] call yaml client close method from test suite (#31591) [TEST] Close additional clients created while running yaml tests (#31575) Node selector per client rather than per request (#31471) Preserve thread context when connecting to remote cluster (#31574) Remove redundant 'minimum_should_match' Unify headers for full text queries JDBC driver prepared statement set* methods (#31494) add logging breaking changes from 5.3 to 6.0 (#31583) Fix a formatting issue in the docvalue_fields documentation. (#31563) Improve robustness of geo shape parser for malformed shapes QA: Create xpack yaml features (#31403)

astefan added 7 commits June 21, 2018 07:24

Added setObject functionality and tests for it

8e80894

Minor changes

2746d2e

Added the Calendar conversion to UTC for time based setters. Also, ad…

9022d66

…ded the byte[] setter.

Removed DataType.javaName redundant information

31cbdca

Merge remote-tracking branch 'remotes/origin/master' into jdbc_driver…

8f283da

…_types5

Fixed spacing on one line

4a076cc

Fixed gradle check complaints

0550416

astefan requested review from costin, imotov and nik9000 June 21, 2018 07:00

costin requested changes Jun 21, 2018

View reviewed changes

costin added v7.0.0 v6.4.0 :Analytics/SQL SQL querying labels Jun 21, 2018

Cosmetic changes to test methods. Changed the way dates using Calenda…

2e21907

…rs are converted before being sent to ES.

costin approved these changes Jun 22, 2018

View reviewed changes

nik9000 requested changes Jun 25, 2018

View reviewed changes

Readability improvements

04136cf

nik9000 approved these changes Jun 26, 2018

View reviewed changes

astefan merged commit 400db4f into elastic:master Jun 27, 2018

astefan added a commit that referenced this pull request Jun 27, 2018

JDBC driver prepared statement set* methods (#31494)

70995e9

Added setObject functionality and tests for it

astefan added the >enhancement label Aug 22, 2018

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019



		static JDBCType fromJavaToJDBC(Class<?> clazz) throws SQLException {
		for (Class<?> key : javaToJDBC.keySet()) {

Conversation

astefan commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 21, 2018

Uh oh!

costin commented Jun 22, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Jun 26, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

astefan commented Jun 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants