You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/09/26 20:14:52 UTC
[GitHub] [arrow] lwhite1 opened a new pull request, #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
lwhite1 opened a new pull request, #14244:
URL: https://github.com/apache/arrow/pull/14244
Implemented setNull for vector types where it was not supported (including the abstract ExtensionTypeVector class), and added it to the FieldVector interface.
The change makes it possible to call setNull() on arbitrary field vectors without casting.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983559492
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
@lidavidm
> given this is a new method, I think an exception from the get-go is OK.
This _is_ a new method, but there is an existing, equivalent method in the FieldWriters called `writeNull()` which is implemented as a no-op. I don't think they can have different results. If this one throws an exception, that one should as well, which would be a breaking change. Is that your recommendation?
> Could it also delegate to a child?
I looked at delegating to a child vector, and, as with the above option, I think it needs to be consistent with the FieldWriter. Also, I think to ensure predictable results it would have to write null to _all_ the child vectors, which I wasn't sure was completely practical.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #14244: ARROW-17480: [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983617724
##########
java/vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -851,4 +851,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
+ *
+ * @param index position of element
+ */
+ @Override
+ public void setNull(int index) {}
Review Comment:
It needs to be implemented (or throw an exception) to make it possible to use myArbitraryFieldVector.setNull() without casting for all the vector types
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r982738981
##########
java/vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -851,4 +851,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
+ *
+ * @param index position of element
+ */
+ @Override
+ public void setNull(int index) {}
Review Comment:
Or left unimplemented/abstract if possible
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
Could it also delegate to a child?
Also given this is a new method, I think an exception from the get-go is OK.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983592131
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
The writer API is a separate API, isn't it? Why do they have to be tied?
For sparse union vectors, by definition all the vectors would need to contain some value. So I don't see the issue in delegating. For dense union vectors, only one would need a value, and an offset would be appended.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #14244: ARROW-17480: [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #14244:
URL: https://github.com/apache/arrow/pull/14244
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r981263946
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
Is there a way to create unit test for this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14244:
URL: https://github.com/apache/arrow/pull/14244#issuecomment-1258637262
https://issues.apache.org/jira/browse/ARROW-17480
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r981243443
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
It is a little confusing this way. An UnsupportedOperation exception seems to make sense, but this is the way it's handled in UnionWriter and DenseUnionWriter. I'm not sure we should change it to throw an exception now, but they both should work the same way.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #14244: ARROW-17480: [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983625605
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
Ok I will throw an exception.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r980964063
##########
java/vector/src/main/java/org/apache/arrow/vector/FieldVector.java:
##########
@@ -90,4 +90,11 @@ public interface FieldVector extends ValueVector {
* @return buffer address
*/
long getOffsetBufferAddress();
+
+ /**
+ * Sets the value at index to null.
Review Comment:
Seems like other docstrings use the infinitive:
```suggestion
* Set the element at the given index to null.
```
##########
java/vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -851,4 +851,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
Review Comment:
```suggestion
* Set the element at the given index to null. For UnionVector, it is a no-op as nulls are not supported at the
```
##########
java/vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -851,4 +851,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
+ *
+ * @param index position of element
+ */
+ @Override
+ public void setNull(int index) {}
Review Comment:
Same here: should this raise an error?
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
Hmm, should this raise an error then, instead of silently ignoring the request?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on PR #14244:
URL: https://github.com/apache/arrow/pull/14244#issuecomment-1258575728
@davisusanibar or @lidavidm could one or both of you take a look. The changes are small and simple.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r981258715
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
Oh, I see. Well, can you open a separate JIRA to discuss changing this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #14244: ARROW-17480: [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983707492
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
I think UnsupportedOperation is the best solution (and would have been correct in FieldWriter). I made that change to the code.
There seem to be at least a couple potential issues with delegation from an API design standpoint. One is that if you only update one vector, you're effectively choosing the one at random (if you pick the first, it may already be null), and the user will get inconsistent results if they check for nullability in the child vectors. Also, you will have made a change to the data, but the naive test of the effects of the change (calling isNull()) will report the same result as before the change.
> For dense union vectors, only one would need a value, and an offset would be appended.
This suggests a second issue. If there's only one child vector with a value before you setNull(), then none will have a value after you do. I'm not sure the result would be but it seems like it breaks the expectation.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r981322471
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
@davisusanibar I could create a unit test, but since the method does nothing, it's hard to get it wrong.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #14244: ARROW-17480: [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983642958
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
If it's possible to delegate we should (I still don't see the issue but haven't looked closely)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #14244: ARROW-17480: [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14244:
URL: https://github.com/apache/arrow/pull/14244#issuecomment-1262829116
Benchmark runs are scheduled for baseline = 902781d1f3a41563a23d6755433a8e40ce82de7b and contender = 030042f0c698a41bd4b73dd00ca1d80dea3aacfd. 030042f0c698a41bd4b73dd00ca1d80dea3aacfd is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/fcf8b5dfb04d4abea9bd7e01a14c6bf0...1d46662389e94a3990d598a9b55b049c/)
[Failed :arrow_down:0.07% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/81295bb2b48942be9d585a617f0c9bd4...2083d4dd152340438a0d0ea7a9887826/)
[Failed :arrow_down:0.27% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f9c4bf7487ed40988eebac252c8129e7...30617b70079f41558351a70de7079982/)
[Finished :arrow_down:0.11% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/4b6a0326ce66445288f6dd6dce54d6d3...e8597a43de1046f8a6f92481c7405365/)
Buildkite builds:
[Finished] [`030042f0` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1597)
[Failed] [`030042f0` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1615)
[Failed] [`030042f0` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1598)
[Finished] [`030042f0` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1610)
[Finished] [`902781d1` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1596)
[Finished] [`902781d1` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1614)
[Failed] [`902781d1` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1597)
[Finished] [`902781d1` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1609)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14244:
URL: https://github.com/apache/arrow/pull/14244#issuecomment-1258637301
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lwhite1 commented on a diff in pull request #14244: ARROW-17480 [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983559492
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
@lidavidm
> given this is a new method, I think an exception from the get-go is OK.
This _is_ a new method, but there is an existing, equivalent method in the FieldWriters called `writeNull()` which is implemented as a no-op. I don't think they can have different results. If this one throws an exception, that one should as well, which would be a breaking change. Is that your recommendation?
> Could it also delegate to a child?
I looked at delegating to a child vector, and, as with the above option, I think it needs to be consistent with the FieldWriter. Also, I think to ensure predictable results it would have to write null to _all_ the child vectors, which I wasn't sure was completely practical.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #14244: ARROW-17480: [Java] add setNull() to FieldVector interface
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #14244:
URL: https://github.com/apache/arrow/pull/14244#discussion_r983928178
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
This is really more a design issue with union vectors, IMO. I would still prefer we delegate to the underlying vector but there's not a great choice here because of how unions work, there's always going to be some incongruity.
##########
java/vector/src/main/codegen/templates/DenseUnionVector.java:
##########
@@ -940,4 +940,13 @@ public void setInitialCapacity(int valueCount, double density) {
}
}
}
+
+ /**
+ * Set the element at the given index to null. For DenseUnionVector, it is a no-op as nulls are not supported at the
+ * top level, and isNull() always returns false. Nullability is only handled at the level of individual child vectors.
Review Comment:
In either case LGTM
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org