xanderbailey commented on code in PR #2804:
URL: https://github.com/apache/iceberg-rust/pull/2804#discussion_r3792143210
##########
crates/integrations/datafusion/src/table/mod.rs:
##########
@@ -236,6 +251,77 @@ impl TableProvider for IcebergTableProvider {
}
}
+/// Collects the `write-default` values of a schema's top-level columns as
DataFusion
+/// expressions, keyed by column name.
+///
+/// Per the spec, writers must use `write-default` for columns that are not
supplied;
+/// DataFusion's insert planner consults these defaults for columns omitted
from an
+/// `INSERT` and falls back to `NULL` otherwise. Defaults of types that cannot
be
+/// expressed as a DataFusion scalar are skipped.
+fn column_defaults_from_schema(schema: &IcebergSchema) -> HashMap<String,
Expr> {
+ schema
+ .as_struct()
+ .fields()
+ .iter()
+ .filter_map(|field| {
+ let literal = field.write_default.as_ref()?;
+ let scalar = literal_to_scalar_value(&field.field_type, literal)?;
+ Some((field.name.clone(), Expr::Literal(scalar, None)))
+ })
+ .collect()
+}
+
+/// Converts an Iceberg literal of the given type into a DataFusion
[`ScalarValue`].
+///
+/// Returns `None` for combinations that have no scalar representation; the
insert
+/// planner casts the resulting expression to the target arrow type, so minor
+/// representation differences (e.g. timezone strings) are reconciled
downstream.
+fn literal_to_scalar_value(field_type: &Type, literal: &Literal) ->
Option<ScalarValue> {
+ let Type::Primitive(primitive_type) = field_type else {
+ return None;
+ };
+ let Literal::Primitive(primitive) = literal else {
+ return None;
+ };
+ Some(match (primitive_type, primitive) {
+ (PrimitiveType::Boolean, PrimitiveLiteral::Boolean(v)) =>
ScalarValue::Boolean(Some(*v)),
+ (PrimitiveType::Int, PrimitiveLiteral::Int(v)) =>
ScalarValue::Int32(Some(*v)),
+ (PrimitiveType::Long, PrimitiveLiteral::Long(v)) =>
ScalarValue::Int64(Some(*v)),
+ (PrimitiveType::Float, PrimitiveLiteral::Float(v)) =>
ScalarValue::Float32(Some(v.0)),
+ (PrimitiveType::Double, PrimitiveLiteral::Double(v)) =>
ScalarValue::Float64(Some(v.0)),
+ (PrimitiveType::String, PrimitiveLiteral::String(v)) =>
ScalarValue::Utf8(Some(v.clone())),
+ (PrimitiveType::Date, PrimitiveLiteral::Int(v)) =>
ScalarValue::Date32(Some(*v)),
+ (PrimitiveType::Time, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::Time64Microsecond(Some(*v))
+ }
+ (PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampMicrosecond(Some(*v), None)
+ }
+ (PrimitiveType::Timestamptz, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampMicrosecond(Some(*v),
Some(UTC_TIME_ZONE.into()))
+ }
+ (PrimitiveType::TimestampNs, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampNanosecond(Some(*v), None)
+ }
+ (PrimitiveType::TimestamptzNs, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampNanosecond(Some(*v),
Some(UTC_TIME_ZONE.into()))
+ }
+ (PrimitiveType::Decimal { precision, scale },
PrimitiveLiteral::Int128(v)) => {
+ ScalarValue::Decimal128(Some(*v), *precision as u8, *scale as i8)
+ }
+ (PrimitiveType::Binary, PrimitiveLiteral::Binary(v)) => {
+ ScalarValue::Binary(Some(v.clone()))
+ }
+ (PrimitiveType::Fixed(_), PrimitiveLiteral::Binary(v)) => {
Review Comment:
Minor: this sizes the `FixedSizeBinary` from the default *value's* length
(`v.len()`) rather than the column's declared `Fixed(len)`. It works today
because the planner casts to the target type, but a value whose length didn't
match the declared width would produce a differently-typed scalar. Reusing the
shared mapping (see comment above) would make this moot, otherwise a one-line
note might be worth it.
##########
crates/integrations/datafusion/src/table/mod.rs:
##########
@@ -236,6 +251,77 @@ impl TableProvider for IcebergTableProvider {
}
}
+/// Collects the `write-default` values of a schema's top-level columns as
DataFusion
+/// expressions, keyed by column name.
+///
+/// Per the spec, writers must use `write-default` for columns that are not
supplied;
+/// DataFusion's insert planner consults these defaults for columns omitted
from an
+/// `INSERT` and falls back to `NULL` otherwise. Defaults of types that cannot
be
+/// expressed as a DataFusion scalar are skipped.
+fn column_defaults_from_schema(schema: &IcebergSchema) -> HashMap<String,
Expr> {
+ schema
+ .as_struct()
+ .fields()
+ .iter()
+ .filter_map(|field| {
+ let literal = field.write_default.as_ref()?;
+ let scalar = literal_to_scalar_value(&field.field_type, literal)?;
+ Some((field.name.clone(), Expr::Literal(scalar, None)))
+ })
+ .collect()
+}
+
+/// Converts an Iceberg literal of the given type into a DataFusion
[`ScalarValue`].
+///
+/// Returns `None` for combinations that have no scalar representation; the
insert
+/// planner casts the resulting expression to the target arrow type, so minor
+/// representation differences (e.g. timezone strings) are reconciled
downstream.
+fn literal_to_scalar_value(field_type: &Type, literal: &Literal) ->
Option<ScalarValue> {
Review Comment:
`literal_to_scalar_value` re-derives the `(PrimitiveType, PrimitiveLiteral)`
→ arrow-type mapping that already exists in iceberg-core in a couple of places:
`get_arrow_datum` (`crates/iceberg/src/arrow/schema.rs`) and
`create_primitive_array_single_element` (`crates/iceberg/src/arrow/value.rs`).
This is effectively a fourth copy of that knowledge, and it can drift e.g. this
match already handles `Time`, which `get_arrow_datum` currently doesn't.
One option that reuses the tested mappings and lets DataFusion do the
array→scalar step:
```rust
let arrow_type = type_to_arrow_type(field_type)?; // already pub
let array = create_primitive_array_single_element(&arrow_type, &Some(lit))?;
ScalarValue::try_from_array(&array, 0).ok()
```
Bonus: the resulting scalar's arrow type already matches the column
(LargeBinary, FixedSizeBinary(len), UTC tz, …), which removes the reliance on
the downstream cast for reconciliation.
Tradeoff: `create_primitive_array_single_element` is `pub(crate)`, so this
needs promoting it to `pub` in iceberg-core. If you'd rather not expand the
core crate's public surface, keeping this match is reasonable, it's
self-contained. Flagging mainly so the duplication is a conscious choice. @CTTY
or @blackmwk might have stronger opinions.
##########
crates/integrations/datafusion/src/table/mod.rs:
##########
@@ -236,6 +251,77 @@ impl TableProvider for IcebergTableProvider {
}
}
+/// Collects the `write-default` values of a schema's top-level columns as
DataFusion
+/// expressions, keyed by column name.
+///
+/// Per the spec, writers must use `write-default` for columns that are not
supplied;
+/// DataFusion's insert planner consults these defaults for columns omitted
from an
+/// `INSERT` and falls back to `NULL` otherwise. Defaults of types that cannot
be
+/// expressed as a DataFusion scalar are skipped.
+fn column_defaults_from_schema(schema: &IcebergSchema) -> HashMap<String,
Expr> {
+ schema
+ .as_struct()
+ .fields()
+ .iter()
+ .filter_map(|field| {
+ let literal = field.write_default.as_ref()?;
+ let scalar = literal_to_scalar_value(&field.field_type, literal)?;
+ Some((field.name.clone(), Expr::Literal(scalar, None)))
+ })
+ .collect()
+}
+
+/// Converts an Iceberg literal of the given type into a DataFusion
[`ScalarValue`].
+///
+/// Returns `None` for combinations that have no scalar representation; the
insert
+/// planner casts the resulting expression to the target arrow type, so minor
+/// representation differences (e.g. timezone strings) are reconciled
downstream.
+fn literal_to_scalar_value(field_type: &Type, literal: &Literal) ->
Option<ScalarValue> {
+ let Type::Primitive(primitive_type) = field_type else {
+ return None;
+ };
+ let Literal::Primitive(primitive) = literal else {
+ return None;
+ };
+ Some(match (primitive_type, primitive) {
+ (PrimitiveType::Boolean, PrimitiveLiteral::Boolean(v)) =>
ScalarValue::Boolean(Some(*v)),
+ (PrimitiveType::Int, PrimitiveLiteral::Int(v)) =>
ScalarValue::Int32(Some(*v)),
+ (PrimitiveType::Long, PrimitiveLiteral::Long(v)) =>
ScalarValue::Int64(Some(*v)),
+ (PrimitiveType::Float, PrimitiveLiteral::Float(v)) =>
ScalarValue::Float32(Some(v.0)),
+ (PrimitiveType::Double, PrimitiveLiteral::Double(v)) =>
ScalarValue::Float64(Some(v.0)),
+ (PrimitiveType::String, PrimitiveLiteral::String(v)) =>
ScalarValue::Utf8(Some(v.clone())),
+ (PrimitiveType::Date, PrimitiveLiteral::Int(v)) =>
ScalarValue::Date32(Some(*v)),
+ (PrimitiveType::Time, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::Time64Microsecond(Some(*v))
+ }
+ (PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampMicrosecond(Some(*v), None)
+ }
+ (PrimitiveType::Timestamptz, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampMicrosecond(Some(*v),
Some(UTC_TIME_ZONE.into()))
+ }
+ (PrimitiveType::TimestampNs, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampNanosecond(Some(*v), None)
+ }
+ (PrimitiveType::TimestamptzNs, PrimitiveLiteral::Long(v)) => {
+ ScalarValue::TimestampNanosecond(Some(*v),
Some(UTC_TIME_ZONE.into()))
+ }
+ (PrimitiveType::Decimal { precision, scale },
PrimitiveLiteral::Int128(v)) => {
+ ScalarValue::Decimal128(Some(*v), *precision as u8, *scale as i8)
+ }
+ (PrimitiveType::Binary, PrimitiveLiteral::Binary(v)) => {
Review Comment:
Minor: `Binary` maps to `ScalarValue::Binary` here, whereas iceberg-core's
`type_to_arrow_type` maps `Binary` → `LargeBinary`
(`crates/iceberg/src/arrow/schema.rs`). The downstream cast reconciles it, so
not a correctness issue but this is another spot where reusing the shared
mapping would keep things aligned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
