SQL: Extract SQL request and response classes #30457

imotov · 2018-05-08T13:40:37Z

Extracts SQL request and response classes. This is the first step
towards creation of a small minimal dependencies jdbc driver.

Relates #29856

Extracts SQL request and response classes. This is the first step towards creation of a small minimal dependencies jdbc driver. Relates elastic#29856

elasticmachine · 2018-05-08T13:40:38Z

Pinging @elastic/es-search-aggs

nik9000 · 2018-05-08T14:38:38Z

...ugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlTranslateRequest.java

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        // This is needed just to test parsing of SqlTranslateRequest, so we can reuse SqlQuerySerialization


What if you remove the xcontent stuff from the requests and responses entirely and always use the objects from the sql-proto jar for that. On the way in you'd parse using the proto objects and then build this request from the proto object. On the way out you'd use a response to build the proto object and then let it render the xcontent. No more funny round trip testing.

@nik9000 then we are going back to all the weirdness with QueryBuilder that you so didn't like in the previous iteration.

Nah, I don't mean that the Request and Response should contain the proto objects, but that they should be built from them or build them.

I see now - we can't parse the XContent in proto because it contains a QueryBuilder.

imotov · 2018-05-08T18:24:38Z

retest this please

nik9000

Ok. I get it now. I think this is indeed better than the other way. Do you want to merge this as is and then make a followup to shuffle things around and remove the deps?

nik9000 · 2018-05-08T18:46:50Z

...ugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlTranslateRequest.java

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        // This is needed just to test parsing of SqlTranslateRequest, so we can reuse SqlQuerySerialization


I see now - we can't parse the XContent in proto because it contains a QueryBuilder.

costin

Left some minor comments; LGTM

costin · 2018-05-10T04:30:45Z

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

@@ -43,7 +43,7 @@
    private String catalog;
    private String schema;

-    public JdbcConnection(JdbcConfiguration connectionInfo) throws SQLException {
+    public JdbcConnection(JdbcConfiguration connectionInfo) {


The SQLException is the only type of Exception the JDBC API can throw (and that the user expects).
If we remove it, we need to make sure no other types of Exceptions (runtime or otherwise) are thrown - is that the case here?
Otherwise they need to be converted.

OK. I will bring it back and add a comment.

costin · 2018-05-10T04:31:52Z

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

@@ -7,7 +7,7 @@

 import org.elasticsearch.xpack.sql.jdbc.net.client.Cursor;
 import org.elasticsearch.xpack.sql.jdbc.net.client.RequestMeta;
-import org.elasticsearch.xpack.sql.plugin.SqlTypedParamValue;
+import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue;


+1 for the package change.

costin · 2018-05-10T04:35:36Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java

+ */
+public enum Mode {
+    PLAIN,
+    JDBC;


I propose to rename JDBC to something like DRIVER or METADATA since it's used by ODBC as well (can be a follow-up since this PR passes).

Yes, I think it is out of scope for this PR.

costin · 2018-05-10T04:45:31Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Protocol.java

+    /**
+     * Global choice for the default fetch size.
+     */
+    public static final int FETCH_SIZE = 1000;


We need to be careful in externalizing the defaults as outside the initial config, we should always be explicit about the values of these parameters since they are user configurable.

I just moved them from the original request here since we now have 2 requests. We can address this, but I think it should be in another PR, since this will change the protocol as well (these parameters are used not only for the initial client config, but also on the server side as part of the protocol. For example, we don't serialize these parameters if they have default values.)

imotov

Will push an update in a bit.

imotov · 2018-05-10T19:01:35Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java

+ */
+public enum Mode {
+    PLAIN,
+    JDBC;


Yes, I think it is out of scope for this PR.

imotov · 2018-05-10T19:02:07Z

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

@@ -43,7 +43,7 @@
    private String catalog;
    private String schema;

-    public JdbcConnection(JdbcConfiguration connectionInfo) throws SQLException {
+    public JdbcConnection(JdbcConfiguration connectionInfo) {


OK. I will bring it back and add a comment.

imotov · 2018-05-10T19:06:47Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Protocol.java

+    /**
+     * Global choice for the default fetch size.
+     */
+    public static final int FETCH_SIZE = 1000;


I just moved them from the original request here since we now have 2 requests. We can address this, but I think it should be in another PR, since this will change the protocol as well (these parameters are used not only for the initial client config, but also on the server side as part of the protocol. For example, we don't serialize these parameters if they have default values.)

imotov · 2018-05-11T18:07:49Z

retest this please

…t-jdbc-protocol-classes

Extracts SQL request and response classes. This is the first step towards creation of a small minimal dependencies jdbc driver. Relates #29856

* 6.x: Revert "Silence IndexUpgradeIT test failures. (#30430)" [DOCS] Remove references to changelog and to highlights Revert "Mute ML upgrade test (#30458)" [ML] Fix BWC version for backport of #30125 [Docs] Improve section detailing translog usage (#30573) [Tests] Relax allowed delta in extended_stats aggregation (#30569) Fail if reading from closed KeyStoreWrapper (#30394) [ML] Reverse engineer Grok patterns from categorization results (#30125) Derive max composite buffers from max content len Update build file due to doc file rename SQL: Extract SQL request and response classes (#30457) Remove the changelog (#30593) Revert "Add deprecation warning for default shards (#30587)" Silence IndexUpgradeIT test failures. (#30430) Add deprecation warning for default shards (#30587) [DOCS] Adds 6.4.0 release highlight pages [DOCS] Adds release highlight pages (#30590) Docs: Document how to rebuild analyzers (#30498) [DOCS] Fixes title capitalization in security content LLRest: Add equals and hashcode tests for Request (#30584) [DOCS] Fix realm setting names (#30499) [DOCS] Fix path info for various security files (#30502) Docs: document precision limitations of geo_bounding_box (#30540) Fix non existing javadocs link in RestClientTests Auto-expand replicas only after failing nodes (#30553)

SQL: Extract SQL request and response classes

86897dd

Extracts SQL request and response classes. This is the first step towards creation of a small minimal dependencies jdbc driver. Relates elastic#29856

imotov added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.4.0 labels May 8, 2018

imotov requested review from dakrone, costin and nik9000 May 8, 2018 13:40

imotov added >non-issue and removed >enhancement labels May 8, 2018

nik9000 reviewed May 8, 2018

View reviewed changes

nik9000 approved these changes May 8, 2018

View reviewed changes

costin approved these changes May 10, 2018

View reviewed changes

imotov commented May 10, 2018

View reviewed changes

Restore SQLExceptions for JDBC client

900c608

Merge remote-tracking branch 'elastic/master' into issue-29856-extrac…

7812ce5

…t-jdbc-protocol-classes

imotov merged commit 56d32bc into elastic:master May 14, 2018

imotov added a commit that referenced this pull request May 15, 2018

SQL: Extract SQL request and response classes (#30457)

63700fe

Extracts SQL request and response classes. This is the first step towards creation of a small minimal dependencies jdbc driver. Relates #29856

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

imotov deleted the issue-29856-extract-jdbc-protocol-classes branch May 1, 2020 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Extract SQL request and response classes #30457

SQL: Extract SQL request and response classes #30457

imotov commented May 8, 2018

elasticmachine commented May 8, 2018

nik9000 May 8, 2018

imotov May 8, 2018

nik9000 May 8, 2018

nik9000 May 8, 2018

imotov commented May 8, 2018

nik9000 left a comment

nik9000 May 8, 2018

costin left a comment

costin May 10, 2018

imotov May 10, 2018

costin May 10, 2018

costin May 10, 2018

imotov May 10, 2018

costin May 10, 2018

imotov May 10, 2018

imotov left a comment

imotov May 10, 2018

imotov May 10, 2018

imotov May 10, 2018

imotov commented May 11, 2018

SQL: Extract SQL request and response classes #30457

SQL: Extract SQL request and response classes #30457

Conversation

imotov commented May 8, 2018

elasticmachine commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imotov commented May 8, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imotov commented May 11, 2018