Skip to content

Commit 8a7ca6e

Browse files
committed
feat!: add reader timezone to map-to-struct
1 parent 570861e commit 8a7ca6e

15 files changed

Lines changed: 813 additions & 82 deletions

File tree

datafusion-executor/src/expression.rs

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -407,15 +407,22 @@ fn struct_columns_from_patch(
407407
/// - Decimal: arrow's cast silently rescales/rounds to the target scale, while kernel requires the
408408
/// value's scale to match the target's exactly (and hard-errors otherwise).
409409
///
410+
/// Configured options are rejected rather than silently lowered with default semantics.
411+
///
410412
/// # 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.
413+
/// Returns an error when options are configured, `output_type` is absent, not a struct, or has a
414+
/// non-primitive field, or from lowering the map expression.
413415
fn map_to_struct_to_df_expr(
414416
map_to_struct: &MapToStructExpression,
415417
input_schema: &StructType,
416418
output_type: Option<&KernelDataType>,
417419
) -> DeltaResult<DFExpr> {
418420
let target = require_struct_output(output_type, "MapToStruct")?;
421+
if !map_to_struct.options.is_default() {
422+
return Err(Error::unsupported(
423+
"DataFusion execution of MapToStruct with configured options",
424+
));
425+
}
419426
let map = to_df_expr(&map_to_struct.map_expr, input_schema, None)?;
420427

421428
let mut args = Vec::with_capacity(target.num_fields() * 2);
@@ -547,7 +554,7 @@ mod tests {
547554
use datafusion::physical_expr::execution_props::ExecutionProps;
548555
use delta_kernel::expressions::{
549556
col, lit, null_lit, ColumnName as KernelColumnName, Expression as KernelExpr,
550-
ExpressionStructPatch, ExpressionStructPatchBuilder,
557+
ExpressionStructPatch, ExpressionStructPatchBuilder, MapToStructOptions,
551558
};
552559
use delta_kernel::schema::{schema, schema_ref, ArrayType, DataType, MapType, StructType};
553560
use rstest::rstest;
@@ -1001,7 +1008,7 @@ mod tests {
10011008
/// Lowers a `MapToStruct` over `pv` targeting `output_schema` and renders it as a `Display`
10021009
/// string.
10031010
fn lower_map_to_struct(output_schema: StructType) -> String {
1004-
let kernel = KernelExpr::map_to_struct(col!("pv"));
1011+
let kernel = KernelExpr::map_to_struct(col!("pv"), MapToStructOptions::default());
10051012
let target: DataType = output_schema.into();
10061013
to_df_expr(&kernel, &pv_map_schema(), Some(&target))
10071014
.unwrap()
@@ -1063,13 +1070,28 @@ mod tests {
10631070
#[case] output_type: Option<DataType>,
10641071
#[case] expected_message: &str,
10651072
) {
1066-
let kernel = KernelExpr::map_to_struct(col!("pv"));
1073+
let kernel = KernelExpr::map_to_struct(col!("pv"), MapToStructOptions::default());
10671074
let err = to_df_expr(&kernel, &pv_map_schema(), output_type.as_ref())
10681075
.unwrap_err()
10691076
.to_string();
10701077
assert!(err.contains(expected_message), "{err}");
10711078
}
10721079

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

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

ffi/src/expressions/engine_visitor.rs

Lines changed: 46 additions & 6 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 !map_to_struct.options.is_default() => {
700+
visit_unknown(visitor, sibling_list_id, "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);
@@ -771,7 +774,7 @@ fn visit_predicate_internal(predicate: &Predicate, visitor: &mut EngineExpressio
771774

772775
#[cfg(test)]
773776
mod tests {
774-
use delta_kernel::expressions::{lit, Expression, Scalar};
777+
use delta_kernel::expressions::{lit, Expression, MapToStructOptions, Scalar};
775778
use rstest::rstest;
776779

777780
use super::*;
@@ -791,6 +794,10 @@ mod tests {
791794
sibling_list_id: usize,
792795
parts: Vec<String>,
793796
},
797+
Unknown {
798+
sibling_list_id: usize,
799+
name: String,
800+
},
794801
}
795802

796803
#[derive(Default)]
@@ -847,6 +854,18 @@ mod tests {
847854
});
848855
}
849856

857+
extern "C" fn visit_unknown_name(
858+
data: *mut c_void,
859+
sibling_list_id: usize,
860+
name: KernelStringSlice,
861+
) {
862+
let builder = unsafe { &mut *(data as *mut TestExpressionBuilder) };
863+
let name = unsafe { String::try_from_slice(&name) }.unwrap();
864+
builder.events.push(LiteralEvent::Unknown {
865+
sibling_list_id,
866+
name,
867+
});
868+
}
850869
macro_rules! ignore_fn {
851870
($fn_name:ident $(, $arg_type:ty)*) => {
852871
extern "C" fn $fn_name(
@@ -925,7 +944,7 @@ mod tests {
925944
visit_field_patch: ignore_field_patch,
926945
visit_opaque_expr: ignore_opaque_expr,
927946
visit_opaque_pred: ignore_opaque_pred,
928-
visit_unknown: ignore_string_slice,
947+
visit_unknown: visit_unknown_name,
929948
}
930949
}
931950

@@ -992,4 +1011,25 @@ mod tests {
9921011
assert_eq!(top_level_id, 0);
9931012
assert_eq!(builder.events, vec![expected]);
9941013
}
1014+
1015+
#[test]
1016+
fn timezone_aware_map_to_struct_visits_unknown() {
1017+
let expression = Expression::map_to_struct(
1018+
Expression::column(["partitionValues"]),
1019+
MapToStructOptions::default().with_timestamp_timezone("America/Los_Angeles"),
1020+
);
1021+
let mut builder = TestExpressionBuilder::default();
1022+
let mut visitor = test_visitor(&mut builder);
1023+
1024+
let top_level_id = visit_expression_internal(&expression, &mut visitor);
1025+
1026+
assert_eq!(top_level_id, 0);
1027+
assert_eq!(
1028+
builder.events,
1029+
vec![LiteralEvent::Unknown {
1030+
sibling_list_id: 0,
1031+
name: "map_to_struct".to_string(),
1032+
}]
1033+
);
1034+
}
9951035
}

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/proto/expressions.proto

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,25 @@ 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.
157+
// A timezone or offset in a value takes precedence; absence means UTC. For named timezones,
158+
// repeated local times use the earlier occurrence. Local times skipped by a forward clock change
159+
// use the prior offset, shifting the result forward by the size of the gap.
160+
optional string timestamp_timezone = 1;
161+
}
162+
153163
message MapToStructExpression {
154164
Expression map_expr = 1;
155165
}
156166

167+
message ConfiguredMapToStructExpression {
168+
Expression map_expr = 1;
169+
MapToStructOptions options = 2;
170+
}
171+
157172
// `nullability_predicate` is optional: when set and it evaluates to false/null, the whole
158173
// struct is null.
159174
message StructExpression {
@@ -237,7 +252,10 @@ message Expression {
237252
IfExpression if_expr = 9;
238253
OpaqueExpression opaque = 10;
239254
ParseJsonExpression parse_json = 11;
255+
// This tag carries only default MapToStruct semantics.
240256
MapToStructExpression map_to_struct = 12;
241257
string unknown = 13;
258+
// Configured semantics use a distinct tag so executors without options support reject them.
259+
ConfiguredMapToStructExpression configured_map_to_struct = 14;
242260
}
243261
}

kernel/src/checkpoint/checkpoint_transform.rs

Lines changed: 7 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,12 @@ 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 caller-supplied timezone, so reconstruct with
215+
// deterministic UTC rather than guessing from the host environment.
216+
Expression::map_to_struct(
217+
col!(ADD_NAME, PARTITION_VALUES_FIELD),
218+
MapToStructOptions::default(),
219+
),
215220
]))
216221
}
217222

0 commit comments

Comments
 (0)