API: Add Unit tests for StructProjection class#15984
API: Add Unit tests for StructProjection class#15984Govindarajan-D wants to merge 2 commits intoapache:mainfrom
Conversation
| public void testSubsetProjection() { | ||
| Schema dataSchema = | ||
| new Schema( | ||
| Types.NestedField.required(10, "id", Types.LongType.get()), |
There was a problem hiding this comment.
Could you please import Types.NestedField or even Types.NestedField.required. This could make the code way smaller.
There was a problem hiding this comment.
Hi @pravy. Thanks for your review. I used Types.NestedField explicitly because that seems to be the convention used. If you check StructProjection for which these test cases were written, it doesn't use the complete import and same seems to be the case in all files.
Let me know your thoughts!
There was a problem hiding this comment.
Here is the result of my investigation:
$ git grep NestedField|grep import|wc -l
711
$ git grep NestedField.required|grep import|wc -l
333
The whole class is about creating schemas. We should hide as much boilerplate as possible to be able to read the important parts.
|
|
||
| @Test | ||
| public void testNestedProjection() { | ||
| Types.StructType dataCoordinateStruct = |
| Types.NestedField.required(10, "street", Types.StringType.get()), | ||
| Types.NestedField.required(20, "coordinates", dataCoordinateStruct)); | ||
|
|
||
| Schema dataSchema = |
There was a problem hiding this comment.
You can create a schema like this:
new Schema(
required(14, "content", Types.IntegerType.get()),
required(1, "path", Types.StringType.get()),
required(
8,
"partition_summaries",
ListType.ofRequired(
9,
StructType.of(
required(10, "contains_null", Types.BooleanType.get()),
required(11, "contains_nan", Types.BooleanType.get()),
required(12, "lower_bound", Types.StringType.get()),
required(13, "upper_bound", Types.StringType.get())))));
There was a problem hiding this comment.
@pravy. I am planning to write some more tests for this file especially negative tests for partial projection. Will include nested List in addition to partial projection.
There was a problem hiding this comment.
Since the code is too chatty, I think it is way more readable if we create the whole Schema in a single command. Also the structs are not reused, so we don't really need them
|
Thanks @Govindarajan-D for the PR. Left some minor formatting comments |
2 participants
Added 5 Unit tests that improves the reliability of StructProjection class.
Code coverage from 0% to 82% (Based on Jacoco report)