SNOW-1812949 add get object and get bytes support for native arrow structured types #1968
Conversation
…ormat ARROW is set
…, add variant test case
| return createJsonSqlInput(columnIndex, obj); | ||
| } else if (converter instanceof StructConverter) { | ||
| return createArrowSqlInput(columnIndex, (Map<String, Object>) obj); | ||
| if (type == Types.STRUCT && isStructuredType && converter instanceof VarCharConverter) { |
There was a problem hiding this comment.
Could we extract these conditions?
There was a problem hiding this comment.
You mean something like isVarcharConvertedStructuredType() function? I also consider some rearrangement of conditions but not sure how to make it pretty:
if (type != Types.STRUCT) {
return obj;
}
if (!resultSetMetaData.isStructuredTypeColumn(columnIndex)) {
return obj;
}
if (!(converter instanceof VarCharConverter)) {
return obj;
}
if (obj != null) {
return new StructObjectWrapper((String) obj, createJsonSqlInput(columnIndex, obj));
}
return null;
| return text; | ||
| } | ||
|
|
||
| private static String buildJsonStringFromElements(Object elements) throws SQLException { | ||
| try { | ||
| return SnowflakeUtil.mapJson(elements); |
There was a problem hiding this comment.
Here, we create JSON using ObjectMapper. Shouldn't it be created by our types mappers?
There was a problem hiding this comment.
I think this method is used for both JSON and ARROW data. For arrow structured types the text field will always be set so it won't be executed. Now I think that it may be worth to check for potential divergence in primitive types though
| private int baseType; | ||
| private Object elements; | ||
|
|
||
| public SfSqlArray(String text, int baseType, Object elements) { |
There was a problem hiding this comment.
- can we reuse new constructor in other constructors?
- can we remove other constructors?
There was a problem hiding this comment.
Good catch, I'll reuse it but also keep the old one to avoid passing null in usages that remain untouched
| public void testRunAsGetString() throws SQLException { | ||
| withFirstRow( | ||
| connections.get(queryResultFormat), | ||
| selectSql, | ||
| (resultSet) -> assertGetStringIsCompatible(resultSet, expectedStructureTypeRepresentation)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testRunAsGetObject() throws SQLException { |
There was a problem hiding this comment.
we are fetching and getting the same data for Object and Bytes - can we run bot (or even more tests) on the same result set?
…y, simplify getObject conditions
Add getObject and getBytes implementations for native arrow structured types
SNOW-1812949
The getString unification was already introduced to JDBC driver. This PR introduces changes for getObject and getBytes functions:
Pre-review self checklist
masterbranchmvn -P check-style validate)mvn verifyand inspecttarget/japicmp/japicmp.html)SNOW-XXXX: