[GitHub] [ignite-3] isapego commented on a diff in pull request #1927: IGNITE-18899 Refactor ClientTuple to wrap BinaryTuple

["isapego (via GitHub)" <gi...@apache.org>]

isapego commented on code in PR #1927:
URL: https://github.com/apache/ignite-3/pull/1927#discussion_r1162504278


##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleParser.java:
##########
@@ -298,15 +303,16 @@ public final float floatValue(int begin, int end) {
      * @return Element value.
      */
     public final double doubleValue(int begin, int end) {
-        switch (end - begin) {
+        int len = end - begin;
+        switch (len) {
             case 0:
                 return 0.0;
             case Float.BYTES:
                 return buffer.getFloat(begin);
             case Double.BYTES:
                 return buffer.getDouble(begin);
             default:
-                throw new BinaryTupleFormatException("Invalid length for a tuple element");
+                throw new BinaryTupleFormatException("Invalid length for a tuple element: " + len);

Review Comment:
   Probably give more information to a user? Though the same information could probably be retrieved from a stack trace, so optional suggestion.
   ```suggestion
                   throw new BinaryTupleFormatException("Invalid length for a double tuple element: " + len);
   ```



##########
modules/api/src/main/java/org/apache/ignite/table/Tuple.java:
##########
@@ -199,7 +202,7 @@ static boolean equals(Tuple firstTuple, Tuple secondTuple) {
      * @param <T>          Default value type.
      * @return Column value if the tuple contains a column with the specified name. Otherwise, {@code defaultValue}.
      */
-    <T> T valueOrDefault(@NotNull String columnName, T defaultValue);
+    @Nullable <T> T valueOrDefault(@NotNull String columnName, T defaultValue);

Review Comment:
   Probably? Or do we mark parameters as `@Nullable` at all?
   ```suggestion
       @Nullable <T> T valueOrDefault(@NotNull String columnName, @Nullable T defaultValue);
   ```



##########
modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientColumnTypeConverter.java:
##########
@@ -24,12 +24,12 @@
  */
 public class ClientColumnTypeConverter {
     /**
-     * Converts column type to wire code.
+     * Converts SQL column type to wire code.
      *
      * @param columnType Column type.
      * @return Wire code.
      */
-    public static int columnTypeToOrdinal(ColumnType columnType) {
+    public static int sqlColumnTypeToOrdinal(ColumnType columnType) {

Review Comment:
   Why not `ColumnType.ordinal()`? Is this because we want to make sure ordinals do not change in future?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org