feat: implemented with_field() for FixedSizeListBuilder #5541

istvan-fodor · 2024-03-21T20:10:24Z

Which issue does this PR close?

Rationale for this change

FixedSizeListBuilder doesn't allow users to specify the inner field of elements and defaults to the following FieldRef: Arc::new(Field::new("item", values_data.data_type().clone(), true))

We would like to keep the default behavior as-is, but also allow users to specify use their own FieldRef.

What changes are included in this PR?

Added with_field to FixedSizeListBuilder, copied over from GenericListBuilder
Added new field: Option<FieldRef> to FixedSizeListBuilder
Implemented field logic patterned after GenericListBuilder in finish() and finish_build()
Changed unsafe build to safe build (see user-facing changes)
Unit tests for 3 scenarios:
1. with_field used
2. with_field used with non-nullable field, expect panic
3. with_field used with wrong type, expect panic

Are there any user-facing changes?

Yes.

The finish_cloned() and finish() methods no longer use the build_unchecked() method, but rely on build(). If the build fails, the thread panics.

alamb

Thank you @istvan-fodor -- this looks like a nice API improvement to me.

arrow-array/src/builder/fixed_size_list_builder.rs

alamb

Thank you @istvan-fodor -- I think this looks good to me. I had a small test organization suggestion but I don't think it is required to merge this PR

I don't think the CI failures are related to changes in this PR -- I filed #5544 to fix.

Thanks again

cc @TimDiekmann

alamb · 2024-03-22T20:54:10Z

arrow-array/src/builder/fixed_size_list_builder.rs

+                assert_eq!(
+                    f.data_type(),
+                    values_data.data_type(),
+                    "DataType of field ({}) should be the same as the values_builder DataType ({})",


alamb · 2024-03-22T21:01:50Z

arrow-array/src/builder/fixed_size_list_builder.rs

+        //  [[0, 1, 2], null, [3, null, 5], [6, 7, null]]
+        builder.values().append_value(0);
+        builder.values().append_value(1);
+        builder.values().append_value(2);
+        builder.append(true);
+        builder.values().append_null();
+        builder.values().append_null();
+        builder.values().append_null();
+        builder.append(false);
+        builder.values().append_value(3);
+        builder.values().append_null();
+        builder.values().append_value(5);
+        builder.append(true);
+        builder.values().append_value(6);
+        builder.values().append_value(7);
+        builder.values().append_null();
+        builder.append(true);


It might be nice to avoid repeating this same setup multiple times in the tests (and it would make it clear the tests share the same data)

for example

fn make_list_builder() -> FixedSizedListBuilder<i32> { ... }

Suggested change

// [[0, 1, 2], null, [3, null, 5], [6, 7, null]]

builder.values().append_value(0);

builder.values().append_value(1);

builder.values().append_value(2);

builder.append(true);

builder.values().append_null();

builder.values().append_null();

builder.values().append_null();

builder.append(false);

builder.values().append_value(3);

builder.values().append_null();

builder.values().append_value(5);

builder.append(true);

builder.values().append_value(6);

builder.values().append_value(7);

builder.values().append_null();

builder.append(true);

let builder = make_list_builder();

Makes sense, I'll go through the tests and rework the builders.

arrow-array/src/builder/fixed_size_list_builder.rs

istvan-fodor · 2024-03-27T14:04:35Z

@alamb , not sure you saw my comment on line 328. Can you check? The new behavior of non-nullable fields raises an additional question about null fields and I wanted to make sure this is in line with the rest of the arrow builders.

tustvold · 2024-04-03T08:54:34Z

I could be missing something but I don't see a comment on line 328? Is it possible that it is pending and the review hasn't been submitted?

istvan-fodor · 2024-04-03T14:57:38Z

arrow-array/src/builder/fixed_size_list_builder.rs

@@ -248,9 +325,37 @@ mod tests {
    }

    #[test]
-    fn test_fixed_size_list_array_builder_finish_cloned() {
+    fn test_fixed_size_list_array_builder_with_field() {
+        let builder = make_list_builder(false, false);


@alamb , I had a question about this test in particular. With the new capability to set the field as a non-nullable we have the situation where the builder has a null element built like this:

builder.values().append_null(); builder.values().append_null(); builder.values().append_null(); builder.append(false);

If the field is set to non-nullable, this element will fail the null check. I wanted to check with you if this is logically right or not? Are we making a distinction between nulls in the values builder, where let's say 1 of the 3 elements of a particular fixed size list is null (which would be a valid fail on non-nullability) or nulls in the main builder (which is built up as 3 nulls + a append(false) in the values builder). To me the second one is questionable and comes down to how you want this to work. If this test should not fail with the second complete null case, then I have to go back and revise the null check of the values builder and somehow control for null elements of the main builder.

The logic we apply for FixedSizeList and StructArray elsewhere is to permit "masked" nulls, this is because a null must always take up space. I am not sure if we want to replicate this logic here.

See https://docs.rs/arrow-array/latest/arrow_array/array/struct.FixedSizeListArray.html#method.try_new

If the field is set to non-nullable, this element will fail the null check. I wanted to check with you if this is logically right or not?

I think I would expect it to be consistent with whatever FixedSlizeListArray::try_new did

istvan-fodor · 2024-04-03T14:58:23Z

I could be missing something but I don't see a comment on line 328? Is it possible that it is pending and the review hasn't been submitted?

Yes, that was the case. I just changed it to a regular comment

SteampunkIslande · 2024-04-04T10:20:48Z

Hello ! I'm very interested in this PR, I really need FixedSizeList columns and didn't find a workaround that makes the FixedSizeListBuilder create a column with an appropriate name. So I'm really looking forward for this to be accepted

tustvold · 2024-04-04T10:29:29Z

There is currently an ongoing discussion w.r.t null handling, in the short term you could possibly use the array constructors directly - https://docs.rs/arrow-array/latest/arrow_array/array/struct.FixedSizeListArray.html#method.try_new

SteampunkIslande · 2024-04-05T12:30:15Z

There is currently an ongoing discussion w.r.t null handling, in the short term you could possibly use the array constructors directly - https://docs.rs/arrow-array/latest/arrow_array/array/struct.FixedSizeListArray.html#method.try_new

Thanks, but I really need a builder. Tried implementing it myself with no luck so far. I'm pretty surprised that the builder doesn't know its field name though. There should be a way to know right ? At least I'm hoping that with your pull request it will be fixed

alamb

Thanks you @istvan-fodor -- I think this is getting close. Some final touchups:

Update the test for zero length lists (should not fail null check)
Add a test for building a 2 element array where one elemnt is NULL (with a null element)
(maybe) Another test showing the null behavior you are asking about

alamb · 2024-04-07T17:27:06Z

arrow-array/src/builder/fixed_size_list_builder.rs

+
+    #[test]
+    #[should_panic(
+        expected = "field is nullable = false, but the values_builder contains null values"


this doesn't seem right -- the builder doesn't have null values, instead it has no values at all.

hi @alamb. this is not a 0 sized list. I created the make_list_builder to construct builders (you recommended something similar when I first opened the PR). The two boolean parameters control whether to include a null in the builder and to include a particular fixed size list that has a null element.

alamb · 2024-04-07T17:28:08Z

arrow-array/src/builder/fixed_size_list_builder.rs

+                );
+                if !f.is_nullable() {
+                    assert!(
+                        values_data.null_count() == 0,


I think it is valid to have a zero length array (which would also have no nulls, but the null count would be zero)

Suggested change

values_data.null_count() == 0,

values_data.null_count() == 0 || values_data.len() == 0,

(same for the check below)

alamb · 2024-04-07T17:35:10Z

arrow-array/src/builder/fixed_size_list_builder.rs

@@ -248,9 +325,37 @@ mod tests {
    }

    #[test]
-    fn test_fixed_size_list_array_builder_finish_cloned() {
+    fn test_fixed_size_list_array_builder_with_field() {
+        let builder = make_list_builder(false, false);


If the field is set to non-nullable, this element will fail the null check. I wanted to check with you if this is logically right or not?

I think I would expect it to be consistent with whatever FixedSlizeListArray::try_new did

istvan-fodor · 2024-04-08T15:46:02Z

@alamb @tustvold , I added some changes.

null check now follows the FixedSizeListArray::try_new logic
Added two new unit tests to test scenario with nulls: test_fixed_size_list_array_builder_with_field_and_null and test_fixed_size_list_array_builder_cloned_with_field_and_null
Test cases for nullable fields and empty builders: test_fixed_size_list_array_builder_with_field_empty and test_fixed_size_list_array_builder_cloned_with_field_empty

…n-fodor/arrow-rs into feature/fixed-size-list-field-1353

alamb · 2024-04-09T12:53:01Z

Thanks you for sticking with this @istvan-fodor 🙏

Also, thank you @tustvold for the follow up in #5612 🚀

feat: implemented with_field() for FixedSizeListBuilder

d1bc1b0

github-actions bot added the arrow Changes to the arrow crate label Mar 21, 2024

istvan-fodor marked this pull request as draft March 21, 2024 20:11

alamb mentioned this pull request Mar 22, 2024

DataFusion weekly project plan (Andrew Lamb) - March 18, 2024 apache/datafusion#9675

Closed

7 tasks

alamb reviewed Mar 22, 2024

View reviewed changes

arrow-array/src/builder/fixed_size_list_builder.rs Show resolved Hide resolved

alamb reviewed Mar 22, 2024

View reviewed changes

arrow-array/src/builder/fixed_size_list_builder.rs Show resolved Hide resolved

Switched back to build_unchecked, assertions on array_data

83605bb

alamb marked this pull request as ready for review March 22, 2024 20:54

alamb mentioned this pull request Mar 22, 2024

Fix new clippy lints for Rust 1.77 #5544

Merged

alamb approved these changes Mar 22, 2024

View reviewed changes

decresed code repetition

09741dd

istvan-fodor commented Apr 3, 2024

View reviewed changes

alamb reviewed Apr 7, 2024

View reviewed changes

Fixed null logic

06a8cec

istvan-fodor and others added 2 commits April 8, 2024 10:49

Added unit tests for empty with field

052fd66

Merge branch 'apache:master' into feature/fixed-size-list-field-1353

ac768f6

alamb mentioned this pull request Apr 8, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 apache/datafusion#10002

Closed

9 tasks

istvan-fodor and others added 3 commits April 8, 2024 15:23

Fixed clippy issue

355c941

Merge branch 'feature/fixed-size-list-field-1353' of github.com:istva…

1999530

…n-fodor/arrow-rs into feature/fixed-size-list-field-1353

Merge branch 'apache:master' into feature/fixed-size-list-field-1353

ed48b9d

tustvold merged commit 1b3d1a9 into apache:master Apr 9, 2024
25 checks passed

tustvold mentioned this pull request Apr 9, 2024

Use FixedSizeListArray::new in FixedSizeListBuilder #5612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implemented with_field() for FixedSizeListBuilder #5541

feat: implemented with_field() for FixedSizeListBuilder #5541

istvan-fodor commented Mar 21, 2024 •

edited

Loading

alamb left a comment

alamb left a comment

alamb Mar 22, 2024

alamb Mar 22, 2024

istvan-fodor Mar 23, 2024

istvan-fodor commented Mar 27, 2024

tustvold commented Apr 3, 2024

istvan-fodor Apr 3, 2024

tustvold Apr 3, 2024 •

edited

Loading

alamb Apr 7, 2024

istvan-fodor commented Apr 3, 2024

SteampunkIslande commented Apr 4, 2024

tustvold commented Apr 4, 2024

SteampunkIslande commented Apr 5, 2024

alamb left a comment

alamb Apr 7, 2024

istvan-fodor Apr 8, 2024

alamb Apr 7, 2024

alamb Apr 7, 2024

istvan-fodor commented Apr 8, 2024 •

edited

Loading

alamb commented Apr 9, 2024

	values_data.null_count() == 0,
	values_data.null_count() == 0 \|\| values_data.len() == 0,

feat: implemented with_field() for FixedSizeListBuilder #5541

feat: implemented with_field() for FixedSizeListBuilder #5541

Conversation

istvan-fodor commented Mar 21, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istvan-fodor commented Mar 27, 2024

tustvold commented Apr 3, 2024

Choose a reason for hiding this comment

tustvold Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istvan-fodor commented Apr 3, 2024

SteampunkIslande commented Apr 4, 2024

tustvold commented Apr 4, 2024

SteampunkIslande commented Apr 5, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istvan-fodor commented Apr 8, 2024 • edited Loading

alamb commented Apr 9, 2024

istvan-fodor commented Mar 21, 2024 •

edited

Loading

tustvold Apr 3, 2024 •

edited

Loading

istvan-fodor commented Apr 8, 2024 •

edited

Loading