address review comments

snowflakedb · Nov 14, 2024 · 4a74df7 · 4a74df7
1 parent 475f18d
commit 4a74df7
Show file tree

Hide file tree

Showing 10 changed files with 38 additions and 33 deletions.
diff --git a/src/main/java/net/snowflake/client/core/SFArrowResultSet.java b/src/main/java/net/snowflake/client/core/SFArrowResultSet.java
@@ -26,7 +26,7 @@
 import java.util.stream.Stream;
 import net.snowflake.client.core.arrow.ArrayConverter;
 import net.snowflake.client.core.arrow.ArrowVectorConverter;
-import net.snowflake.client.core.arrow.StructObject;
+import net.snowflake.client.core.arrow.StructObjectWrapper;
 import net.snowflake.client.core.arrow.VarCharConverter;
 import net.snowflake.client.core.arrow.VectorTypeConverter;
 import net.snowflake.client.core.json.Converters;
@@ -580,7 +580,7 @@ public Object getObject(int columnIndex) throws SFException {
       if (obj == null) {
         return null;
       }
-      return new StructObject((String) obj, createJsonSqlInput(columnIndex, obj));
+      return new StructObjectWrapper((String) obj, createJsonSqlInput(columnIndex, obj));
     }
     return obj;
   }
@@ -616,9 +616,9 @@ public Array getArray(int columnIndex) throws SFException {
     if (converter instanceof VarCharConverter) {
       return getJsonArray((String) obj, columnIndex);
     } else if (converter instanceof ArrayConverter || converter instanceof VectorTypeConverter) {
-      StructObject structObject = (StructObject) obj;
+      StructObjectWrapper structObjectWrapper = (StructObjectWrapper) obj;
       return getArrowArray(
-          structObject.getStringJson(), (List<Object>) structObject.getObject(), columnIndex);
+          structObjectWrapper.getJsonString(), (List<Object>) structObjectWrapper.getObject(), columnIndex);
     } else {
       throw new SFException(queryId, ErrorCode.INVALID_STRUCT_DATA);
     }

diff --git a/src/main/java/net/snowflake/client/core/SFJsonResultSet.java b/src/main/java/net/snowflake/client/core/SFJsonResultSet.java
@@ -15,7 +15,7 @@
 import java.sql.Types;
 import java.util.List;
 import java.util.TimeZone;
-import net.snowflake.client.core.arrow.StructObject;
+import net.snowflake.client.core.arrow.StructObjectWrapper;
 import net.snowflake.client.core.json.Converters;
 import net.snowflake.client.jdbc.ErrorCode;
 import net.snowflake.client.jdbc.FieldMetadata;
@@ -88,7 +88,7 @@ public Object getObject(int columnIndex) throws SFException {
 
       case Types.STRUCT:
         if (resultSetMetaData.isStructuredTypeColumn(columnIndex)) {
-          return new StructObject((String) obj, getSqlInput((String) obj, columnIndex));
+          return new StructObjectWrapper((String) obj, getSqlInput((String) obj, columnIndex));
         } else {
           throw new SFException(ErrorCode.FEATURE_UNSUPPORTED, "data type: " + type);
         }

diff --git a/src/main/java/net/snowflake/client/core/arrow/ArrayConverter.java b/src/main/java/net/snowflake/client/core/arrow/ArrayConverter.java
@@ -25,7 +25,7 @@ public ArrayConverter(ListVector valueVector, int vectorIndex, DataConversionCon
 
   @Override
   public Object toObject(int index) throws SFException {
-    return isNull(index) ? null : new StructObject(toString(index), vector.getObject(index));
+    return isNull(index) ? null : new StructObjectWrapper(toString(index), vector.getObject(index));
   }
 
   @Override

diff --git a/src/main/java/net/snowflake/client/core/arrow/MapConverter.java b/src/main/java/net/snowflake/client/core/arrow/MapConverter.java
@@ -29,14 +29,18 @@ public MapConverter(MapVector valueVector, int columnIndex, DataConversionContex
 
   @Override
   public Object toObject(int index) throws SFException {
+    if (isNull(index)) {
+      return null;
+    }
+
     List<JsonStringHashMap<String, Object>> entriesList =
         (List<JsonStringHashMap<String, Object>>) vector.getObject(index);
-    Map map =
+    Map<String, Object> map =
         entriesList.stream()
             .collect(
                 Collectors.toMap(
                     entry -> entry.get("key").toString(), entry -> entry.get("value")));
-    return isNull(index) ? null : new StructObject(toString(index), map);
+    return new StructObjectWrapper(toString(index), map);
   }
 
   @Override

diff --git a/src/main/java/net/snowflake/client/core/arrow/StructConverter.java b/src/main/java/net/snowflake/client/core/arrow/StructConverter.java
@@ -21,7 +21,7 @@ public StructConverter(StructVector vector, int columnIndex, DataConversionConte
 
   @Override
   public Object toObject(int index) throws SFException {
-    return isNull(index) ? null : new StructObject(toString(index), structVector.getObject(index));
+    return isNull(index) ? null : new StructObjectWrapper(toString(index), structVector.getObject(index));
   }
 
   @Override

diff --git a/...flake/client/core/arrow/StructObject.java → ...lient/core/arrow/StructObjectWrapper.java b/...flake/client/core/arrow/StructObject.java → ...lient/core/arrow/StructObjectWrapper.java
@@ -3,17 +3,17 @@
 import net.snowflake.client.core.SnowflakeJdbcInternalApi;
 
 @SnowflakeJdbcInternalApi
-public class StructObject {
-  private final String stringJson;
+public class StructObjectWrapper {
+  private final String jsonString;
   private final Object object;
 
-  public StructObject(String stringJson, Object object) {
-    this.stringJson = stringJson;
+  public StructObjectWrapper(String jsonString, Object object) {
+    this.jsonString = jsonString;
     this.object = object;
   }
 
-  public String getStringJson() {
-    return stringJson;
+  public String getJsonString() {
+    return jsonString;
   }
 
   public Object getObject() {

diff --git a/src/main/java/net/snowflake/client/core/arrow/VectorTypeConverter.java b/src/main/java/net/snowflake/client/core/arrow/VectorTypeConverter.java
@@ -24,8 +24,11 @@ public VectorTypeConverter(
 
   @Override
   public Object toObject(int index) throws SFException {
+    if (isNull(index)) {
+      return null;
+    }
     Object object = vector.getObject(index);
-    return new StructObject(object.toString(), object);
+    return new StructObjectWrapper(object.toString(), object);
   }
 
   @Override

diff --git a/src/main/java/net/snowflake/client/jdbc/SnowflakeBaseResultSet.java b/src/main/java/net/snowflake/client/jdbc/SnowflakeBaseResultSet.java
@@ -45,7 +45,7 @@
 import net.snowflake.client.core.SFBaseResultSet;
 import net.snowflake.client.core.SFBaseSession;
 import net.snowflake.client.core.SFException;
-import net.snowflake.client.core.arrow.StructObject;
+import net.snowflake.client.core.arrow.StructObjectWrapper;
 import net.snowflake.client.core.structs.SQLDataCreationHelper;
 import net.snowflake.client.log.SFLogger;
 import net.snowflake.client.log.SFLoggerFactory;
@@ -1397,8 +1397,8 @@ public <T> T getObject(int columnIndex, Class<T> type) throws SQLException {
         SQLInput sqlInput =
             SnowflakeUtil.mapSFExceptionToSQLException(
                 () -> {
-                  StructObject structObject = (StructObject) sfBaseResultSet.getObject(columnIndex);
-                  return (SQLInput) createJsonSqlInput(columnIndex, structObject);
+                  StructObjectWrapper structObjectWrapper = (StructObjectWrapper) sfBaseResultSet.getObject(columnIndex);
+                  return (SQLInput) createJsonSqlInput(columnIndex, structObjectWrapper);
                 });
         if (sqlInput == null) {
           return null;
@@ -1635,23 +1635,23 @@ public <T> Map<String, T> getMap(int columnIndex, Class<T> type) throws SQLExcep
     int columnType = ColumnTypeHelper.getColumnType(valueFieldMetadata.getType(), session);
     int scale = valueFieldMetadata.getScale();
     TimeZone tz = sfBaseResultSet.getSessionTimeZone();
-    StructObject object =
-        (StructObject)
+    StructObjectWrapper structObjectWrapper =
+        (StructObjectWrapper)
             SnowflakeUtil.mapSFExceptionToSQLException(
                 () -> sfBaseResultSet.getObject(columnIndex));
-    if (object == null) {
+    if (structObjectWrapper == null) {
       return null;
     }
     Map<String, Object> map =
-        mapSFExceptionToSQLException(() -> prepareMapWithValues(object.getObject(), type));
+        mapSFExceptionToSQLException(() -> prepareMapWithValues(structObjectWrapper.getObject(), type));
     Map<String, T> resultMap = new HashMap<>();
     for (Map.Entry<String, Object> entry : map.entrySet()) {
       if (SQLData.class.isAssignableFrom(type)) {
         SQLData instance = (SQLData) SQLDataCreationHelper.create(type);
         SQLInput sqlInput =
             sfBaseResultSet.createSqlInputForColumn(
                 entry.getValue(),
-                object.getObject().getClass(),
+                structObjectWrapper.getObject().getClass(),
                 columnIndex,
                 session,
                 valueFieldMetadata.getFields());
@@ -1831,14 +1831,14 @@ private <T> Map<String, Object> prepareMapWithValues(Object object, Class<T> typ
     }
   }
 
-  private Object createJsonSqlInput(int columnIndex, StructObject obj) throws SFException {
+  private Object createJsonSqlInput(int columnIndex, StructObjectWrapper obj) throws SFException {
     try {
       if (obj == null) {
         return null;
       }
-      JsonNode jsonNode = OBJECT_MAPPER.readTree(obj.getStringJson());
+      JsonNode jsonNode = OBJECT_MAPPER.readTree(obj.getJsonString());
       return new JsonSqlInput(
-          obj.getStringJson(),
+          obj.getJsonString(),
           jsonNode,
           session,
           sfBaseResultSet.getConverters(),

diff --git a/src/main/java/net/snowflake/client/jdbc/SnowflakeResultSetV1.java b/src/main/java/net/snowflake/client/jdbc/SnowflakeResultSetV1.java
@@ -34,7 +34,7 @@
 import net.snowflake.client.core.SFBaseResultSet;
 import net.snowflake.client.core.SFException;
 import net.snowflake.client.core.SfSqlArray;
-import net.snowflake.client.core.arrow.StructObject;
+import net.snowflake.client.core.arrow.StructObjectWrapper;
 import net.snowflake.client.log.SFLogger;
 import net.snowflake.client.log.SFLoggerFactory;
 
@@ -276,8 +276,8 @@ public Object getObject(int columnIndex) throws SQLException {
       return null;
     } else if (object instanceof JsonSqlInput) {
       return ((JsonSqlInput) object).getText();
-    } else if (object instanceof StructObject) {
-      return ((StructObject) object).getStringJson();
+    } else if (object instanceof StructObjectWrapper) {
+      return ((StructObjectWrapper) object).getJsonString();
     } else if (object instanceof SfSqlArray) {
       return ((SfSqlArray) object).getJsonString();
     } else if (object instanceof ArrowSqlInput) {

diff --git a/...owflake/client/jdbc/structuredtypes/StructuredTypesGetStringArrowJsonCompatibilityIT.java b/...owflake/client/jdbc/structuredtypes/StructuredTypesGetStringArrowJsonCompatibilityIT.java
@@ -54,7 +54,6 @@ public static void closeConnections() throws SQLException {
   }
 
   @Test
-  @ConditionalIgnoreRule.ConditionalIgnore(condition = RunningOnGithubAction.class)
   public void testRunAsGetString() throws SQLException {
     withFirstRow(
         connections.get(queryResultFormat),
@@ -63,7 +62,6 @@ public void testRunAsGetString() throws SQLException {
   }
 
   @Test
-  @ConditionalIgnoreRule.ConditionalIgnore(condition = RunningOnGithubAction.class)
   public void testRunAsGetObject() throws SQLException {
     withFirstRow(
         connections.get(queryResultFormat),