Repository navigation
support StructInternalRow.getVariant - #14379
Conversation
| if (value instanceof VariantVal) { | ||
| return (VariantVal) value; | ||
| } |
There was a problem hiding this comment.
[doubt] when can this have VariantVal ?
[optional] can we also add a test for it ?
There was a problem hiding this comment.
When the backing StructLike already holds Spark’s native VariantVal, it goes this path.
|
also ping @aihuaxu @amogh-jahagirdar @nastra |
| private VariantVal getVariantInternal(int ordinal) { | ||
| Object value = struct.get(ordinal, Object.class); | ||
|
|
||
| if (value instanceof VariantVal) { |
There was a problem hiding this comment.
As I understand, we will only have VariantVal in InternalRow. In what case do we have Variant object?
There was a problem hiding this comment.
ParentVariantReader returns Iceberg Variant, and the DataTask row path can wrap those records in StructInternalRow, so we can see Variant there.
| throw new UnsupportedOperationException( | ||
| "Unsupported value for VARIANT in StructInternalRow: " + value.getClass()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we also need to add the support in the following get(int ordinal, DataType dataType) and private ArrayData collectionToArrayData(Type elementType, Collection<?> values) - seems for nested variant.
|
|
||
| private static VariantVal toVariantVal(Object value) { | ||
| if (value instanceof VariantVal) { | ||
| return (VariantVal) value; |
There was a problem hiding this comment.
Can you help me understand what kind of data will be present ? From other types such as Map, Struct, seems we should only expect Variant here and then we are converting to VariantVal.
By checking the usage, seems we should expect Variant and we should not see VariantVal?
There was a problem hiding this comment.
In test, we can construct GenericRecord with Spark-native values.
I am not sure if there are any real usage. I am OK to remove this.
There was a problem hiding this comment.
@aokolnychyi Can you help take a look? I think we can remove.
There was a problem hiding this comment.
I removed this conditional branch.
aihuaxu
left a comment
aihuaxu
left a comment
There was a problem hiding this comment.
LGTM. Thanks @huaxingao working on this improvement.
|
Thanks @aihuaxu @singhpk234 for the review! |
* support StructInternalRow.getVariant * add a test for GetVariantPassesThroughVariantVal * address comments * remove path for VariantVal
* support StructInternalRow.getVariant * add a test for GetVariantPassesThroughVariantVal * address comments * remove path for VariantVal
3 participants
No description provided.