Skip to content

Commit 2f4b237

Browse files
committed
feat!: add reader timezone to map-to-struct
1 parent 9b25e81 commit 2f4b237

18 files changed

Lines changed: 792 additions & 89 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

datafusion-executor/Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

datafusion-executor/src/expression.rs

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -390,13 +390,11 @@ fn struct_columns_from_patch(
390390
/// types come from `output_type`, which must be a struct holding only primitive fields (matching
391391
/// the kernel evaluator, which supports only primitive targets).
392392
///
393-
/// Each field extracts its value with `cast(get_field(map, name), T)`. For a numeric or temporal
394-
/// type the raw value is first wrapped in `nullif(.., '')`, mapping an empty string to null before
395-
/// the cast, so an empty string becomes null (kernel's `empty_string_partition_cast`) while an
396-
/// unparseable value fails the cast (kernel's hard parse error). String and Binary keep the raw
397-
/// value (empty is a valid empty string / empty bytes). A missing key or null value is already null
398-
/// via [`get_field`]. The whole struct is nulled where the input map row is null, via `<map> IS NOT
399-
/// NULL`.
393+
/// Each field extracts its value with `cast(get_field(map, name), T)`. For every type except String
394+
/// and Binary, the raw value is first wrapped in `nullif(.., '')`, mapping an empty string to null
395+
/// before the cast. String and Binary keep the raw value because empty strings and bytes are valid.
396+
/// A missing key or null value is already null via [`get_field`]. The whole struct is nulled where
397+
/// the input map row is null, via `<map> IS NOT NULL`.
400398
///
401399
/// KNOWN DIVERGENCES from the kernel parser, confined to malformed or non-spec-compliant values
402400
/// (spec-compliant writers never emit them):
@@ -408,14 +406,20 @@ fn struct_columns_from_patch(
408406
/// value's scale to match the target's exactly (and hard-errors otherwise).
409407
///
410408
/// # Errors
411-
/// Returns an error when `output_type` is absent, not a struct, or has a non-primitive field, or
412-
/// from lowering the map expression.
409+
///
410+
/// Returns an error when options are configured, `output_type` is absent, not a struct, or has a
411+
/// non-primitive field, or from lowering the map expression.
413412
fn map_to_struct_to_df_expr(
414413
map_to_struct: &MapToStructExpression,
415414
input_schema: &StructType,
416415
output_type: Option<&KernelDataType>,
417416
) -> DeltaResult<DFExpr> {
418417
let target = require_struct_output(output_type, "MapToStruct")?;
418+
if !can_lower_map_to_struct_options(map_to_struct) {
419+
return Err(Error::unsupported(
420+
"DataFusion execution of MapToStruct with configured options",
421+
));
422+
}
419423
let map = to_df_expr(&map_to_struct.map_expr, input_schema, None)?;
420424

421425
let mut args = Vec::with_capacity(target.num_fields() * 2);
@@ -445,6 +449,10 @@ fn map_to_struct_to_df_expr(
445449
Ok(struct_null_when_not(map.is_not_null(), named_struct(args)))
446450
}
447451

452+
fn can_lower_map_to_struct_options(map_to_struct: &MapToStructExpression) -> bool {
453+
map_to_struct.options.is_default()
454+
}
455+
448456
/// Lowers a `ParseJson` (parse a JSON-string column into a struct) to a call of the
449457
/// [`ParseJsonUdf`] scalar UDF, which delegates to kernel's own JSON parser. Unlike the
450458
/// struct-shaped arms, `ParseJson` is self-typed -- it carries its target `output_schema` -- so it
@@ -547,7 +555,7 @@ mod tests {
547555
use datafusion::physical_expr::execution_props::ExecutionProps;
548556
use delta_kernel::expressions::{
549557
col, lit, null_lit, ColumnName as KernelColumnName, Expression as KernelExpr,
550-
ExpressionStructPatch, ExpressionStructPatchBuilder,
558+
ExpressionStructPatch, ExpressionStructPatchBuilder, MapToStructOptions,
551559
};
552560
use delta_kernel::schema::{schema, schema_ref, ArrayType, DataType, MapType, StructType};
553561
use rstest::rstest;
@@ -1001,7 +1009,7 @@ mod tests {
10011009
/// Lowers a `MapToStruct` over `pv` targeting `output_schema` and renders it as a `Display`
10021010
/// string.
10031011
fn lower_map_to_struct(output_schema: StructType) -> String {
1004-
let kernel = KernelExpr::map_to_struct(col!("pv"));
1012+
let kernel = KernelExpr::map_to_struct(col!("pv"), MapToStructOptions::default());
10051013
let target: DataType = output_schema.into();
10061014
to_df_expr(&kernel, &pv_map_schema(), Some(&target))
10071015
.unwrap()
@@ -1063,13 +1071,28 @@ mod tests {
10631071
#[case] output_type: Option<DataType>,
10641072
#[case] expected_message: &str,
10651073
) {
1066-
let kernel = KernelExpr::map_to_struct(col!("pv"));
1074+
let kernel = KernelExpr::map_to_struct(col!("pv"), MapToStructOptions::default());
10671075
let err = to_df_expr(&kernel, &pv_map_schema(), output_type.as_ref())
10681076
.unwrap_err()
10691077
.to_string();
10701078
assert!(err.contains(expected_message), "{err}");
10711079
}
10721080

1081+
#[test]
1082+
fn configured_map_to_struct_is_unsupported() {
1083+
let target = DataType::from(schema! { nullable "ts": TIMESTAMP });
1084+
let kernel = KernelExpr::map_to_struct(
1085+
col!("pv"),
1086+
MapToStructOptions::default().with_timestamp_timezone("America/Los_Angeles"),
1087+
);
1088+
1089+
let error = to_df_expr(&kernel, &pv_map_schema(), Some(&target))
1090+
.unwrap_err()
1091+
.to_string();
1092+
1093+
assert!(error.contains("MapToStruct with configured options"));
1094+
}
1095+
10731096
// === ParseJson Shared Helpers ===
10741097

10751098
/// Input schema for JSON tests: `{ j: string }`.

ffi/src/expressions/engine_visitor.rs

Lines changed: 94 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,9 @@ pub struct EngineExpressionVisitor {
174174
/// `child_list_id`. The `output_schema` handle specifies the schema to parse the JSON
175175
/// into.
176176
pub visit_parse_json: VisitParseJsonFn,
177-
/// Visits the `MapToStruct` expression belonging to the list identified by `sibling_list_id`.
178-
/// The sub-expression (map column) will be in a _one_ item list identified by `child_list_id`.
179-
/// The output struct schema is determined by the evaluator's result type.
177+
/// Visits a `MapToStruct` expression with default options. Expressions with configured options
178+
/// are reported through `visit_unknown` without visiting the child expression. The
179+
/// sub-expression is in the one-item list identified by `child_list_id`.
180180
pub visit_map_to_struct: VisitUnaryFn,
181181
/// Visits the `LessThan` binary operator belonging to the list identified by
182182
/// `sibling_list_id`. The operands will be in a _two_ item list identified by
@@ -696,7 +696,10 @@ fn visit_expression_impl(
696696
schema_handle
697697
);
698698
}
699-
Expression::MapToStruct(MapToStructExpression { map_expr }) => {
699+
Expression::MapToStruct(map_to_struct) if !can_visit_map_to_struct(map_to_struct) => {
700+
visit_unknown(visitor, sibling_list_id, "configured_map_to_struct")
701+
}
702+
Expression::MapToStruct(MapToStructExpression { map_expr, .. }) => {
700703
let child_list_id = call!(visitor, make_field_list, 1);
701704
visit_expression_impl(visitor, map_expr, child_list_id);
702705
call!(visitor, visit_map_to_struct, sibling_list_id, child_list_id);
@@ -711,6 +714,10 @@ fn visit_expression_impl(
711714
}
712715
}
713716

717+
fn can_visit_map_to_struct(map_to_struct: &MapToStructExpression) -> bool {
718+
map_to_struct.options.is_default()
719+
}
720+
714721
fn visit_predicate_impl(
715722
visitor: &mut EngineExpressionVisitor,
716723
predicate: &Predicate,
@@ -771,7 +778,7 @@ fn visit_predicate_internal(predicate: &Predicate, visitor: &mut EngineExpressio
771778

772779
#[cfg(test)]
773780
mod tests {
774-
use delta_kernel::expressions::{lit, Expression, Scalar};
781+
use delta_kernel::expressions::{lit, Expression, MapToStructOptions, Scalar};
775782
use rstest::rstest;
776783

777784
use super::*;
@@ -791,6 +798,14 @@ mod tests {
791798
sibling_list_id: usize,
792799
parts: Vec<String>,
793800
},
801+
Unknown {
802+
sibling_list_id: usize,
803+
name: String,
804+
},
805+
MapToStruct {
806+
sibling_list_id: usize,
807+
child_list_id: usize,
808+
},
794809
}
795810

796811
#[derive(Default)]
@@ -847,6 +862,30 @@ mod tests {
847862
});
848863
}
849864

865+
extern "C" fn visit_unknown_name(
866+
data: *mut c_void,
867+
sibling_list_id: usize,
868+
name: KernelStringSlice,
869+
) {
870+
let builder = unsafe { &mut *(data as *mut TestExpressionBuilder) };
871+
let name = unsafe { String::try_from_slice(&name) }.unwrap();
872+
builder.events.push(LiteralEvent::Unknown {
873+
sibling_list_id,
874+
name,
875+
});
876+
}
877+
878+
extern "C" fn visit_map_to_struct(
879+
data: *mut c_void,
880+
sibling_list_id: usize,
881+
child_list_id: usize,
882+
) {
883+
let builder = unsafe { &mut *(data as *mut TestExpressionBuilder) };
884+
builder.events.push(LiteralEvent::MapToStruct {
885+
sibling_list_id,
886+
child_list_id,
887+
});
888+
}
850889
macro_rules! ignore_fn {
851890
($fn_name:ident $(, $arg_type:ty)*) => {
852891
extern "C" fn $fn_name(
@@ -907,7 +946,7 @@ mod tests {
907946
visit_is_null: ignore_child_list,
908947
visit_to_json: ignore_child_list,
909948
visit_parse_json: ignore_parse_json,
910-
visit_map_to_struct: ignore_child_list,
949+
visit_map_to_struct,
911950
visit_lt: ignore_child_list,
912951
visit_gt: ignore_child_list,
913952
visit_eq: ignore_child_list,
@@ -925,7 +964,7 @@ mod tests {
925964
visit_field_patch: ignore_field_patch,
926965
visit_opaque_expr: ignore_opaque_expr,
927966
visit_opaque_pred: ignore_opaque_pred,
928-
visit_unknown: ignore_string_slice,
967+
visit_unknown: visit_unknown_name,
929968
}
930969
}
931970

@@ -992,4 +1031,52 @@ mod tests {
9921031
assert_eq!(top_level_id, 0);
9931032
assert_eq!(builder.events, vec![expected]);
9941033
}
1034+
1035+
#[test]
1036+
fn timezone_aware_map_to_struct_visits_unknown() {
1037+
let expression = Expression::map_to_struct(
1038+
Expression::column(["partitionValues"]),
1039+
MapToStructOptions::default().with_timestamp_timezone("America/Los_Angeles"),
1040+
);
1041+
let mut builder = TestExpressionBuilder::default();
1042+
let mut visitor = test_visitor(&mut builder);
1043+
1044+
let top_level_id = visit_expression_internal(&expression, &mut visitor);
1045+
1046+
assert_eq!(top_level_id, 0);
1047+
assert_eq!(
1048+
builder.events,
1049+
vec![LiteralEvent::Unknown {
1050+
sibling_list_id: 0,
1051+
name: "configured_map_to_struct".to_string(),
1052+
}]
1053+
);
1054+
}
1055+
1056+
#[test]
1057+
fn default_map_to_struct_visits_child_then_map_to_struct() {
1058+
let expression = Expression::map_to_struct(
1059+
Expression::column(["partitionValues"]),
1060+
MapToStructOptions::default(),
1061+
);
1062+
let mut builder = TestExpressionBuilder::default();
1063+
let mut visitor = test_visitor(&mut builder);
1064+
1065+
let top_level_id = visit_expression_internal(&expression, &mut visitor);
1066+
1067+
assert_eq!(top_level_id, 0);
1068+
assert_eq!(
1069+
builder.events,
1070+
vec![
1071+
LiteralEvent::Column {
1072+
sibling_list_id: 1,
1073+
parts: vec!["partitionValues".to_string()],
1074+
},
1075+
LiteralEvent::MapToStruct {
1076+
sibling_list_id: 0,
1077+
child_list_id: 1,
1078+
},
1079+
]
1080+
);
1081+
}
9951082
}

ffi/src/expressions/kernel_visitor.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use std::sync::Arc;
66
use delta_kernel::engine::arrow_expression::opaque::ArrowOpaquePredicate;
77
use delta_kernel::expressions::{
88
lit, null_lit, BinaryExpressionOp, BinaryPredicateOp, ColumnName, Expression,
9-
JunctionPredicateOp, Predicate, Scalar, UnaryPredicateOp,
9+
JunctionPredicateOp, MapToStructOptions, Predicate, Scalar, UnaryPredicateOp,
1010
};
1111
use delta_kernel::schema::{DataType, PrimitiveType};
1212
use delta_kernel::DeltaResult;
@@ -692,7 +692,10 @@ pub extern "C" fn visit_expression_map_to_struct(
692692
child_expr: usize,
693693
) -> usize {
694694
unwrap_kernel_expression(state, child_expr).map_or(0, |expr| {
695-
wrap_expression(state, Expression::map_to_struct(expr))
695+
wrap_expression(
696+
state,
697+
Expression::map_to_struct(expr, MapToStructOptions::default()),
698+
)
696699
})
697700
}
698701

ffi/src/test_ffi.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ use std::sync::Arc;
66

77
use delta_kernel::expressions::{
88
col, column_name, column_pred, lit, null_lit, ArrayData, BinaryExpressionOp, BinaryPredicateOp,
9-
Expression as Expr, ExpressionStructPatchBuilder, MapData, OpaqueExpressionOp,
10-
OpaquePredicateOp, Predicate as Pred, Scalar, ScalarExpressionEvaluator, StructData,
9+
Expression as Expr, ExpressionStructPatchBuilder, MapData, MapToStructOptions,
10+
OpaqueExpressionOp, OpaquePredicateOp, Predicate as Pred, Scalar, ScalarExpressionEvaluator,
11+
StructData,
1112
};
1213
use delta_kernel::kernel_predicates::{
1314
DirectDataSkippingPredicateEvaluator, DirectPredicateEvaluator,
@@ -158,7 +159,7 @@ pub unsafe extern "C" fn get_testing_kernel_expression() -> Handle<SharedExpress
158159
Expr::struct_from([lit(5_i32), lit(20_i64)]),
159160
Expr::opaque(OpaqueTestOp("foo".to_string()), vec![lit(42), lit(1.111)]),
160161
Expr::unknown("mystery"),
161-
Expr::map_to_struct(col!("pv")),
162+
Expr::map_to_struct(col!("pv"), MapToStructOptions::default()),
162163
Expr::coalesce([col!("col"), lit(0_i32)]),
163164
Expr::array([lit(1_i32), lit(2_i32)]),
164165
];
@@ -254,7 +255,7 @@ pub unsafe extern "C" fn get_simple_testing_kernel_expression() -> Handle<Shared
254255
Expr::binary(BinaryExpressionOp::Multiply, lit(5), lit(6)),
255256
Expr::binary(BinaryExpressionOp::Divide, lit(100), lit(4)),
256257
Expr::struct_from([lit(1_i32), lit(2_i64), lit(3.0_f64)]),
257-
Expr::map_to_struct(col!("partitionValues")),
258+
Expr::map_to_struct(col!("partitionValues"), MapToStructOptions::default()),
258259
];
259260
Arc::new(Expr::struct_from(sub_exprs)).into()
260261
}

kernel/Cargo.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ pre-release-hook = [
4242
delta_kernel_derive = { path = "../derive-macros", version = "0.28.0" }
4343
bytes = "1.10"
4444
chrono = "0.4.41"
45+
chrono-tz = { version = "0.10.4", optional = true }
4546
crc = "3.2.2"
4647
indexmap = "2.10.0"
4748
itertools = "0.14"
@@ -109,7 +110,7 @@ need-arrow = [] # need-arrow is a marker that the feature needs arrow dep
109110
arrow-58 = ["dep:arrow_58", "dep:parquet_58", "dep:object_store_13"]
110111
arrow-59 = ["dep:arrow_59", "dep:parquet_59", "dep:object_store_13"]
111112
arrow-conversion = ["need-arrow"]
112-
arrow-expression = ["need-arrow"]
113+
arrow-expression = ["need-arrow", "dep:chrono-tz"]
113114

114115
# Schema diffing functionality (experimental)
115116
schema-diff = []

kernel/proto/expressions.proto

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,8 +150,17 @@ message ParseJsonExpression {
150150
delta.kernel.schema.StructType output_schema = 2;
151151
}
152152

153+
message MapToStructOptions {
154+
// Optional connector-supplied IANA timezone or normalized fixed offset (`+HH:MM`, `-HH:MM`,
155+
// `+HH:MM:SS`, or `-HH:MM:SS`) for offset-less TIMESTAMP values. Kernel never infers this from
156+
// the host or table metadata. A timezone or offset in a value takes precedence; absence means
157+
// UTC.
158+
optional string timestamp_timezone = 1;
159+
}
160+
153161
message MapToStructExpression {
154162
Expression map_expr = 1;
163+
MapToStructOptions options = 2;
155164
}
156165

157166
// `nullability_predicate` is optional: when set and it evaluates to false/null, the whole

kernel/src/checkpoint/checkpoint_transform.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
use std::sync::{Arc, LazyLock};
1717

1818
use crate::actions::{ADD_NAME, STATS_PARSED as STATS_PARSED_FIELD};
19-
use crate::expressions::{col, Expression, ExpressionRef, UnaryExpressionOp};
19+
use crate::expressions::{col, Expression, ExpressionRef, MapToStructOptions, UnaryExpressionOp};
2020
use crate::schema::{DataType, SchemaRef, SchemaStructPatchBuilder, StructField, StructType};
2121
use crate::struct_patch::ProjectionStructPatchBuilder;
2222
use crate::table_properties::TableProperties;
@@ -211,7 +211,11 @@ fn build_stats_parsed_expr(stats_schema: &SchemaRef) -> ExpressionRef {
211211
fn build_partition_values_parsed_expr() -> ExpressionRef {
212212
Arc::new(Expression::coalesce([
213213
col!(ADD_NAME, PARTITION_VALUES_PARSED_FIELD),
214-
Expression::map_to_struct(col!(ADD_NAME, PARTITION_VALUES_FIELD)),
214+
// Checkpoint construction has no reader timezone, so use UTC.
215+
Expression::map_to_struct(
216+
col!(ADD_NAME, PARTITION_VALUES_FIELD),
217+
MapToStructOptions::default(),
218+
),
215219
]))
216220
}
217221

0 commit comments

Comments
 (0)