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 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.

[nastra]

It would be nice to explore and see how this looks in Spark's vectorized path

[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]

Ok having gone through the code I think I may have a strategy here but please let me know what i'm missing in this interpretation.

ConstantVectorHolder is used as a dummy holder prior to this commit. It has no vector , column descriptor, etc ... everything is null. The current approach sometimes makes a ConstantVectorHolder which sometimes does have something inside of it. This is done so that GenericArrowVectorAccessorFactory will have something to work with.

From what I can see in GenericArrowVectorAccessorFactory is that it doesn't work with ConstantVectorHolders at all. It currently can only handle cases in which holder.vector() is a non null type and matches some known vector type. Spark on the other hand, does not actually use this path and when it sees a ConstantVectorHolder it instead creates a o.a.i.s.ConstantColumnVector. This is why I don't think Spark has any issue with this. (cc @amogh-jahagirdar )

Now this makes me think what we really need is to just fix GenericArrowVectorAccessorFactory to handle the case where the "vector" is null and in that circumstance attempt to cast the VectorHolder to a ConstantVectorHolder and create the appropriate vector type or accessor. I think we have a few options to do this.

I think the easiest may be to just create an accessor that looks like the Spark ConstantColumnVector and just use that as our generic accessor.

[RussellSpitzer]

The another approach is to directly mimic the Spark implementation and fork here

iceberg/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/ArrowBatchReader.java

Line 53 in c07f2aa

// Handle null vector for constant case

And if the holder is a ConstantVectorHolder set the vector to a new ConstantVector which we define similar to the Spark ConstantColumnVector code again.

[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?

[nastra]

Suggested change

	(null == constantValue) ? new NullVector(icebergField.name(), numRows) : null,
	null == constantValue ? new NullVector(icebergField.name(), numRows) : null,

[nastra]

Suggested change

	assertThat(root.getVector("a").getClass()).isEqualTo(IntVector.class);
	assertThat(root.getVector("a")).isInstanceOf(IntVector.class);

please also update all other places in this test class

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.