-
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 5 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 |
---|---|---|
|
@@ -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