apply a schema to fix column names #331

nicklan · 2024-09-10T17:25:30Z

Enforce/apply the schema given when we evaluate an expression.

Given we want to go to expression based fixup and allow the final schema to dictate the output, we will need to do this.

This code will fix-up at all levels of the output, which is messy in arrow since schemas are embedded all over the place. The schema is only applied if the output of the expression doesn't exactly match the passed schema.

codecov · 2024-09-10T17:33:10Z

Codecov Report

Attention: Patch coverage is 88.73786% with 58 lines in your changes missing coverage. Please review.

Project coverage is 78.85%. Comparing base (2b1c46f) to head (929a187).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/engine/arrow_expression.rs	73.07%	16 Missing and 19 partials ⚠️
kernel/src/engine/ensure_data_types.rs	94.04%	17 Missing and 3 partials ⚠️
kernel/src/engine/arrow_utils.rs	87.50%	0 Missing and 1 partial ⚠️
kernel/src/error.rs	0.00%	1 Missing ⚠️
kernel/src/scan/mod.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
+ Coverage   78.28%   78.85%   +0.56%     
==========================================
  Files          50       51       +1     
  Lines       10292    10640     +348     
  Branches    10292    10640     +348     
==========================================
+ Hits         8057     8390     +333     
  Misses       1781     1781              
- Partials      454      469      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich

Initial pass. Interested to understand better how this fits into the broader column mapping situation etc.

kernel/src/engine/arrow_expression.rs

kernel/src/error.rs

kernel/src/engine/arrow_utils.rs

scovich · 2024-10-09T15:38:11Z

kernel/src/engine/arrow_expression.rs

+// make column `col` with type `arrow_type` look like `kernel_type`. For now this only handles name
+// transforms. if the actual data types don't match, this will return an error


What about nullability? IIRC we have to read parquet with everything nullable, because parquet can't express the concept of a non-nullable field nesting inside a nullable field.

Or did we handle that already by just making everything nullable in our action schema?

We made all the top level structs nullable in the action schema (which is correct). But we do have non-nullable things inside. So I suspect maybe the final things don't match on nullability if the parquet schema really says everything is nullable. I will test and if we get mismatch we can fix that here too

If we do decide to allow non-nullable fields nested inside nullable fields, we'll have to verify that our default engine's row visitor handles it gracefully?

We do indeed have nullability mismatches in that the arrow schema says things can be null and our schema says they cannot.

I feel like this is something that might need a little thought to fix as it'll slow down metadata parsing if we have to go through and fix up the schema every time with nullability changes, although that will never be exposed to the connector.

There's also the issue of if we should adjust the metadata of each schema element, although that's less semantically important.

Thoughts?

The code now really makes the schema match the target, both for nullability and for metadata. This isn't cheap so we need to discuss it a bit probably

nit: can we add to this comment and give the result<option<...>> semantics? that is (AFAICT) we return Some(new array) for any complex types and None if we validated that the primitives match up?

See my other comment -- I think we can simplify the method contract

kernel/src/engine/arrow_expression.rs

scovich · 2024-10-09T19:56:27Z

kernel/src/scan/mod.rs

+fn make_data_type_physical(
+    logical_dt: &DataType,
+    column_mapping_mode: ColumnMappingMode,


FYI: Normally, the field's field id and physical name are stored in the field's metadata... but Iceberg requires field ids even for the internal columns used by Map and Array, and there's no way to associate metadata with those. So, when IcebergCompatV2 table feature is enabled, we have to remember the most recently-seen field, as well as the column path we descended through since then, so we can fetch the field ids out of that parent field's metadata.

Actually, I don't think Delta cares about those iceberg field ids -- even in column mapping field mode -- so maybe we can ignore all of this on the read path.

I'm going to close my eyes and hope you're right ;)

kernel/src/scan/mod.rs

nicklan

addressed some comments, not all. if I haven't replied to comment I'm still getting to it.

nicklan · 2024-10-10T00:30:21Z

kernel/src/engine/arrow_expression.rs

+// make column `col` with type `arrow_type` look like `kernel_type`. For now this only handles name
+// transforms. if the actual data types don't match, this will return an error


We made all the top level structs nullable in the action schema (which is correct). But we do have non-nullable things inside. So I suspect maybe the final things don't match on nullability if the parquet schema really says everything is nullable. I will test and if we get mismatch we can fix that here too

kernel/src/engine/arrow_utils.rs

kernel/src/engine/arrow_expression.rs

nicklan

thanks for the feedback!

kernel/src/engine/arrow_expression.rs

nicklan · 2024-10-18T21:48:00Z

kernel/src/engine/arrow_utils.rs

+                            arrow_field.is_nullable()
+                        )));
+                    }
+                    if &kernel_field.metadata_as_string() != arrow_field.metadata() {


Sadly both kernel and arrow metadata are just HashMap and we can't impl PartialEq<HashMap<X,Y>> for HashMap<X,Z> because HashMap isn't in our crate.

So I've done an impl to compare MetadataValue to a String, and written a helper method to compare the two hashmaps. Not as general but still pretty clean.

kernel/src/schema.rs

kernel/tests/golden_tables.rs

scovich

Getting so close!

Sorry for all the review passes and suggestions, but there's so much (and such complex) code that it's important to simplify it as much as possible if we want to stay sane...

scovich · 2024-10-18T22:10:11Z

kernel/src/engine/arrow_utils.rs

+    }
+    kernel_metadata
+        .iter()
+        .all(|(key, value)| arrow_metadata.get(key).map_or(false, |v| *value == *v))


hah, yep. You should let the rust guys know: https://doc.rust-lang.org/src/std/collections/hash/map.rs.html#1289

kernel/src/schema.rs

scovich · 2024-10-18T22:15:45Z

kernel/src/engine/arrow_expression.rs

Rescuing #331 (comment):

We're still only at 9% patch coverage. We might need to take @zachschuermann advice and make a schema just for this?

Maybe you could reuse this one from the schema depth checker test?
https://github.com/delta-incubator/delta-kernel-rs/blob/main/kernel/src/schema.rs#L1005
It has a wide variety of nested structures, at least.

Down to 7% now...

Yep, I've added some more tests and it's up now

kernel/src/engine/arrow_expression.rs

scovich · 2024-10-18T22:36:31Z

kernel/src/engine/arrow_expression.rs

+            ),
+        ])
+        .map(|(arrow_field, (target_type, nullable))| {
+            StructField::new(arrow_field.name(), target_type.clone(), nullable)


Just double checking -- no metadata here, because this is a hidden/internal struct that isn't part of the user-visible schema?

kernel/src/engine/arrow_expression.rs

nicklan

Thanks! Appreciate all the feedback. Adding tests actually uncovered that we weren't properly validating the nullability of array/map elements.

nicklan · 2024-10-18T23:29:07Z

kernel/src/engine/arrow_expression.rs

Yep, I've added some more tests and it's up now

kernel/src/engine/arrow_expression.rs

nicklan · 2024-10-18T23:31:40Z

kernel/src/engine/arrow_expression.rs

+            ),
+        ])
+        .map(|(arrow_field, (target_type, nullable))| {
+            StructField::new(arrow_field.name(), target_type.clone(), nullable)


nicklan · 2024-10-18T23:35:27Z

kernel/src/engine/arrow_utils.rs

+    }
+    kernel_metadata
+        .iter()
+        .all(|(key, value)| arrow_metadata.get(key).map_or(false, |v| *value == *v))


hah, yep. You should let the rust guys know: https://doc.rust-lang.org/src/std/collections/hash/map.rs.html#1289

kernel/src/engine/arrow_expression.rs

scovich

One last round of simplification and I think we're done!

kernel/src/engine/arrow_expression.rs

scovich · 2024-10-19T11:43:21Z

kernel/src/engine/arrow_expression.rs

+    let transformed_field = new_field_with_metadata(
+        field.name(),
+        transformed_values.data_type(),
+        target_inner_type.contains_null,
+        None,
+    );


Heh... after all that, I just realized that this one is simply

Suggested change

let transformed_field = new_field_with_metadata(

field.name(),

transformed_values.data_type(),

target_inner_type.contains_null,

None,

);

let transformed_field = ArrowField::new(

field.name(),

transformed_values.data_type().clone(),

target_inner_type.contains_null,

);

... which leaves new_field_with_metadata with only a single call site.

That said, we probably should keep the helper function, and update our impl TryFrom<&StructField> for ArrowField to use it.

Agree on updating that. Right now those things are protected by different feature flags.

I've filled #411 to sort this out and make the above suggested change.

scovich · 2024-10-19T11:48:28Z

kernel/src/engine/arrow_utils.rs

-        (
-            DataType::Primitive(PrimitiveType::Decimal(kernel_prec, kernel_scale)),
-            ArrowDataType::Decimal128(arrow_prec, arrow_scale),
-        ) if arrow_prec == kernel_prec && *arrow_scale == *kernel_scale as i8 => {
-            // decimal isn't primitive in arrow. cast above is okay as we limit range
-            Ok(DataTypeCompat::Identical)
-        }


Was this match arm deleted on purpose?
Does the cast-compat path cover it now, perhaps?

Yes, exactly. Should have noted that in a comment here.

scovich · 2024-10-19T11:50:57Z

kernel/src/engine/arrow_utils.rs

+) -> bool {
+    if kernel_metadata.len() != arrow_metadata.len() {


It's not rare for fields to have no metadata, so perhaps worth doing:

Suggested change

) -> bool {

if kernel_metadata.len() != arrow_metadata.len() {

) -> bool {

if kernel_metadata.is_empty() && arrow_metadata.is_empty() {

return true;

}

if kernel_metadata.len() != arrow_metadata.len() {

But on the other hand, an empty iterator should be very cheap to traverse, so meh?

is_empty() just calls self.len() == 0. So since we're optimizing here I just went with saving kernel len and then early exit if the lens are equal and kernel len is 0.

scovich · 2024-10-19T11:53:27Z

kernel/src/engine/arrow_utils.rs

@@ -126,15 +127,46 @@ fn can_upcast_to_decimal(
        && target_precision - source_precision >= (target_scale - source_scale) as u8
 }

+/// check if two fields have the same nullability and metadata
+fn test_nullability_and_metadata(


Naming? This sounds like a unit test at first glance... maybe something like verify_nullability_and_metadata_eq would be clearer?

Or maybe ensure_nullability_and_metadata, following the same convention as ensure_data_types?

Also: Suggest to split out ensure_nullability as a separate method, e.g.

fn ensure_nullability( kernel_nullable: bool, arrow_nullable: bool, context: impl FnOnce() -> impl Into<ToString>, ) -> DeltaResult<()> { if kernel_nullable == arrow_nullable { Ok(()) } else { Err(Error::Generic(format!( "{} {kernel_nullable} in kernel and {arrow_nullable} in arrow", context().into(), ))) } }

and then all three call sites can use it:

ensure_nullability( kernel_field.nullable, arrow_field.is_nullable(), || format!("Field {} has nullability", field.name), )?

and e.g.

if check_nullability_and_metadata { ensure_nullability( inner_type.contains_null, arrow_list_field.is_nullable(), || "List has contains_null", )?; }

We could go a step further and wrap all this up in a helper struct:

// Convenient public entry point pub(crate) fn ensure_data_types(...) -> DeltaResult<DataTypeCompat> { let check = EnsureDataTypes { check_nullability_and_metadata }; check.ensure_data_types(kernel_type, arrow_type) } struct EnsureDataTypes { check_nullability_and_metadata: bool, } impl EnsureDataTypes { fn ensure_data_types(&self, ...) -> DeltaResult<DataTypeCompat> { ... today's match statement, but simpler ... } fn ensure_nullability(&self, ...) -> DeltaResult<()> { if self.check_nullability_and_metadata && kernel_nullable != arrow_nullable => { Err(...) } else { _ => Ok(()), } } fn ensure_nullability_and_metadata(&self, ...) -> DeltaResult<()> { self.ensure_nullability(...)?; match self.check_nullability_and_metadata { true if !metadata_eq(&kernel_field.metadata, arrow_field.metadata()) => { Err(...) } _ => Ok(()), } }

Then, the implementation simplifies to things like:

(DataType::Array(inner_type), ArrowDataType::List(arrow_list_field)) => { self.ensure_nullability( inner_type.contains_null, arrow_list_field.is_nullable(), || "List has contains_null", )?; self.ensure_data_types( &inner_type.element_type, arrow_list_field.data_type(), )

Yep this is nicer.

I ended up also moving all of the ensure_x stuff to its own module as the utils mod was getting real big.

scovich · 2024-10-19T11:54:37Z

kernel/src/engine/arrow_utils.rs

+            if check_nullability_and_metadata
+                && inner_type.contains_null != arrow_list_field.is_nullable()


Why not just use the helper method? There should never be any metadata on either field, so that part will be a no-op?

Oh -- because we're not comparing two fields here!

(similar story for maps below)

Yeah. I split out that method thinking I would call it in multiple places, then realized I wouldn't, but this method is big enough that I thought having the nullabilty check pulled out still made sense.

nicklan · 2024-10-21T20:30:33Z

kernel/src/engine/mod.rs

@@ -5,26 +5,32 @@
 #[cfg(feature = "arrow-conversion")]
 pub(crate) mod arrow_conversion;

-#[cfg(feature = "arrow-expression")]
+#[cfg(all(


This was broken before because arrow_expression uses stuff in arrow_utils so enabling only arrow_expression would not have compiled.

nicklan · 2024-10-21T20:31:23Z

kernel/src/engine/mod.rs

 pub mod arrow_expression;

-#[cfg(any(feature = "default-engine", feature = "sync-engine"))]


Moved this to a macro to avoid the repeated checking of the two feature flags. Hopefully reviewers agree it's better :)

nicklan · 2024-10-21T20:32:51Z

kernel/src/engine/arrow_utils.rs

-}
-
-/// Capture the compatibility between two data-types, as passed to [`ensure_data_types`]
-pub(crate) enum DataTypeCompat {


note this all moved to ensure_data_types.rs

scovich

Nice!

Just a few suggestions to consider before merging.

kernel/src/engine/ensure_data_types.rs

scovich · 2024-10-21T21:08:55Z

kernel/src/engine/ensure_data_types.rs

+                )
+            }
+            (DataType::Map(kernel_map_type), ArrowDataType::Map(arrow_map_type, _)) => {
+                if let ArrowDataType::Struct(fields) = arrow_map_type.data_type() {


Good place for the let-else idiom?

let ArrowDataType::Struct(fields) = arrow_map_type.data_type() else { return Err(make_arrow_error("Arrow map type wasn't a struct.") };

scovich · 2024-10-21T21:12:26Z

kernel/src/engine/ensure_data_types.rs

+                    let mut fields = fields.iter();
+                    if let Some(key_type) = fields.next() {


Unlike kernel, arrow fields Deref to slice. So we should be able to do:

let [key_type, value_type] = fields else { return Err(...); }; self.ensure_data_types(... key ...); self.ensure_data_types(... value ...); self.ensure_nullability(... value ...);

kernel/src/engine/ensure_data_types.rs

kernel/src/engine/arrow_expression.rs

kernel/src/engine/ensure_data_types.rs

kernel/src/schema.rs

Co-authored-by: Ryan Johnson <[email protected]>

nicklan requested review from scovich, zachschuermann and roeap September 10, 2024 17:25

scovich reviewed Sep 10, 2024

View reviewed changes

Nick Lanham and others added 9 commits October 8, 2024 16:16

apply a schema to fix column names

5a7c419

remove an unwrap

31e1977

cleanup

5acbdb2

checkpoint with working rename

1ea7554

cleanup unneeded complexity, fmt

54493aa

add map types

532a870

fmt

a5feb17

only apply if needed

91ca99c

fmt+clippy

29b09f7

nicklan marked this pull request as ready for review October 8, 2024 23:33

nicklan force-pushed the fix-cm-name-take1 branch from 57f3c96 to 29b09f7 Compare October 8, 2024 23:34

nicklan requested a review from scovich October 8, 2024 23:35

scovich reviewed Oct 9, 2024

View reviewed changes

nicklan added 5 commits October 9, 2024 17:23

Merge branch 'main' into fix-cm-name-take1

451a76d

checkpoint

ada2f7b

Merge branch 'main' into fix-cm-name-take1

1633e49

checkpoint

4af1bbd

refactor for helpers

e0e3ed6

nicklan commented Oct 12, 2024

View reviewed changes

nicklan mentioned this pull request Oct 14, 2024

Add an efficient way in the arrow expression handler to check if an expression result needs a schema transform #396

Open

nicklan requested a review from scovich October 15, 2024 00:01

nicklan added 4 commits October 14, 2024 17:39

small bug fix

ce4d3f3

arg order

9d3b49d

checkpoint

69c1e27

Merge branch 'main' into fix-cm-name-take1

e232c77

nicklan commented Oct 18, 2024

View reviewed changes

nicklan requested a review from scovich October 18, 2024 21:59

nicklan added 4 commits October 18, 2024 15:01

fix comment

9cbd992

comment

17a5303

more consistent naming

ae67479

Merge branch 'main' into fix-cm-name-take1

b2e143a

scovich reviewed Oct 18, 2024

View reviewed changes

nicklan added 4 commits October 18, 2024 16:24

add tests

0b48b4c

address some comments

65b6218

ArrayRef all the things

479758d

refactor to new_field_with_metadata

efc5193

nicklan commented Oct 18, 2024

View reviewed changes

nicklan requested a review from scovich October 18, 2024 23:50

scovich reviewed Oct 19, 2024

View reviewed changes

nicklan added 4 commits October 21, 2024 12:46

move ensure_data_types to own module

e02357a

avoid feature flag repetition

eb786d8

switch to struct for ensure

d9af456

address final comments, and make feature flags at least consistent

622c03f

nicklan mentioned this pull request Oct 21, 2024

Sort out feature flags for everything in the engine mod #411

Open

Merge branch 'main' into fix-cm-name-take1

dd65781

nicklan commented Oct 21, 2024

View reviewed changes

nicklan requested a review from scovich October 21, 2024 20:32

nicklan commented Oct 21, 2024

View reviewed changes

scovich approved these changes Oct 21, 2024

View reviewed changes

Apply suggestions from code review

ecbd668

Co-authored-by: Ryan Johnson <[email protected]>

github-actions bot added the breaking-change Change that will require a version bump label Oct 21, 2024

final comments

929a187

nicklan removed the breaking-change Change that will require a version bump label Oct 21, 2024

nicklan merged commit cd53bc1 into delta-io:main Oct 21, 2024
17 checks passed

		// make column `col` with type `arrow_type` look like `kernel_type`. For now this only handles name
		// transforms. if the actual data types don't match, this will return an error

	.all(\|(key, value)\| arrow_metadata.get(key).map_or(false, \|v\| value == v))
	.all(\|(key, value)\| arrow_metadata.get(key).is_some_and(\|v\| value == v))

		) -> bool {
		if kernel_metadata.len() != arrow_metadata.len() {

		if check_nullability_and_metadata
		&& inner_type.contains_null != arrow_list_field.is_nullable()

		pub mod arrow_expression;

		#[cfg(any(feature = "default-engine", feature = "sync-engine"))]

		let mut fields = fields.iter();
		if let Some(key_type) = fields.next() {

apply a schema to fix column names #331

apply a schema to fix column names #331

Conversation

nicklan commented Sep 10, 2024 • edited Loading

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

scovich 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

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

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan 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

scovich 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

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

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

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

scovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading