ResultSet API for TarantoolClient #223

nicktorwald · 2019-10-11T19:08:05Z

Extend an TarantoolClient API to be able to get data using type safe
methods and conversions.

Closes: #211

Totktonada · 2019-11-18T23:49:30Z

I would consider several points:

A user may want to instruct a driver to forbid implicit conversions which would loss precision. Examples: some integer values larger that 2^53 (or lesser the -2^53) have no exact representation as doubles (the similar also boundaries exist for 32-bit floats); floating-point values may loss its fractional part when they are converted to integral values.
A user may want to restrict cross-type conversions w/o precision loss such as boolean <-> String to protect self from wrong interpretation of unexpected values. Example: a user want to get an error if a migration, which moves a string column to an index where was a boolean column, fails (get an error at getString() call for a boolean column).
A user may want to treat a row as an array of specific size: it may be an actual tuple size, a user specified value (including an infinite size) or a field_count value from a schema. This may ease interaction with data without a strict schema: say, when they are stored in the way similar to provided by tarantool/document.

I think we should behave in a most strict way by default, but allow to relax restrictions when asked for this explicitly. We should describe which kinds of precision loss or implicit type changes will be enabled by each of provided options.

I think a client configuration options is the proper places for those options.

OTOH, I would not want to provide more options then necessary (just to keep things as simple as possible). If there are reasons to omit some of proposed options, then we should consider them.

NB: I always afraid of data losses or misinterpretations, especially in context of databases, so I don't sure where an adequate balance between mistake protection and usage convenience exists. In other words, I would appreciate a critical view on my points.

Totktonada · 2019-11-25T09:36:14Z

A user may want to treat a row as an array of specific size: it may be an actual tuple size, a user specified value (including an infinite size) or a field_count value from a schema.

It seems that when field_count is specified, all tuples are of the same size (null's are encoded explicitly where needed). So we don't need to handle this case specifically. Using of an actual tuple size will give the same behaviour.

This DSL includes set of request builders that can be used as a public API to construct requests. The main idea here is to provide more natural DSL-like approach to build operations instead of current abstract types like List<?> or List<Object>. Closes: #212

Totktonada · 2019-12-03T23:45:50Z

About first two points re numeric precision loss and cross-type conversions: let's implement those conversions for now, because they may be required for JDBC part of the driver. We can forbid them later under an option if there will be demand.

This commit introduces a new API to handle TarantoolClient result. The concept is similar to the JDBC ResultSet in terms of a getting the data using rows ans columns. Instead of a guessing-style processing the result via List<?>, TarantoolResultSet offers set of typed methods to retrieve the data or get an error if the result cannot be represented as the designated type. Latter case requires to declare formal rules of a casting between the types. In scope of this commit it is supported 11 standard types and conversions between each other. These types are byte, short, int, long, float, double, boolean, BigInteger, BigDecimal, String, and byte[]. Closes: #211

coveralls · 2019-12-17T10:04:47Z

Coverage decreased (-0.6%) to 77.054% when pulling ce50492 on nicktorwald/gh-211-resultset-api into 4ba88fb on master.

akudiyar

The diff is large and has many comments on it. If it will be considered for modifying and pushing, I'd recommend splitting it into several PRs.

akudiyar · 2020-06-11T12:43:50Z

README.md

+the same result. 
+
+```java
+TarantoolResultSet result = client.executeRequest(Requests.selectRequest("space", "pk"));


This is a good idea, although it's better to provide the exact way of mapping MsgPack entities to resulting structures (List, Map or some custom object) in some kind of mapping layer, where the mapping may be customized. Ofc, the driver will provide some default way of mapping, say, into TarantoolResultSet objects.

akudiyar · 2020-06-11T12:47:07Z

README.md

+TarantoolResultSet result = client.executeRequest(Requests.selectRequest("space", "pk"));
+while (result.next()) {
+    long id = result.getLong(0);
+    String text = result.getString(1);


NPE here (or further) is inevitable. The underlying object is essentially a MsgPack array with a non-fixed length. So we should either return an Optional here or provide some kind of interface with checked and unchecked calls, like checked get with Optional or throwing some kind of ArrayOutOfBounds exception, and unchecked getOrDefault.

akudiyar · 2020-06-11T12:49:15Z

README.md

+
+```java
+TarantoolResultSet result = client.executeRequest(Requests.selectRequest("space", "pk"));
+while (result.next()) {


Modern (1.8+) Java code is rarely dealing with bare iterators -- streams, for-each loops and their functional mirrors are the common practice.

akudiyar · 2020-06-11T12:50:39Z

README.md

+```java
+TarantoolResultSet result = client.executeRequest(Requests.selectRequest("space", "pk"));
+while (result.next()) {
+    long id = result.getLong(0);


Do we have unsafe casts here? Or the user will get some kind of an unchecked exception?

akudiyar · 2020-06-11T13:03:55Z

README.md

+
+Numeric types internally can represent each other if a type range allows to do it. For example,
+byte 100 can be represented as a short, int and other types wider than byte. But 200 integer
+cannot be narrowed to a byte because of overflow (byte range is [-128..127]). If a floating


Will the conversion be performed actually when calling getInt/getFloat?

But in this case we will have double conversion with two sources of possible errors: MessagePack entity-to-object and then object-to-value conversions -- not very safe and not very fast. Also the conversion and overflow/underflow shouldn't be implicit without user interference and checked exceptions.

akudiyar · 2020-06-11T15:00:54Z

src/main/java/org/tarantool/dsl/Operator.java

+        return Stream.of(Operator.values())
+            .filter(s -> s.getOpCode().equals(opCode))
+            .findFirst()
+            .orElseThrow(IllegalArgumentException::new);


Exception message is missing

akudiyar · 2020-06-11T15:06:02Z

src/main/java/org/tarantool/dsl/Requests.java

+ * Entry point to build requests
+ * using DSL approach a.k.a Request DSL.
+ */
+public class Requests {


Each request DSL interface offers some kind of request building logic and convenient result processing API instead of function calls with a bunch of parameters. See, for example, how typesafe JPA Criteria API is built or take a look at Querydsl.
IMO, this interface improves select methods a bit but neither is a DSL nor removes the boilerplate completely.

akudiyar · 2020-06-11T15:09:51Z

src/main/java/org/tarantool/dsl/SelectRequestSpec.java

+
+    public SelectRequestSpec index(int indexId) {
+        this.indexId = indexId;
+        this.indexName = null;


That mutability again

akudiyar · 2020-06-11T15:10:13Z

src/main/java/org/tarantool/dsl/SelectRequestSpec.java

+        this.indexName = Objects.requireNonNull(indexName);
+    }
+
+    public SelectRequestSpec(String spaceName, int indexId) {


the index must have a separate specification

akudiyar · 2020-06-11T15:13:19Z

src/test/java/org/tarantool/ClientResultSetIT.java

+
+    @Test
+    void testGetSimpleRows() {
+        testHelper.executeLua(


(not in this request) Consider using test-containers for setting up the integration test environment.

akudiyar · 2020-06-11T15:23:53Z

I also strongly recommend running a style checker for this code, there are some places with double semicolons and other leftovers.

Totktonada mentioned this pull request Oct 18, 2019

Soft automatic schema reload #200

Merged

nicktorwald force-pushed the nicktorwald/gh-211-resultset-api branch from 8fd6068 to 416a303 Compare December 17, 2019 09:31

nicktorwald force-pushed the nicktorwald/gh-211-resultset-api branch from 416a303 to ce50492 Compare December 17, 2019 09:41

akudiyar reviewed Jun 11, 2020

View reviewed changes

akudiyar mentioned this pull request Jun 11, 2020

Introduce Tarantool client DSL #247

Closed

dkasimovskiy closed this Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ResultSet API for TarantoolClient #223

ResultSet API for TarantoolClient #223

nicktorwald commented Oct 11, 2019

Totktonada commented Nov 18, 2019

Totktonada commented Nov 25, 2019

Totktonada commented Dec 3, 2019

coveralls commented Dec 17, 2019

akudiyar left a comment

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020 •

edited

Loading

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020

akudiyar Jun 11, 2020

akudiyar commented Jun 11, 2020

ResultSet API for TarantoolClient #223

ResultSet API for TarantoolClient #223

Conversation

nicktorwald commented Oct 11, 2019

Totktonada commented Nov 18, 2019

Totktonada commented Nov 25, 2019

Totktonada commented Dec 3, 2019

coveralls commented Dec 17, 2019

akudiyar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akudiyar Jun 11, 2020 • edited Loading

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

Choose a reason for hiding this comment

akudiyar commented Jun 11, 2020

akudiyar Jun 11, 2020 •

edited

Loading