Add Fields abstraction (#3955) by tustvold · Pull Request #3965 · apache/arrow-rs

tustvold · 2023-03-27T20:35:43Z

Which issue does this PR close?

Part of #3955

Rationale for this change

This adds a cheaply cloneable Fields abstraction, that internally contains FieldRef within a reference counted slice.

This achieves a couple of things:

The use of FieldRef allows projecting / reconstructing schema without needing to copy Field
The use of Arc<[FieldRef]> allows cheap cloning of DataType, construction of DataType::Struct, etc...
Allows cheap pointer comparisons between Schema and DataType
Provides a potential extension point for faster field name lookups

What changes are included in this PR?

Are there any user-facing changes?

Yes, this makes a fundamental change to the schema representation

tustvold · 2023-03-27T20:37:25Z

arrow-schema/src/fields.rs

I originally defined Fields = Vec<FieldPtr>

Whilst simple the lack of a newtype made for a more convoluted migration, with a newtype we can define conversions From<Vec<Field>>, etc... to help reduce friction

I agree this is a better formulation than a typedef and will allow for more flexibility

tustvold · 2023-03-27T20:37:54Z

arrow-schema/src/datatype.rs

A quick follow PR would then replace Box<Field> with FieldRef

Another followup could be done for Union, although that would also benefit from a Vec<(Field, i8)> instead of two separate vectors. I think that also currently makes it the largest variant, which increases the needed size of all datatypes.

A slightly hacky improvement for union could also be to move the type_id into Field and leave it unused in most places. That should basically be free since Field already has a few bits of padding left.

Yes, I plan to do the other variants in a follow up. I think changing it to (Fields, Arc<[i8]>, UnionMode) may be sufficient and would keep things simple

tustvold · 2023-03-27T20:44:45Z

arrow-schema/src/fields.rs

It is perhaps worth highlighting that this is implemented as a memove, it cannot reuse the allocation

it cannot reuse the allocation

If the vector is allocation is oversized. I think it will reuse the allocation if the vector is at capacity (which is rare though).

Sadly the implementation will always move regardless, I think it is some limitation of unsized coercion

tustvold · 2023-03-27T20:45:10Z

FYI @alamb @crepererum @viirya I would appreciate your thoughts on this

crepererum

I like it 👍

arrow-schema/src/fields.rs

crepererum · 2023-03-28T08:43:01Z

arrow-schema/src/fields.rs

it cannot reuse the allocation

If the vector is allocation is oversized. I think it will reuse the allocation if the vector is at capacity (which is rare though).

alamb

I also like it

alamb · 2023-03-28T10:14:36Z

arrow-schema/src/fields.rs

I agree this is a better formulation than a typedef and will allow for more flexibility

alamb · 2023-03-28T10:14:49Z

arrow-schema/src/fields.rs

While constructing / Modifying lists of fields, I think it would be great if we could also add functions like

/// Maybe something more generic to allow adding a Field and FieldREf pub fn push(mut &self, field: Field...) { ... }

Yeah, I think we can make SchemaBuilder public and add such a method to it, Fields itself is inherently immutable

alamb · 2023-03-28T10:16:24Z

arrow-schema/src/schema.rs

This I think is one of the most expensive operations in DataFusion planning now: apache/datafusion#5157 (comment)

So 👍

tustvold · 2023-03-28T17:16:41Z

arrow-array/src/array/struct_array.rs

-            },
-        );
-
+        let iter = v.into_iter();


I hope to rework these once StructArray::new exists as part of #3880

tustvold · 2023-03-28T17:17:18Z

arrow-array/src/array/struct_array.rs

    }
 }

+impl From<RecordBatch> for StructArray {


This replaces the existing implementation in record_batch.rs with a more optimal implementation

tustvold · 2023-03-28T17:17:37Z

arrow-array/src/record_batch.rs

    }
 }

-impl From<RecordBatch> for StructArray {


Moved to struct_array.rs

tustvold · 2023-03-28T17:18:35Z

arrow-schema/src/fields.rs

+        self.iter().map(|field| field.size()).sum()
+    }
+
+    /// Searches for a field by name, returning it along with its index if found


This will be an obvious place to add hash based lookup or similar

viirya · 2023-03-28T18:21:33Z

arrow-array/src/array/mod.rs

        let struct_type =
-            DataType::Struct(vec![Field::new("data", DataType::Int64, false)]);
+            DataType::Struct(vec![Field::new("data", DataType::Int64, false)].into());


into works like Fields::from here ?

Yeah, I switched between the two to make rustfmt happy 😅

viirya · 2023-03-28T19:04:25Z

arrow-schema/src/fields.rs

+
+/// A cheaply cloneable, owned slice of [`FieldRef`]
+///
+/// Similar to `Arc<Vec<FieldPtr>>` or `Arc<[FieldPtr]>`


FieldPtr? Do you mean FieldRef?

viirya

This abstraction looks good and datatype/schema manipulation can be more efficient.

tustvold · 2023-03-29T15:50:15Z

Notified the mailing list about this - https://lists.apache.org/thread/pmxq5j864qlkp36lvxg8kvk0kct56r8m

alamb

Thank you @tustvold -- I think this looks in general very good.

My biggest concern is on the amount of API churn that this will generate -- I think there may be a way to reduce the churn and make this PR smaller, and I left comments to that effect.

Once we sort it out and get this merged, I think we should then try (almost immediately) to upgrade some other project that makes significant use of arrow-rs to see how painful the upgrade is (and if there are other ergonomic things that could be done to ease the transition pain)

Thank you again for pushing this through

arrow-array/src/builder/struct_builder.rs

alamb · 2023-03-29T21:34:08Z

arrow-integration-test/src/schema.rs

                Field::new(
                    "c25",
-                    DataType::Struct(vec![
+                    DataType::Struct(Fields::from(vec![


For anyone who uses DataType::Struct this is now getting complicated to construct

I wonder if we can ease the pain by having something

impl DataType { fn new_struct(fields: impl Into<Fields>) -> Self { .. }

So then this could be

Suggested change

DataType::Struct(Fields::from(vec![

DataType::new_struct(vec![

I'm not convinced by this, the major reason for using the more verbose DataType::Struct(Fields::from(..)) was to reduce formatting churn, most downstreams will just be able to use .into().

I'll have a go upgrading DataFusion to assess the churn required

I guess in my mind it is about the cognative load. Now I need to know what a Fields is, import it, construct one, etc.

Maybe the magic into() will make it ok

tustvold · 2023-03-29T23:01:09Z

DataFusion upgrade PR - apache/datafusion#5782

tustvold · 2023-03-30T10:57:34Z

There don't appear to be any objections to this, and there is plenty of time until the next release, and so I am going to get this in before it develops merge conflicts. We can continue to iterate from there

github-actions bot added the arrow Changes to the arrow crate label Mar 27, 2023

tustvold commented Mar 27, 2023

View reviewed changes

tustvold force-pushed the add-fields branch from 99f19b6 to e48e93f Compare March 27, 2023 20:42

tustvold commented Mar 27, 2023

View reviewed changes

crepererum approved these changes Mar 28, 2023

View reviewed changes

alamb reviewed Mar 28, 2023

View reviewed changes

alamb mentioned this pull request Mar 28, 2023

Use Arc<str> inside Field (#3955) #3956

Closed

Add Fields abstraction (apache#3955)

fa9c067

tustvold force-pushed the add-fields branch from e48e93f to fa9c067 Compare March 28, 2023 17:14

github-actions bot added arrow-flight Changes to the arrow-flight crate parquet Changes to the parquet crate labels Mar 28, 2023

tustvold commented Mar 28, 2023

View reviewed changes

arrow-array/src/record_batch.rs

}

}

impl From<RecordBatch> for StructArray {

Copy link

Contributor Author

tustvold Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to struct_array.rs

tustvold commented Mar 28, 2023

View reviewed changes

tustvold added 2 commits March 28, 2023 18:59

Fix ffi

83d3d60

Fix array_cast

8e1578a

tustvold marked this pull request as ready for review March 28, 2023 18:37

Fix doc

9b6e0d3

viirya reviewed Mar 28, 2023

View reviewed changes

tustvold mentioned this pull request Mar 29, 2023

Store Timezone as Arc<str> #3976

Merged

alamb approved these changes Mar 29, 2023

View reviewed changes

tustvold added 2 commits March 29, 2023 23:03

Add Into<Fields> for StructBuilder

f243315

Further SchemaBuilder utilities

bc70e09

tustvold merged commit a9ac325 into apache:master Mar 30, 2023

This was referenced Apr 6, 2023

Add Fields::contains #4026

Merged

Reduce Cloning of Field #3955

Closed

tustvold mentioned this pull request Apr 21, 2023

First-Class Array Abstractions #3880

Closed

alamb mentioned this pull request Mar 19, 2024

refactor: make dfschema wrap schemaref apache/datafusion#9595

Merged

	DataType::Struct(Fields::from(vec![
	DataType::new_struct(vec![

Comments

Conversation

tustvold commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

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!

tustvold commented Mar 27, 2023

Uh oh!

crepererum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

tustvold Mar 28, 2023 • 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!

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!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

tustvold commented Mar 29, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

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!

tustvold commented Mar 29, 2023

Uh oh!

tustvold commented Mar 30, 2023

Uh oh!

Reviewers

tustvold commented Mar 27, 2023 •

edited

Loading

tustvold Mar 28, 2023 •

edited

Loading