parthchandra commented on code in PR #1422:
URL: https://github.com/apache/arrow-java/pull/1422#discussion_r4461648315
##########
vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowFieldNode.java:
##########
@@ -31,12 +30,14 @@ public class ArrowFieldNode implements FBSerializable {
* Constructs a new instance.
*
* @param length The number of values written.
- * @param nullCount The number of null values.
+ * @param nullCount The number of null values, or -1 if unknown.
*/
public ArrowFieldNode(long length, long nullCount) {
super();
- this.length = checkedCastToInt(length);
- this.nullCount = checkedCastToInt(nullCount);
+ Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s",
length);
Review Comment:
The PR description says this also fixes the C Data path. C Data imports
reach this check through ArrayImporter.java:131, which calls new
ArrowFieldNode(snapshot.length, snapshot.null_count). But no test covers that
path. Could you add a test in the c module that imports an array with a length
of -1 and another with a length larger than Integer.MAX_VALUE?
Each test should check the error and check that no memory leaks. Also,
ArrayImporter.java:131 is outside the try block that adds the field name to
errors. So a bad length from C Data gives an error that does not say which
field was bad.
--
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]
