Arrow: add support for null vectors

[slessard]

Add new class NullAccessor to aid reading null columns. A null column is a column that exists in the Iceberg schema, but does not exist in the parquet schema. A column would exist in Iceberg, but not in Parquet when a column is added to the Iceberg schema, but no new rows have been added to the table. See ArrowReadetTest.testReadColumnThatDoesNotExistInParquetSchema for an example.

Closes #10275

[nastra]

@sl255051 I spent some time understanding the problem and going through the code and fixing it myself (without first looking at this PR). I think you're on the right track here to handle the root cause and we should have a NullAccessor that internally tracks a NullVector.

I would suggest to squash all the commits to a single one and then go through my comments

[sl255051]

Thank you, @nastra, for your help. I will be on vacation next week. I will pick this up again when I return on September 3.

[slessard]

@amogh-jahagirdar This PR is ready for your review

[slessard]

@amogh-jahagirdar I am not planning to make any additional changes. Please review

[slessard]

@RussellSpitzer I'd appreciate it if you could review these changes in iceberg-arrow

[amogh-jahagirdar]

I might be confusing vectorized read paths but I'm curious why this isn't reproducible in Spark vectorized reads? Or is it?

[amogh-jahagirdar]

I can try and repro in a unit test for Spark and see if it's the case. To be clear I don't want to hold up this PR on that though since it does seem like a legitimate problem based on the test being done here.

[slessard]

I never tried reproducing the issue in Spark. My guess as to why there was no existing test case is that null vectors isn't a bug so much as it is a subfeature that was never implemented. No sense in creating a test for a subfeature that was knowingly never implemented.

What makes me say null vector support was knowingly never implemented? Look at the removed comment in arrow/src/main/java/org/apache/iceberg/arrow/vectorized/ArrowBatchReader.java. That comment wasn't a "this is how this code works" type of comment. In my opinion that comment is a TODO comment.

[RussellSpitzer]

I'm not a big fan of placing an if-statement in the constructor itself, it feels more like we should just have a different constructor or class?

[slessard]

@RussellSpitzer In an earlier draft of this PR I did create a NullVectorHolder class but was told to instead adapt ConstantVectorHolder to work for my case, so I did that.

Here's a patch to add a new constructor, but it has at least one issue. Passing null as the constant value in the existing constructor is no longer allowed. This is a semantic breaking change, though not an API breaking change. What this means is that third party code that upgrades to a version of Apache Iceberg containing this patch that was previously using this constructor and passing in a null constant value will still compile fine but will have a runtime failure. That doesn't seem like an appealing solution to me.

I think adding a new class such as NullVectorHolder has its own issue. What would be the use case for a ConstantVectorHolder containing a null value versus a NullVectorHolder containing a null value?

Which solution would you prefer?

1. ternary operator in the super call
2. Overloaded constructor with semantics breaking change
3. New class, NullvectorHolder, with ambiguous use case

Subject: [PATCH] Changes
---
Index: arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java b/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java
--- a/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java	(revision 163ee62ce52bc9611198325997010c7e2b793c71)
+++ b/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java	(date 1728512295607)
@@ -148,16 +148,21 @@
     }
 
     public ConstantVectorHolder(Types.NestedField icebergField, int numRows, T constantValue) {
+      super(icebergField);
+      Preconditions.checkNotNull(constantValue, "Constant value cannot be null");
+      this.numRows = numRows;
+      this.constantValue = constantValue;
+    }
+
+    public ConstantVectorHolder(Types.NestedField icebergField, int numRows) {
       super(
-          (null == constantValue) ? new NullVector(icebergField.name(), numRows) : null,
-          icebergField,
-          new NullabilityHolder(numRows));
-      if (null == constantValue) {
-        nullabilityHolder().setNulls(0, numRows);
-      }
+              new NullVector(icebergField.name(), numRows),
+              icebergField,
+              new NullabilityHolder(numRows));
+      nullabilityHolder().setNulls(0, numRows);
 
       this.numRows = numRows;
-      this.constantValue = constantValue;
+      this.constantValue = null;
     }
 
     @Override

[RussellSpitzer]

Why are we keeping this constructor? Is it needed if we can make a NullVectorHolder?

[slessard]

This constructor could be deleted but that would be an API breaking change because this is a public constructor in a public class. Which would you prefer, an extra constructor or a breaking change?