-
Notifications
You must be signed in to change notification settings - Fork 360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: adopt kernel schema types #2495
base: main
Are you sure you want to change the base?
Conversation
ArrowDataType::Decimal128(p, s) => { | ||
Ok(DataType::Primitive(PrimitiveType::Decimal(*p, *s))) | ||
} | ||
ArrowDataType::Decimal256(p, s) => DataType::decimal(*p, *s).map_err(|_| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This arrow type is missing in kernel, I think that should be upstreamed there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, given that precision/scale are limited to 38, does that make sense? not entirely sure anymore but I think the 256 bit type only makes sense for larger p/s values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly for convenience. Users might have their source data using decimal256, but still precision/scale below 38.
@@ -1458,7 +1458,7 @@ def test_invalid_decimals(tmp_path: pathlib.Path, engine): | |||
|
|||
with pytest.raises( | |||
SchemaMismatchError, | |||
match=re.escape("Invalid data type for Delta Lake: decimal(39,1)"), | |||
match=re.escape("Invalid data type for Delta Lake: Decimal256(39, 1)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is a bit more significant then meets the eye - i.e. we are no longer trying to convert 256 bit decimals to a compliant type in delta. main reason being, for precisions that fit into 128 bit decimals, the user should really be using these, if the larger type is required, we cannot store it in the table anyhow.
crates/core/src/kernel/scalars.rs
Outdated
Struct(fields) => { | ||
let struct_fields = fields | ||
.iter() | ||
.flat_map(|f| TryFrom::try_from(f.as_ref())) | ||
.collect::<Vec<_>>(); | ||
let values = arr | ||
.as_any() | ||
.downcast_ref::<StructArray>() | ||
.and_then(|struct_arr| { | ||
struct_fields | ||
.iter() | ||
.map(|f: &StructField| { | ||
struct_arr | ||
.column_by_name(f.name()) | ||
.and_then(|c| Self::from_array(c.as_ref(), index)) | ||
}) | ||
.collect::<Option<Vec<_>>>() | ||
})?; | ||
if struct_fields.len() != values.len() { | ||
return None; | ||
} | ||
Some(Self::Struct( | ||
StructData::try_new(struct_fields, values).ok()?, | ||
)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scovich - here is an example of creating a struct scalar in the wild.
@@ -315,8 +315,8 @@ impl Snapshot { | |||
let stats_fields = if let Some(stats_cols) = self.table_config().stats_columns() { | |||
stats_cols | |||
.iter() | |||
.map(|col| match schema.field_with_name(col) { | |||
Ok(field) => match field.data_type() { | |||
.map(|col| match schema.field(col) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stats columns are defined for nested fields like parent_a.child_b
. Looking at kernel code, schema.field
will just look up by this name and fail. The reason I introduced #2519
Is #2519 appropriate for the kernel or should we support the nested lookup here and parse the stats col? Other uses of field_with_name should be audited to ensure they don't accept a nested name
Description
First pass adopting
delta_kernel
indelta-rs
.depends on delta-incubator/delta-kernel-rs#189 being merged and released.
This PR focusses on the schema types. Adopting the action types would be a follow up, but might have a even greater blast radius then this one.
Related Issue(s)
part of #2489
Documentation