-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SNOW-1848734 Prevent string creation on getObject call when not necessary #1989
Changes from 8 commits
0c3b19a
f1a6c48
cafd880
601842e
f6b74e3
797c745
1b9057f
9e7d246
6498459
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +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.StructConverter; | ||
import net.snowflake.client.core.arrow.StructObjectWrapper; | ||
import net.snowflake.client.core.arrow.VarCharConverter; | ||
import net.snowflake.client.core.arrow.VectorTypeConverter; | ||
|
@@ -564,32 +565,81 @@ public Timestamp getTimestamp(int columnIndex, TimeZone tz) throws SFException { | |
|
||
@Override | ||
public Object getObject(int columnIndex) throws SFException { | ||
int type = resultSetMetaData.getColumnType(columnIndex); | ||
if (type == SnowflakeUtil.EXTRA_TYPES_VECTOR) { | ||
int columnType = resultSetMetaData.getColumnType(columnIndex); | ||
if (columnType == SnowflakeUtil.EXTRA_TYPES_VECTOR) { | ||
return getString(columnIndex); | ||
} | ||
ArrowVectorConverter converter = currentChunkIterator.getCurrentConverter(columnIndex - 1); | ||
|
||
ArrowVectorConverter converter = getConfiguredConverter(columnIndex); | ||
int index = currentChunkIterator.getCurrentRowInRecordBatch(); | ||
wasNull = converter.isNull(index); | ||
converter.setTreatNTZAsUTC(treatNTZAsUTC); | ||
converter.setUseSessionTimezone(useSessionTimezone); | ||
converter.setSessionTimeZone(sessionTimeZone); | ||
Object obj = converter.toObject(index); | ||
boolean isStructuredType = resultSetMetaData.isStructuredTypeColumn(columnIndex); | ||
if (isVarcharConvertedStruct(type, isStructuredType, converter)) { | ||
if (obj != null) { | ||
Object obj = converter.toObject(index); | ||
if (obj == null) { | ||
return null; | ||
} | ||
if (columnType == Types.STRUCT && isStructuredType) { | ||
if (converter instanceof VarCharConverter) { | ||
return new StructObjectWrapper((String) obj, createJsonSqlInput(columnIndex, obj)); | ||
} else if (converter instanceof StructConverter) { | ||
return new StructObjectWrapper( | ||
converter.toString(index), | ||
createArrowSqlInput(columnIndex, (Map<String, Object>) obj), | ||
obj); | ||
} | ||
} | ||
return obj; | ||
return new StructObjectWrapper(converter.toString(index), null, obj); | ||
} | ||
|
||
private boolean isVarcharConvertedStruct( | ||
int type, boolean isStructuredType, ArrowVectorConverter converter) { | ||
return type == Types.STRUCT && isStructuredType && converter instanceof VarCharConverter; | ||
@Override | ||
public <T> Object getObject(int columnIndex, Class<T> type) throws SFException { | ||
if (String.class.isAssignableFrom(type)) { | ||
return getObject(columnIndex); | ||
} | ||
|
||
int columnType = resultSetMetaData.getColumnType(columnIndex); | ||
if (columnType == SnowflakeUtil.EXTRA_TYPES_VECTOR) { | ||
return getString(columnIndex); | ||
} | ||
ArrowVectorConverter converter = getConfiguredConverter(columnIndex); | ||
int index = currentChunkIterator.getCurrentRowInRecordBatch(); | ||
wasNull = converter.isNull(index); | ||
Object obj = converter.toObject(index); | ||
boolean isStructuredType = resultSetMetaData.isStructuredTypeColumn(columnIndex); | ||
if (obj == null) { | ||
return null; | ||
} | ||
if (columnType == Types.STRUCT && isStructuredType) { | ||
if (converter instanceof VarCharConverter) { | ||
return new StructObjectWrapper(null, createJsonSqlInput(columnIndex, obj)); | ||
} else if (converter instanceof StructConverter) { | ||
return new StructObjectWrapper( | ||
null, createArrowSqlInput(columnIndex, (Map<String, Object>) obj)); | ||
} | ||
} | ||
return new StructObjectWrapper(null, null, obj); | ||
} | ||
|
||
@SnowflakeJdbcInternalApi | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. private method does not need the internal annotation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch, I'll remove |
||
private ArrowVectorConverter getConfiguredConverter(int columnIndex) throws SFException { | ||
ArrowVectorConverter converter = currentChunkIterator.getCurrentConverter(columnIndex - 1); | ||
converter.setTreatNTZAsUTC(treatNTZAsUTC); | ||
converter.setUseSessionTimezone(useSessionTimezone); | ||
converter.setSessionTimeZone(sessionTimeZone); | ||
|
||
return converter; | ||
} | ||
|
||
private SQLInput createArrowSqlInput(int columnIndex, Map<String, Object> input) | ||
throws SFException { | ||
if (input == null) { | ||
return null; | ||
} | ||
return new ArrowSqlInput( | ||
input, session, converters, resultSetMetaData.getColumnFields(columnIndex)); | ||
} | ||
|
||
private Object createJsonSqlInput(int columnIndex, Object obj) throws SFException { | ||
private SQLInput createJsonSqlInput(int columnIndex, Object obj) throws SFException { | ||
try { | ||
if (obj == null) { | ||
return null; | ||
|
@@ -620,11 +670,7 @@ public Array getArray(int columnIndex) throws SFException { | |
if (converter instanceof VarCharConverter) { | ||
return getJsonArray((String) obj, columnIndex); | ||
} else if (converter instanceof ArrayConverter || converter instanceof VectorTypeConverter) { | ||
StructObjectWrapper structObjectWrapper = (StructObjectWrapper) obj; | ||
return getArrowArray( | ||
structObjectWrapper.getJsonString(), | ||
(List<Object>) structObjectWrapper.getObject(), | ||
columnIndex); | ||
return getArrowArray(converter.toString(), (List<Object>) obj, columnIndex); | ||
} else { | ||
throw new SFException(queryId, ErrorCode.INVALID_STRUCT_DATA); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,6 +113,8 @@ public abstract class SFBaseResultSet { | |
|
||
public abstract Object getObject(int columnIndex) throws SFException; | ||
|
||
public abstract <T> Object getObject(int columnIndex, Class<T> object) throws SFException; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this API looks strange - the result type should be T, not Object There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, in reality this getObject mostly returns StructObjectWrapper which possibly could be generic. I may rename this method since it's used in higher level getObject anyway to better indicate it's purpose |
||
|
||
public Array getArray(int columnIndex) throws SFException { | ||
throw new UnsupportedOperationException(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1399,7 +1399,10 @@ public <T> T getObject(int columnIndex, Class<T> type) throws SQLException { | |
() -> { | ||
StructObjectWrapper structObjectWrapper = | ||
(StructObjectWrapper) sfBaseResultSet.getObject(columnIndex); | ||
return (SQLInput) createJsonSqlInput(columnIndex, structObjectWrapper); | ||
if (structObjectWrapper == null) { | ||
return null; | ||
} | ||
return structObjectWrapper.getSqlInput(); | ||
}); | ||
if (sqlInput == null) { | ||
return null; | ||
|
@@ -1637,23 +1640,25 @@ public <T> Map<String, T> getMap(int columnIndex, Class<T> type) throws SQLExcep | |
int scale = valueFieldMetadata.getScale(); | ||
TimeZone tz = sfBaseResultSet.getSessionTimeZone(); | ||
StructObjectWrapper structObjectWrapper = | ||
(StructObjectWrapper) | ||
SnowflakeUtil.mapSFExceptionToSQLException( | ||
() -> sfBaseResultSet.getObject(columnIndex)); | ||
SnowflakeUtil.mapSFExceptionToSQLException( | ||
() -> (StructObjectWrapper) sfBaseResultSet.getObject(columnIndex, type)); | ||
if (structObjectWrapper == null) { | ||
return null; | ||
} | ||
|
||
Map<String, Object> map = | ||
mapSFExceptionToSQLException( | ||
() -> prepareMapWithValues(structObjectWrapper.getObject(), type)); | ||
mapSFExceptionToSQLException(() -> prepareMapWithValues(structObjectWrapper, 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 parentSqlInput = structObjectWrapper.getSqlInput(); | ||
SQLInput sqlInput = | ||
sfBaseResultSet.createSqlInputForColumn( | ||
entry.getValue(), | ||
structObjectWrapper.getObject().getClass(), | ||
parentSqlInput == null | ||
? structObjectWrapper.getObject().getClass() | ||
: parentSqlInput.getClass(), | ||
columnIndex, | ||
session, | ||
valueFieldMetadata.getFields()); | ||
|
@@ -1812,11 +1817,12 @@ public boolean isWrapperFor(Class<?> iface) throws SQLException { | |
return iface.isInstance(this); | ||
} | ||
|
||
private <T> Map<String, Object> prepareMapWithValues(Object object, Class<T> type) | ||
throws SFException { | ||
if (object instanceof JsonSqlInput) { | ||
private <T> Map<String, Object> prepareMapWithValues( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where are we using type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was already there, in line 1830 to check if it's assignable to SQLData |
||
StructObjectWrapper structObjectWrapper, Class<T> type) throws SFException { | ||
SQLInput sqlInput = structObjectWrapper.getSqlInput(); | ||
if (sqlInput instanceof JsonSqlInput) { | ||
Map map = new HashMap<>(); | ||
JsonNode jsonNode = ((JsonSqlInput) object).getInput(); | ||
JsonNode jsonNode = ((JsonSqlInput) sqlInput).getInput(); | ||
for (Iterator<String> it = jsonNode.fieldNames(); it.hasNext(); ) { | ||
String name = it.next(); | ||
map.put( | ||
|
@@ -1826,14 +1832,14 @@ private <T> Map<String, Object> prepareMapWithValues(Object object, Class<T> typ | |
: SnowflakeUtil.getJsonNodeStringValue(jsonNode.get(name))); | ||
} | ||
return map; | ||
} else if (object instanceof Map) { | ||
return (Map<String, Object>) object; | ||
} else if (structObjectWrapper.getObject() != null) { | ||
return (Map<String, Object>) structObjectWrapper.getObject(); | ||
} else { | ||
throw new SFException(ErrorCode.INVALID_STRUCT_DATA, "Object couldn't be converted to map"); | ||
} | ||
} | ||
|
||
private Object createJsonSqlInput(int columnIndex, StructObjectWrapper obj) throws SFException { | ||
private SQLInput createJsonSqlInput(int columnIndex, StructObjectWrapper obj) throws SFException { | ||
try { | ||
if (obj == null) { | ||
return null; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lot of duplicated code lines between getObject(index) and getObject(index, type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved some part of it to external method