[Kernel] Implement getChild for a few remaining column vectors #2133
Conversation
allisonport-db
force-pushed
the
support-get-child
branch
2 times, most recently
from
c87daed to
dd3a925
Compare
allisonport-db
force-pushed
the
support-get-child
branch
from
dd3a925 to
a468fab
Compare
| @@ -120,4 +123,16 @@ public Row getStruct(int rowId) { | |||
| public ArrayValue getArray(int rowId) { | |||
| return (ArrayValue) value; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Seems incomplete to ignore DefaultConstantVector but this should never be used. I think possible options
- Restrict DefaultConstantVector to only allow StructType for null values (for
NonExistentColumnConverter) - Provide this implementation and add a test somewhere (not sure where to put this?)
There was a problem hiding this comment.
Option 1 + restrict it for Map and Array types for non-null values only?
kernel/kernel-api/src/test/java/io/delta/kernel/internal/types/JsonHandlerTestImpl.java
Outdated
Show resolved
Hide resolved
| @@ -120,4 +123,16 @@ public Row getStruct(int rowId) { | |||
| public ArrayValue getArray(int rowId) { | |||
| return (ArrayValue) value; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Option 1 + restrict it for Map and Array types for non-null values only?
| @@ -136,6 +137,17 @@ public MapValue getMap(int rowId) { | |||
| return (MapValue) values[rowId]; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Can DefaultConstantVector be derived from DefaultGenericVector? If yes, then we can reduce the code size bit.
There was a problem hiding this comment.
We could combine them and use a function like "value accessor" possibly? Then all the class does really is cast
There was a problem hiding this comment.
Can't restrict the types then though
There was a problem hiding this comment.
You can add the type checks in DefaultConstantVector constructor (this will just have the constructor)
There was a problem hiding this comment.
Combined them here instead
| /** | ||
| * {@link ColumnVector} wrapper on top of {@link Row} objects. This wrapper allows referencing | ||
| * any nested level column vector from a set of rows. | ||
| * TODO: We should change the {@link io.delta.kernel.defaults.client.DefaultJsonHandler} to |
There was a problem hiding this comment.
is this TODO still needed?
There was a problem hiding this comment.
yes I think so we're still just wrapping rows in DefaultRowBasedColumnarBatch
There was a problem hiding this comment.
Moving the todo to a more appropriate spot
## Description Provides implementations for `getChild` for column vectors that are missing them. ## How was this patch tested? Adds simple tests for `DefaultViewVector` and `DefaultGenericVector` (used by complex types in the JSON handler). #2131 also is based off these changes and uses `getChild` instead of `getStruct` everywhere in the code.
…-io#2133) ## Description Provides implementations for `getChild` for column vectors that are missing them. ## How was this patch tested? Adds simple tests for `DefaultViewVector` and `DefaultGenericVector` (used by complex types in the JSON handler). delta-io#2131 also is based off these changes and uses `getChild` instead of `getStruct` everywhere in the code.
Which Delta project/connector is this regarding?
Description
Provides implementations for
getChildfor column vectors that are missing them.How was this patch tested?
Adds simple tests for
DefaultViewVectorandDefaultGenericVector(used by complex types in the JSON handler).#2131 also is based off these changes and uses
getChildinstead ofgetStructeverywhere in the code.