[WIP][VARIANT] Support reading basic variant #259

richardc-db · 2024-06-15T00:30:08Z

Still very much WIP.

Implements basic variant support.

To read variants, engines can read physical variants from storage as structs and implement (or use the default) "variant_coalesce" function, which is intended to create a Variant column vector from the struct input.

Because kernel-defaults uses Arrow as its in-memory data representation (and there isn't currently an arrow variant type), variants in kernel-defaults are simply an arrow struct with "value" and "metadata" binary child fields. A piece of metadata "isVariant" is inserted onto the variant struct field to differentiate between a struct and variant.

In the post-shredding future, the "variant_coalesce" function (which is currently more or less a no-op) will be used to rebuild shredded variants into their fully encoded representation.

Tested using an external golden table with variants and nested variants.

scovich

Made an initial pass. Overall impression is that we "care too much" about variant in "too many places" -- which complicates the code and makes it error-prone. Not sure the best way to address this shortcoming (differentiating logical vs. physical types might help)?

Two high-level questions I wasn't able to answer during the code review:

Is there a "clean" boundary between the variant logical type and the physical struct-of-binary type+data? Ideally, that boundary should be enforced very early in the query's lifecycle, so that almost all the kernel code doesn't have to care about it, and simply sees a struct that happens to carry a metadata field?
What does VariantCoalesce really do? I think it just ensures that each variant column has the correct number and ordering of physical sub-fields. But it seems like that should happen automatically (and much more cleanly) if we send the correct read schema to the file readers?

scovich · 2024-07-01T16:25:05Z

kernel/src/engine/arrow_conversion.rs

            .metadata()
            .iter()
            .map(|(key, val)| Ok((key.clone(), serde_json::to_string(val)?)))
-            .collect::<Result<_, serde_json::Error>>()
+            .collect::<Result<HashMap<String, String>, serde_json::Error>>()


I believe you can get rid of the turbofish by:

use itertools::Itertools; ... let mut metadata: HashMap<_, _> = f .metadata .iter() .map(...) .try_collect() .map_err(...)?;

scovich · 2024-07-01T16:38:29Z

kernel/src/engine/arrow_conversion.rs

+                            ArrowField::new("value", ArrowDataType::Binary, false),
+                            ArrowField::new("metadata", ArrowDataType::Binary, false),


I believe those need to be nullable, since a null variant field will be physically represented as a null entry in these arrays?

scovich · 2024-07-01T16:43:34Z

kernel/src/engine/arrow_expression.rs

@@ -89,6 +90,10 @@ impl Scalar {
                        Decimal128Array::new_null(num_rows)
                            .with_precision_and_scale(*precision, *scale as i8)?,
                    ),
+                    // TODO(r.chen): Fill this out correctly.
+                    PrimitiveType::Variant => {
+                        panic!("UNSUPPORTED - VARIANT DOESNT HAVE AN ARROW TYPE")


nit: could also just do

PrimitiveType::Variant =>unimplemented!(),

... but wouldn't the following suffice to implement the required functionality?

PrimitiveType::Variant => { // Arrow has no variant type, so we have to emit a null struct-of-arrays instead. let arrow_variant = ArrowDataType::try_from(DataType::VARIANT)?; let physical_variant = DataType::try_from(arrow_variant)?; Scalar::Null(physical_variant).to_array(num_rows) }

That said, it's a bit troubling that a round trip from kernel to arrow and back is not idempotent?
Do we need to introduce a concept of physical vs. logical data types in kernel, so that variant (logical type) is simply an array-of-struct (physical type) everywhere?

scovich · 2024-07-01T16:48:07Z

kernel/src/engine/arrow_expression.rs

@@ -228,13 +233,131 @@ fn ensure_data_types(kernel_type: &DataType, arrow_type: &ArrowDataType) -> Delt
            }
            Ok(())
        }
+        (DataType::Primitive(PrimitiveType::Variant), ArrowDataType::Struct(_)) => Ok(()),


Don't we need to verify the struct's schema? Otherwise weird errors could arise downstream...

Seems like we just need a recursive call to ensure_data_types, where the first arg is the physical variant schema (see comment on the null to_array case above), and the second arg is whatever got passed in?

scovich · 2024-07-01T16:50:36Z

kernel/src/engine/arrow_expression.rs

+                let new_arrays = arrays
+                    .into_iter()
+                    .zip(kernel_struct_type.fields().into_iter())
+                    .map(|(child_arr, struct_field)| {
+                        variant_coalesce_impl(child_arr, struct_field.data_type())
+                    })
+                    .collect::<Result<Vec<_>, _>>()?;


If you use itertools::Itertools, then this code simplifies to:

let new_arrays: Vec<_>= arrays ... .try_collect()?;

scovich · 2024-07-01T17:19:04Z

kernel/src/engine/arrow_expression.rs

+                    ))
+                }
+            }
+            _ => Ok(Arc::new(arr)),


I think this code only succeeds by accident, because Arc implements the AsRef trait...

Suggested change

_ => Ok(Arc::new(arr)),

_ => Ok(arr),

scovich · 2024-07-01T17:35:22Z

kernel/src/scan/mod.rs

+                        variant_coalesce_results.push(coalesced_batch);
+                    }
+                }
+                None => (),


This doesn't look right... if variant is disabled then the entire query result is thrown away?

Seems like it would be a lot cleaner to have a two pass approach:

At query compilation time, verify that variant mode is actually enabled if any variant column is found in the schema

At runtime, unconditionally apply VariantCoalesce, since it's anyway a no-op for non-variant columns

Then we don't need all this option and if/else control flow here?

scovich · 2024-07-01T17:43:01Z

kernel/src/schema.rs

@@ -418,7 +419,8 @@ impl Display for PrimitiveType {
            PrimitiveType::TimestampNtz => write!(f, "timestamp_ntz"),
            PrimitiveType::Decimal(precision, scale) => {
                write!(f, "decimal({},{})", precision, scale)
-            }
+            },


I don't think commas are required after case arms that use curly braces?

scovich · 2024-07-01T17:48:57Z

kernel/src/engine/arrow_expression.rs

+                            Ok::<usize, Error>(idx)
+                        } else {
+                            Err(Error::invalid_variant_representation(
+                                "\"value\" field is not of binary type.",


I think we usually surround column names with ' rather than "?
(better match with SQL, but also avoids needing escape characters here)

(several more below)

scovich · 2024-07-01T17:52:55Z

kernel/src/engine/arrow_expression.rs

+                if let Some(struct_array) = arr.as_any().downcast_ref::<StructArray>() {
+                    let (fields, arrays, nulls) = struct_array.clone().into_parts();
+
+                    let value_idx = if let Some((idx, field)) = fields.find("value") {


I think it's ok to require the physical schema to be in a particular order, rather than checking at runtime like this. In theory, kernel is -- or could be -- in charge of the schema that goes to parquet reader, so it should always match?

Also: If we do fix the field ordering, can the VariantCoalesce operation (and variant_coalesce_impl function) just disappear? Or are they accomplishing something else besides field reordering that I didn't notice?

richardc-db · 2024-08-12T06:52:41Z

hey @scovich, thanks for taking a look! Sorry for the very late reply, this PR is very much a WIP and was more of a hack for me to familiarize myself with the code base (and rust). But to answer some of your questions:

Is there a "clean" boundary between the variant logical type and the physical struct-of-binary type+data? Ideally, that boundary should be enforced very early in the query's lifecycle, so that almost all the kernel code doesn't have to care about it, and simply sees a struct that happens to carry a metadata field?

This makes sense. One thing I've discussed with Nick is a new API where the kernel can receive the parquet schema through the parquet footer. This way, the kernel will see just a struct (with a metadata field). The details of this are still being discussed, though.

What does VariantCoalesce really do? I think it just ensures that each variant column has the correct number and ordering of physical sub-fields. But it seems like that should happen automatically (and much more cleanly) if we send the correct read schema to the file readers?

yep, currently it doesn't really have a purpose. I have it in this PR just so I can understand the expression framework, but in the future, we'd like to have an expression which will "reconstruct" shredded variants. We intend that this "variant_coalesce" expression will do this in the future - i.e. the engine's scan will return an arbitrary struct, and the "variant_coalesce" expression will process it to return the struct of binaries.

richardc-db added 2 commits June 14, 2024 17:12

init

5dad25d

comments

fec536c

scovich reviewed Jul 1, 2024

View reviewed changes

zachschuermann self-requested a review July 18, 2024 01:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][VARIANT] Support reading basic variant #259

[WIP][VARIANT] Support reading basic variant #259

richardc-db commented Jun 15, 2024 •

edited

Loading

scovich left a comment •

edited

Loading

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

scovich Jul 1, 2024

richardc-db commented Aug 12, 2024 •

edited

Loading

		ArrowField::new("value", ArrowDataType::Binary, false),
		ArrowField::new("metadata", ArrowDataType::Binary, false),

[WIP][VARIANT] Support reading basic variant #259

Are you sure you want to change the base?

[WIP][VARIANT] Support reading basic variant #259

Conversation

richardc-db commented Jun 15, 2024 • edited Loading

scovich left a comment • edited Loading

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

richardc-db commented Aug 12, 2024 • edited Loading

richardc-db commented Jun 15, 2024 •

edited

Loading

scovich left a comment •

edited

Loading

richardc-db commented Aug 12, 2024 •

edited

Loading