GH-31387: [C++] Check nullability when validating fields on batches or struct arrays #46129

singh1203 · 2025-04-14T11:22:33Z

Rationale for this change

Ensures schema validation catches null values in non-nullable fields, preventing silent errors when writing to formats like Parquet.

What changes are included in this PR?

Fixes: #31387

Nullability checks were added in ValidateFull() for arrays, struct arrays, union arrays, and record batches.
Introduced new validation logic in validate.cc to recursively check for nulls in non-nullable fields.
Added unit tests in:
- array_test.cc
- array_struct_test.cc
- array_union_test.cc
- record_batch_test.cc

These tests ensure that ValidateFull() fails when nulls are present in non-nullable fields.

Are these changes tested?

Yes, new unit tests have been added

Are there any user-facing changes?

Yes, Users who try to validate or write Arrow data with nulls in non-nullable fields will now receive an explicit validation error.

GitHub Issue: [C++] Check nullability when validating fields on batches or struct arrays #31387

…ruct Signed-off-by: Saurabh Kumar Singh <[email protected]>

…ation Signed-off-by: Saurabh Kumar Singh <[email protected]>

github-actions · 2025-04-14T11:22:59Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2025-04-14T11:26:25Z

⚠️ GitHub issue #31387 has been automatically assigned in GitHub to PR creator.

singh1203 · 2025-04-14T11:29:34Z

cc: @pitrou
@lidavidm

pitrou

Thanks for the PR. You'll find some detailed comments. Two more general comments, though:

most CI builds are failing, and you should have seen test failures locally too. Did you run the unit tests locally before pushing?
the validation logic for nested arrays (struct, list, etc.) is missing, did you forget it?

pitrou · 2025-05-05T16:30:54Z

cpp/src/arrow/array/array_struct_test.cc

+  auto type = struct_({field("a", int32(), /*nullable=*/false),
+                       field("b", utf8(), /*nullable=*/false),
+                       field("c", list(boolean()), /*nullable=*/false)});


We should test more nested situations:

non-nullable struct inside a list

non-nullable struct inside a struct

pitrou · 2025-05-05T16:31:55Z

cpp/src/arrow/array/array_struct_test.cc

+  auto struct_arr = ArrayFromJSON(
+      type, R"([1, "a", [null, false]], [null, "bc", []], [2, null, null]])");
+  auto struct_arr_nonull = ArrayFromJSON(
+      type, R"([[1, "a"], [true, false], [6, "bc", []], [2, "bcj", [true, true]]])");


Are you sure the JSON is right here? The brackets don't seem balanced.

pitrou · 2025-05-05T16:32:11Z

cpp/src/arrow/array/array_test.cc

+  auto array_nested_null = ArrayFromJSON(type, "[[0, 1], [3, 4], [2, null]]");
+
+  ASSERT_RAISES(Invalid, array->ValidateFull());
+  ASSERT_RAISES(Invalid, array->ValidateFull());


This line is repeated, did you mean something else?

pitrou · 2025-05-05T16:32:39Z

cpp/src/arrow/array/array_test.cc

+  ASSERT_RAISES(Invalid, array->ValidateFull());
+}
+
+TEST_F(TestArray, TestValidateFullNullableFixedSizeList) {


I don't see a fixed-size list in this test, did I miss something?

pitrou · 2025-05-05T16:33:08Z

cpp/src/arrow/array/array_union_test.cc

@@ -70,6 +70,21 @@ TEST(TestUnionArray, TestSliceEquals) {
  CheckUnion(batch->column(1));
 }

+TEST(TestSparseUnionArray, TestValidateFullNullable) {
+  auto ty = sparse_union({field("ints", int64()), field("strs", utf8(), false)}, {2, 7});


I don't see any non-nullable field here.

pitrou · 2025-05-05T16:33:21Z

cpp/src/arrow/array/array_union_test.cc

@@ -70,6 +70,21 @@ TEST(TestUnionArray, TestSliceEquals) {
  CheckUnion(batch->column(1));
 }

+TEST(TestSparseUnionArray, TestValidateFullNullable) {


Can you also test a dense union?

pitrou · 2025-05-05T16:33:51Z

cpp/src/arrow/array/validate.cc

@@ -464,8 +465,8 @@ struct ValidateArrayImpl {
    return data.buffers[index] != nullptr && data.buffers[index]->address() != 0;
  }

-  Status RecurseInto(const ArrayData& related_data) {
-    ValidateArrayImpl impl{related_data, full_validation};
+  Status RecurseInto(const ArrayData& related_data, bool nullable = true) {


I would rather make the argument mandatory, so that we don't forget to pass it.

Suggested change

Status RecurseInto(const ArrayData& related_data, bool nullable = true) {

Status RecurseInto(const ArrayData& related_data, bool nullable) {

pitrou · 2025-05-05T16:35:24Z

cpp/src/arrow/array/validate.cc

@@ -558,7 +559,7 @@ struct ValidateArrayImpl {
    }

    if (full_validation) {
-      if (data.null_count != kUnknownNullCount) {
+      if (data.null_count != kUnknownNullCount || !nullable) {


Ok, but where does it test that actual_null_count is 0 if nullable is false?

pitrou · 2025-05-05T16:37:11Z

cpp/src/arrow/table.cc

@@ -187,6 +187,9 @@ class SimpleTable : public Table {
        ss << "Column " << i << ": " << st.message();
        return st.WithMessage(ss.str());
      }
+      if (schema_->field(i)->nullable() && col->null_count() > 0) {


You should check that it's not nullable instead (did you notice the test failures?).

Signed-off-by: Saurabh Kumar Singh <[email protected]>

singh1203 added 5 commits April 3, 2025 23:32

fix: (GH: apache#31387) [C++] Validation for nullability in arrays st…

2df965a

…ruct Signed-off-by: Saurabh Kumar Singh <[email protected]>

Merge branch 'apache:main' into checkNull

fffcbb3

Merge branch 'apache:main' into checkNull

d3ce915

fix: [C++] Nullability mismatch issue in record batch and table valid…

44c8bd5

…ation Signed-off-by: Saurabh Kumar Singh <[email protected]>

Merge branch 'apache:main' into checkNull

609fc30

github-actions bot added Component: C++ awaiting review Awaiting review labels Apr 14, 2025

singh1203 changed the title ~~ARROW-15961: [C++] Check nullability when validating fields on batches or struct arrays~~ GH-31387: [C++] Check nullability when validating fields on batches or struct arrays Apr 14, 2025

Merge branch 'apache:main' into checkNull

bf97170

pitrou requested changes May 5, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 5, 2025

singh1203 added 2 commits May 6, 2025 17:37

Merge branch 'apache:main' into checkNull

1640b91

refactor: Add null check for array data in Array

61cfbae

Signed-off-by: Saurabh Kumar Singh <[email protected]>

singh1203 force-pushed the checkNull branch from 7f4adac to 61cfbae Compare May 20, 2025 12:37

Merge branch 'apache:main' into checkNull

fba30b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GH-31387: [C++] Check nullability when validating fields on batches or struct arrays #46129

GH-31387: [C++] Check nullability when validating fields on batches or struct arrays #46129

Uh oh!

singh1203 commented Apr 14, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Apr 14, 2025

Uh oh!

github-actions bot commented Apr 14, 2025

Uh oh!

singh1203 commented Apr 14, 2025

Uh oh!

pitrou left a comment

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

pitrou May 5, 2025

Uh oh!

Uh oh!

	Status RecurseInto(const ArrayData& related_data, bool nullable = true) {
	Status RecurseInto(const ArrayData& related_data, bool nullable) {

GH-31387: [C++] Check nullability when validating fields on batches or struct arrays #46129

Are you sure you want to change the base?

GH-31387: [C++] Check nullability when validating fields on batches or struct arrays #46129

Uh oh!

Conversation

singh1203 commented Apr 14, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Apr 14, 2025

Uh oh!

github-actions bot commented Apr 14, 2025

Uh oh!

singh1203 commented Apr 14, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singh1203 commented Apr 14, 2025 •

edited by github-actions bot

Loading