Add validation to `RecordBatch` for non-nullable fields containing null values #1890

andygrove · 2022-06-16T18:06:26Z

Which issue does this PR close?

Rationale for this change

Allowing null values in non-nullable fields can lead to incorrect results and missed optimizations in downstream crates such as DataFusion

What changes are included in this PR?

Add validation check when creating a RecordBatch

Are there any user-facing changes?

Yes. Users may start getting errors if their schemas are not currently defined correctly.

… values

andygrove · 2022-06-16T18:28:01Z

@alamb I think this PR has exposed a bug in dictionary IPC encoding. The checked-in dictionary_generated files seem to have an incorrect schema (declaring a column as non-nullable when it contains null values). The roundtrip tests fail for the same reason.

andygrove · 2022-06-16T18:36:36Z

arrow/src/record_batch.rs

+                    // hacky workaround for known issue with dictionary IPC encoding
+                    // https://github.com/apache/arrow-rs/issues/1892


Personally I'd rather an if !condition over an if else with an empty if body

codecov-commenter · 2022-06-16T19:02:37Z

Codecov Report

Merging #1890 (c91f355) into master (f0bf7f9) will increase coverage by 0.00%.
The diff coverage is 97.29%.

@@           Coverage Diff           @@
##           master    #1890   +/-   ##
=======================================
  Coverage   83.44%   83.44%           
=======================================
  Files         202      202           
  Lines       57135    57146   +11     
=======================================
+ Hits        47678    47688   +10     
- Misses       9457     9458    +1

Impacted Files	Coverage Δ
arrow/src/array/array_binary.rs	`94.18% <ø> (ø)`
arrow/src/record_batch.rs	`93.90% <90.90%> (-0.09%)`	⬇️
arrow/src/csv/reader.rs	`89.89% <100.00%> (ø)`
arrow/src/ipc/writer.rs	`81.78% <100.00%> (ø)`
arrow/src/json/reader.rs	`84.58% <100.00%> (ø)`
arrow/src/json/writer.rs	`92.11% <100.00%> (ø)`
arrow/src/datatypes/datatype.rs	`65.42% <0.00%> (-0.38%)`	⬇️
arrow/src/ffi_stream.rs	`79.89% <0.00%> (ø)`
parquet/src/encodings/encoding.rs	`93.65% <0.00%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0bf7f9...c91f355. Read the comment docs.

tustvold

Nice, minor nits, big fan of this

tustvold · 2022-06-16T19:01:15Z

arrow/src/record_batch.rs

@@ -138,6 +138,20 @@ impl RecordBatch {
                )
            })?;

+        for (c, f) in columns.iter().zip(&schema.fields) {
+            if !f.is_nullable() && c.null_count() > 0 {
+                if f.name().len() == 0 {


is_empty() is normally preferred

tustvold · 2022-06-16T19:02:48Z

arrow/src/record_batch.rs

+                    // hacky workaround for known issue with dictionary IPC encoding
+                    // https://github.com/apache/arrow-rs/issues/1892


Personally I'd rather an if !condition over an if else with an empty if body

alamb

Thanks @andygrove

viirya

lgtm

andygrove · 2022-06-16T20:19:16Z

Integration tests failed with:

################# FAILURES #################
FAILED TEST: datetime Java producing,  C# consuming

andygrove · 2022-06-16T22:32:10Z

Failed with a Rust / C# test this time

==========================================================
Testing file duration
==========================================================
Traceback (most recent call last):
################# FAILURES #################
FAILED TEST: datetime Rust producing,  C# consuming
1 failures
  File "/arrow/dev/archery/archery/integration/runner.py", line 246, in _run_ipc_test_case
    run_binaries(producer, consumer, test_case)
  File "/arrow/dev/archery/archery/integration/runner.py", line 286, in _produce_consume
    consumer.stream_to_file(producer_stream_path, consumer_file_path)
  File "/arrow/dev/archery/archery/integration/tester_csharp.py", line 63, in stream_to_file
    self.run_shell_command(cmd)
  File "/arrow/dev/archery/archery/integration/tester.py", line 49, in run_shell_command
    subprocess.check_call(cmd, shell=True)
  File "/opt/conda/envs/arrow/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '/arrow/csharp/artifacts/Apache.Arrow.IntegrationTest/Debug/net6.0/Apache.Arrow.IntegrationTest --mode stream-to-file -a /tmp/tmpyt_o8mf0/b64aa0aa_generated_datetime.consumer_stream_as_file < /tmp/tmpyt_o8mf0/b64aa0aa_generated_datetime.producer_file_as_stream' returned non-zero exit status 1.

andygrove · 2022-06-16T22:46:06Z

Other builds are failing with the same issue. I wonder if it is related to apache/arrow#13279 ?

liukun4515

LGTM

viirya · 2022-06-17T04:08:42Z

arrow/src/record_batch.rs

+                // hacky workaround for known issue with dictionary IPC encoding
+                // https://github.com/apache/arrow-rs/issues/1892


#1893 should fix this issue.

Thanks @viirya I have upmerged and removed this workaround

tustvold · 2022-06-18T06:03:01Z

I think a rebase should clear the test failures related to IPC dictionaries

…nullable-column

alamb · 2022-06-19T11:09:56Z

🎉

alamb · 2022-06-23T20:22:43Z

Marking this as an API change because it caused many errors in the datafusion tests -- see apache/datafusion#2778 -- and I think it should be highlighted prominently in the release notes

add validation to RecordBatch for non-nullable fields containing null…

0c08160

… values

github-actions bot added the arrow Changes to the arrow crate label Jun 16, 2022

Fix some IPC tests

4055c49

andygrove requested a review from alamb June 16, 2022 18:26

andygrove mentioned this pull request Jun 16, 2022

Dictionary IPC writer writes incorrect schema #1892

Closed

add workaround for dictionary issue

d64f888

andygrove changed the title ~~WIP: Add validation to RecordBatch for non-nullable fields containing null values~~ Add validation to RecordBatch for non-nullable fields containing null values Jun 16, 2022

andygrove marked this pull request as ready for review June 16, 2022 18:36

andygrove commented Jun 16, 2022

View reviewed changes

tustvold approved these changes Jun 16, 2022

View reviewed changes

address feedback

c91f355

alamb approved these changes Jun 16, 2022

View reviewed changes

viirya approved these changes Jun 16, 2022

View reviewed changes

liukun4515 approved these changes Jun 17, 2022

View reviewed changes

viirya reviewed Jun 17, 2022

View reviewed changes

viirya mentioned this pull request Jun 17, 2022

Correct nullable in read_dictionary #1893

Merged

andygrove added 3 commits June 18, 2022 07:17

Merge remote-tracking branch 'apache/master' into check-nulls-in-non-…

da472a5

…nullable-column

upmerge

6495a76

remove workaround for dictionary types

cada5e4

github-actions bot added the arrow-flight Changes to the arrow-flight crate label Jun 18, 2022

revert unrelated cargo fmt changes

c1c4c94

andygrove removed the arrow-flight Changes to the arrow-flight crate label Jun 18, 2022

andygrove merged commit 535cd20 into apache:master Jun 18, 2022

andygrove deleted the check-nulls-in-non-nullable-column branch June 18, 2022 14:43

alamb added the api-change Changes to the arrow API label Jun 23, 2022

tustvold mentioned this pull request Jul 31, 2023

ListArray::try_new disallows null elements if field.is_nullable() == false #4602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation to `RecordBatch` for non-nullable fields containing null values #1890

Add validation to `RecordBatch` for non-nullable fields containing null values #1890

andygrove commented Jun 16, 2022

andygrove commented Jun 16, 2022

andygrove Jun 16, 2022

tustvold Jun 16, 2022

codecov-commenter commented Jun 16, 2022 •

edited

Loading

tustvold left a comment

tustvold Jun 16, 2022

tustvold Jun 16, 2022

alamb left a comment

viirya left a comment

andygrove commented Jun 16, 2022

andygrove commented Jun 16, 2022

andygrove commented Jun 16, 2022

liukun4515 left a comment

viirya Jun 17, 2022

andygrove Jun 18, 2022

tustvold commented Jun 18, 2022

alamb commented Jun 19, 2022

alamb commented Jun 23, 2022

		// hacky workaround for known issue with dictionary IPC encoding
		// https://github.com/apache/arrow-rs/issues/1892

Add validation to RecordBatch for non-nullable fields containing null values #1890

Add validation to RecordBatch for non-nullable fields containing null values #1890

Conversation

andygrove commented Jun 16, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

andygrove commented Jun 16, 2022

andygrove Jun 16, 2022

Choose a reason for hiding this comment

tustvold Jun 16, 2022

Choose a reason for hiding this comment

codecov-commenter commented Jun 16, 2022 • edited Loading

Codecov Report

tustvold left a comment

Choose a reason for hiding this comment

tustvold Jun 16, 2022

Choose a reason for hiding this comment

tustvold Jun 16, 2022

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

andygrove commented Jun 16, 2022

andygrove commented Jun 16, 2022

andygrove commented Jun 16, 2022

liukun4515 left a comment

Choose a reason for hiding this comment

viirya Jun 17, 2022

Choose a reason for hiding this comment

andygrove Jun 18, 2022

Choose a reason for hiding this comment

tustvold commented Jun 18, 2022

alamb commented Jun 19, 2022

alamb commented Jun 23, 2022

Add validation to `RecordBatch` for non-nullable fields containing null values #1890

Add validation to `RecordBatch` for non-nullable fields containing null values #1890

codecov-commenter commented Jun 16, 2022 •

edited

Loading