kbendick commented on a change in pull request #4283:
URL: https://github.com/apache/iceberg/pull/4283#discussion_r821244797
##########
File path:
mr/src/test/java/org/apache/iceberg/mr/hive/serde/objectinspector/TestIcebergRecordObjectInspector.java
##########
@@ -61,4 +61,17 @@ public void testIcebergRecordObjectInspector() {
Assert.assertEquals(innerRecord.get(0),
innerSoi.getStructFieldData(innerData, stringField));
}
+ @Test
+ public void testIcebergRecordObjectInspectorWithRowNull() {
+ Schema schema = new Schema(
+ required(1, "integer_field", Types.IntegerType.get()),
+ required(2, "struct_field", Types.StructType.of(
+ Types.NestedField.required(3, "string_field",
Types.StringType.get())))
+ );
+ StructObjectInspector soi = (StructObjectInspector)
IcebergObjectInspector.create(schema);
+ Assert.assertNull(soi.getStructFieldsDataAsList(null));
+ StructField integerField = soi.getStructFieldRef("integer_field");
+ Assert.assertNull(soi.getStructFieldData(null, integerField));
Review comment:
Would it be possible to stick a null column into an e2e Hive job, to
ensure that reading it doesn't cause issues? This is just testing that the
object inspector returns null appropriately. But it doesn't test if other code
(which previously wasn't getting null) will hit any NPEs or anything.
If that test is too complicated to add in somehow, probably not the end of
the world 🙂
##########
File path:
mr/src/test/java/org/apache/iceberg/mr/hive/serde/objectinspector/TestIcebergRecordObjectInspector.java
##########
@@ -61,4 +61,17 @@ public void testIcebergRecordObjectInspector() {
Assert.assertEquals(innerRecord.get(0),
innerSoi.getStructFieldData(innerData, stringField));
}
+ @Test
+ public void testIcebergRecordObjectInspectorWithRowNull() {
+ Schema schema = new Schema(
+ required(1, "integer_field", Types.IntegerType.get()),
+ required(2, "struct_field", Types.StructType.of(
+ Types.NestedField.required(3, "string_field",
Types.StringType.get())))
+ );
Review comment:
Nit: The indenation is off here.
There should be two spaces when the indentation changes, and 4 spaces when
the same code statement / expression continues on the next line. Like so.
```java
Schema schema = new Schema(
required(1, "integer_field", Types.IntegerType.get()),
required(2, "struct_field", Types.StructType.of(
Types.NestedField.required(3, "string_field",
Types.StringType.get())))
);
```
##########
File path:
mr/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergRecordObjectInspector.java
##########
@@ -72,11 +72,17 @@ public StructField getStructFieldRef(String name) {
@Override
public Object getStructFieldData(Object o, StructField structField) {
+ if (o == null) {
+ return null;
+ }
Review comment:
Nit: Please add a newline between control flow blocks (so after this if
block).
Same comment for below 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
