[#8962] improvement(lance): supports more dataTypes for the Lance rest server#8963
Conversation
|
This PR depends on #8947 |
|
@jerryshao @yuqi1129 could you plz help to review this PR? thx |
| List<VectorSchemaRoot> batches = new ArrayList<>(); | ||
|
|
||
| while (reader.loadNextBatch()) { | ||
| VectorSchemaRoot root = reader.getVectorSchemaRoot(); |
There was a problem hiding this comment.
Why do we abandon the data part?
There was a problem hiding this comment.
I haven't seen it used elsewhere.
| @MethodSource("toGravitinoArguments") | ||
| void testToGravitino(String testName, Field arrowField, Type expectedGravitinoType) { | ||
| Type convertedType = CONVERTER.toGravitino(arrowField); | ||
| assertEquals(expectedGravitinoType, convertedType); |
There was a problem hiding this comment.
Can you verify arrowFileld == CONVERTER.fromGravitino(CONVERTER.toGravitino(arrowField))？
There was a problem hiding this comment.
They will not be equal because they have different return types (plz see the method signature). What's more, there are already UTs for these two methods
There was a problem hiding this comment.
they have different return types
What's the meaning of it?
There was a problem hiding this comment.
CONVERTER.fromGravitino(Type type) returns ArrowType, not an Arrow Field, so we cannot do the verify arrowField == CONVERTER.fromGravitino
yuqi1129
left a comment
yuqi1129
left a comment
There was a problem hiding this comment.
Just a minor comment, other LGTM
| @MethodSource("toGravitinoArguments") | ||
| void testToGravitino(String testName, Field arrowField, Type expectedGravitinoType) { | ||
| Type convertedType = CONVERTER.toGravitino(arrowField); | ||
| assertEquals(expectedGravitinoType, convertedType); |
There was a problem hiding this comment.
they have different return types
What's the meaning of it?
jerryshao
merged commit d8a4ded
into
apache:branch-lance-namepspace-dev
…ce rest server (apache#8963) ### What changes were proposed in this pull request? supports more dataTypes for the Lance rest server ### Why are the changes needed? Fix: apache#8962 ### Does this PR introduce _any_ user-facing change? yes, more dataTypes supported ### How was this patch tested? tests added
…ce rest server (apache#8963) supports more dataTypes for the Lance rest server Fix: apache#8962 yes, more dataTypes supported tests added
3 participants
What changes were proposed in this pull request?
supports more dataTypes for the Lance rest server
Why are the changes needed?
Fix: #8962
Does this PR introduce any user-facing change?
yes, more dataTypes supported
How was this patch tested?
tests added