ARROW-11504: [Rust] Added checks to List DataType. #9425

jorgecarleitao · 2021-02-05T05:49:17Z

This PR fixes a bug on which GenericListArray is not validating the datatype passed on to from(ArrayData), causing all types of bugs, such as undefined behavior in interpreting the offset buffer.

This PR adds this validation, panicking if the DataType does not match.

This PR also fixes casting from and to Lists, which was creating an ArrayData out of spec.

github-actions · 2021-02-05T05:49:42Z

https://issues.apache.org/jira/browse/ARROW-11504

sunchao

Thanks @jorgecarleitao for pinging. Left a few comments.

sunchao · 2021-02-06T17:49:07Z

rust/arrow/src/array/array_list.rs

+        let values = data.child_data()[0].clone();
+
+        if let Some(child) = Self::get_type(data.data_type()) {
+            assert_eq!(values.data_type(), child, "[Large]ListArray's child datatype does not correspond to the List's datatype");


nit: long line? I remember we enforce a limit of 100 characters.

sunchao · 2021-02-06T18:01:00Z

rust/arrow/src/array/array_list.rs

+
+        let values = data.child_data()[0].clone();
+
+        if let Some(child) = Self::get_type(data.data_type()) {


can we have a few tests to cover this? checking error message and all.

Also I'm not sure if assert_eq is good here: IMO assertion should only be used for checking internal logic that developer should follow and which are not exposed to the library users, but in this case it appears not. It's just a nit though since this is already used in multiple places before.

I agree with you. However, that requires a larger change as we would need to move from From to TryFrom, so for now I just want to avoid unsafe code by panicking everytime something may go wrong.

Here is a mid-way proposal: a31a35a

Basically, to use normal rust handling, but the make the Into implementation expect the result

I actually think using asserts / panics directly (as in this PR) is also fine beacuse:

it is an improvement over the current behavior (crash / undefined) to get useful error messages (even if it is in a panic :( )

the use of ArrayData in my mind is also an implementation detail of an Array so most users of Arrow shouldn't be interacting with this code at all.

BTW I also tried using TryFrom directly and as @jorgecarleitao suspected there are many kernel implementations that rely in this being infallable.

sunchao · 2021-02-06T18:01:30Z

rust/arrow/src/array/array_list.rs

+        false
+    }
+
    fn prefix() -> &'static str {


maybe we won't need prefix anymore with the new is_large.

We might still need it, we also use it for formatting in Display

I think that we can drop it, yes. We can merge StringOffset, BinaryOffset and OffsetTrait in a single Trait with this, but I wanted to leave it to another PR.

Here is one way to remove prefix that does not go as far as @jorgecarleitao suggests to collapse the traits... 8e68e05

jhorstmann · 2021-02-10T22:50:48Z

Nice! I recently ran into the same issue and the assertion about the nested datatypes would have saved me some time debugging. In my testcase an assert_eq was failing, but printed both sides exactly the same.

jorgecarleitao · 2021-02-11T07:40:44Z

@jhorstmann , I think that that shows another, separate issue: imo what we currently show in "debug" should be shown in "Display", and "Debug" should actually show the full structure, i.e. the one created by #[derive(Debug)].

nevi-me

No additional comments, other than @sunchao's

nevi-me · 2021-02-13T05:47:19Z

rust/arrow/src/array/array_list.rs

+        false
+    }
+
    fn prefix() -> &'static str {


We might still need it, we also use it for formatting in Display

alamb

@jorgecarleitao do you need help with this PR? I can try and take some of @sunchao 's comments if that would help

jorgecarleitao · 2021-02-15T20:38:19Z

@alamb that would definitely help. If you have the time, I would really appreciate.

alamb · 2021-02-15T20:42:31Z

@alamb that would definitely help. If you have the time, I would really appreciate.

@jorgecarleitao I will put it on my queue for tomorrow

alamb

I think this PR is good to merge as is as it makes the code better than on master (less errors), though it can be further improved.

@sunchao when you get some time I would love your feedback / advice -- should we merge this PR as is? Would you suggest incorporating one/both of the approaches prototyped in #9508 (remove prefix and making a fallable version of ArrayData --> GenericListArray)

sunchao

LGTM2 - I think we can merge this as it is and solve the comments separately. Thanks!

BTW this needs rebase.

nevi-me · 2021-02-26T17:27:55Z

@alamb can we merge this when CI is green, then treat #9508 as a separate JIRA ticket? This had fallen quite behind, but I've rebased and fixed failures on it, when I should have done it against #9508 instead.

codecov-io · 2021-02-26T18:56:43Z

Codecov Report

Merging #9425 (979a136) into master (5bea624) will increase coverage by 0.07%.
The diff coverage is 85.93%.

@@            Coverage Diff             @@
##           master    #9425      +/-   ##
==========================================
+ Coverage   82.25%   82.33%   +0.07%     
==========================================
  Files         244      245       +1     
  Lines       55685    56270     +585     
==========================================
+ Hits        45806    46330     +524     
- Misses       9879     9940      +61

Impacted Files	Coverage Δ
rust/arrow/src/alloc/types.rs	`0.00% <0.00%> (ø)`
rust/datafusion/src/logical_plan/expr.rs	`81.56% <ø> (+0.42%)`	⬆️
...ion-testing/src/bin/arrow-json-integration-test.rs	`0.00% <0.00%> (ø)`
rust/arrow/src/ipc/writer.rs	`87.23% <50.00%> (-0.59%)`	⬇️
rust/arrow/src/array/array_list.rs	`92.95% <70.00%> (-1.10%)`	⬇️
...datafusion/src/physical_plan/string_expressions.rs	`77.00% <82.23%> (+7.37%)`	⬆️
...t/datafusion/src/physical_plan/coalesce_batches.rs	`84.95% <83.33%> (-0.23%)`	⬇️
rust/parquet/src/arrow/array_reader.rs	`77.61% <91.30%> (-0.02%)`	⬇️
rust/datafusion/src/physical_plan/functions.rs	`85.52% <91.42%> (+11.69%)`	⬆️
rust/arrow/src/alloc/mod.rs	`92.68% <92.68%> (ø)`
... and 29 more

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 4da5822...06ef10c. Read the comment docs.

alamb · 2021-02-26T19:28:44Z

@nevi-me good plan!

alamb · 2021-02-26T19:30:03Z

rust/arrow/src/ipc/writer.rs

            "generated_dictionary",
            // "generated_duplicate_fieldnames",
            "generated_interval",
-            "generated_large_batch",


@nevi-me I don't remember seeing this in the original PR -- was this change intended ?

NM I see #9587 now

@sunchao

# Background: Left over cleanups suggested by from @sunchao on #9425 Broken out from #9508 # Rationale: This function is redundant with `OffsetSize::is_large` Closes #9690 from alamb/alamb/remove_prefix Authored-by: Andrew Lamb <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>

@sunchao

…ead of `panic!` # Background: Left over cleanups suggested by from @sunchao on #9425 Broken out from #9508 # Rationale: Don't use panic! directly. However, since the caller of this function still calls `unwrap()`, I am not sure how much of an improvement this change really is. However it may set us up for a more `safe` future eventually Closes #9691 from alamb/alamb/fallable_list_conversion Authored-by: Andrew Lamb <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>

@sunchao

# Background: Left over cleanups suggested by from @sunchao on apache/arrow#9425 Broken out from apache/arrow#9508 # Rationale: This function is redundant with `OffsetSize::is_large` Closes #9690 from alamb/alamb/remove_prefix Authored-by: Andrew Lamb <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>

@sunchao

…ead of `panic!` # Background: Left over cleanups suggested by from @sunchao on apache/arrow#9425 Broken out from apache/arrow#9508 # Rationale: Don't use panic! directly. However, since the caller of this function still calls `unwrap()`, I am not sure how much of an improvement this change really is. However it may set us up for a more `safe` future eventually Closes #9691 from alamb/alamb/fallable_list_conversion Authored-by: Andrew Lamb <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>

github-actions bot added the Component: Rust label Feb 5, 2021

jorgecarleitao requested review from nevi-me and sunchao February 5, 2021 05:50

sunchao reviewed Feb 6, 2021

View reviewed changes

nevi-me reviewed Feb 13, 2021

View reviewed changes

jorgecarleitao force-pushed the master branch from d4608a9 to 356c300 Compare February 14, 2021 12:09

alamb reviewed Feb 15, 2021

View reviewed changes

alamb mentioned this pull request Feb 16, 2021

ARROW-11504: [Rust] Added checks to List DataType (WIP IMPROVEMENTS) #9508

Closed

alamb approved these changes Feb 18, 2021

View reviewed changes

alamb added the needs-rebase A PR that needs to be rebased by the author label Feb 18, 2021

sunchao approved these changes Feb 20, 2021

View reviewed changes

nevi-me removed the needs-rebase A PR that needs to be rebased by the author label Feb 26, 2021

nevi-me mentioned this pull request Feb 26, 2021

ARROW-11778: [Rust] Cast from LargeUtf8 to Numerical and temporal types #9571

Closed

Added checks to List DataType.

06ef10c

alamb reviewed Feb 26, 2021

View reviewed changes

alamb closed this in acd2a47 Feb 26, 2021

This was referenced Mar 13, 2021

ARROW-11951: [Rust] Remove OffsetSize::prefix #9690

Closed

ARROW-11952: [Rust] Make ArrayData --> GenericListArray fallable instead of panic! #9691

Closed

This was referenced Mar 13, 2021

[Rust] verify Datatype in ListArray::from(ArrayDataRef) #27382

Closed

[Rust] Remove OffsetSize::prefix #27786

Closed

[Rust] Make ArrayData --> GenericListArray fallable instead of panic! #27787

Closed


		let values = data.child_data()[0].clone();

		if let Some(child) = Self::get_type(data.data_type()) {

ARROW-11504: [Rust] Added checks to List DataType. #9425

ARROW-11504: [Rust] Added checks to List DataType. #9425

Uh oh!

Conversation

jorgecarleitao commented Feb 5, 2021 • edited by sunchao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 5, 2021

Uh oh!

sunchao 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!

jhorstmann commented Feb 10, 2021

Uh oh!

jorgecarleitao commented Feb 11, 2021

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

jorgecarleitao commented Feb 15, 2021

Uh oh!

alamb commented Feb 15, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

sunchao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me commented Feb 26, 2021

Uh oh!

codecov-io commented Feb 26, 2021

Codecov Report

Uh oh!

alamb commented Feb 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jorgecarleitao commented Feb 5, 2021 •

edited by sunchao

Loading

sunchao left a comment •

edited

Loading