slessard commented on code in PR #10953:
URL: https://github.com/apache/iceberg/pull/10953#discussion_r1794325498
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java:
##########
@@ -140,12 +141,21 @@ public static class ConstantVectorHolder<T> extends
VectorHolder {
private final int numRows;
public ConstantVectorHolder(int numRows) {
+ super(new NullVector("_dummy_", numRows), null, new
NullabilityHolder(numRows));
+ nullabilityHolder().setNulls(0, numRows);
this.numRows = numRows;
this.constantValue = null;
}
public ConstantVectorHolder(Types.NestedField icebergField, int numRows, T
constantValue) {
- super(icebergField);
+ super(
Review Comment:
@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 and a
NullVectorHolder containing a null value?
```
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
```
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java:
##########
@@ -140,12 +141,21 @@ public static class ConstantVectorHolder<T> extends
VectorHolder {
private final int numRows;
public ConstantVectorHolder(int numRows) {
Review Comment:
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?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
