Skip to content

Commit 3b6e2fb

Browse files
authored
fix: deprecate data_type_and_nullable and simplify API usage (#18869)
## Which issue does this PR close? - Closes #18844 ## What changes are included in this PR? Instead of using `data_type_and_nullable`, this patch removes the indirection and calls `to_field` directly. The deprecated helper added no additional logic, so the PR encourages callers to use the field returned by to_field to access both the data type and the nullability. ## Are these changes tested? Yes
1 parent 0f1133e commit 3b6e2fb

File tree

4 files changed

+42
-29
lines changed

4 files changed

+42
-29
lines changed

datafusion/expr/src/expr_schema.rs

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ pub trait ExprSchemable {
5858
fn cast_to(self, cast_to_type: &DataType, schema: &dyn ExprSchema) -> Result<Expr>;
5959

6060
/// Given a schema, return the type and nullability of the expr
61+
#[deprecated(
62+
since = "51.0.0",
63+
note = "Use `to_field().1.is_nullable` and `to_field().1.data_type()` directly instead"
64+
)]
6165
fn data_type_and_nullable(&self, schema: &dyn ExprSchema)
6266
-> Result<(DataType, bool)>;
6367
}
@@ -150,7 +154,7 @@ impl ExprSchemable for Expr {
150154
}
151155
}
152156
Expr::ScalarFunction(_func) => {
153-
let (return_type, _) = self.data_type_and_nullable(schema)?;
157+
let return_type = self.to_field(schema)?.1.data_type().clone();
154158
Ok(return_type)
155159
}
156160
Expr::WindowFunction(window_function) => self
@@ -350,7 +354,9 @@ impl ExprSchemable for Expr {
350354
}
351355
Expr::Cast(Cast { expr, .. }) => expr.nullable(input_schema),
352356
Expr::ScalarFunction(_func) => {
353-
let (_, nullable) = self.data_type_and_nullable(input_schema)?;
357+
let field = self.to_field(input_schema)?.1;
358+
359+
let nullable = field.is_nullable();
354360
Ok(nullable)
355361
}
356362
Expr::AggregateFunction(AggregateFunction { func, .. }) => {
@@ -530,9 +536,14 @@ impl ExprSchemable for Expr {
530536
ref right,
531537
ref op,
532538
}) => {
533-
let (lhs_type, lhs_nullable) = left.data_type_and_nullable(schema)?;
534-
let (rhs_type, rhs_nullable) = right.data_type_and_nullable(schema)?;
535-
let mut coercer = BinaryTypeCoercer::new(&lhs_type, op, &rhs_type);
539+
let (left_field, right_field) =
540+
(left.to_field(schema)?.1, right.to_field(schema)?.1);
541+
542+
let (lhs_type, lhs_nullable) =
543+
(left_field.data_type(), left_field.is_nullable());
544+
let (rhs_type, rhs_nullable) =
545+
(right_field.data_type(), right_field.is_nullable());
546+
let mut coercer = BinaryTypeCoercer::new(lhs_type, op, rhs_type);
536547
coercer.set_lhs_spans(left.spans().cloned().unwrap_or_default());
537548
coercer.set_rhs_spans(right.spans().cloned().unwrap_or_default());
538549
Ok(Arc::new(Field::new(
@@ -1125,16 +1136,18 @@ mod tests {
11251136
),
11261137
));
11271138

1139+
let field = expr.to_field(&schema).unwrap().1;
11281140
assert_eq!(
1129-
expr.data_type_and_nullable(&schema).unwrap(),
1130-
(DataType::Utf8, true)
1141+
(field.data_type(), field.is_nullable()),
1142+
(&DataType::Utf8, true)
11311143
);
11321144
assert_eq!(placeholder_meta, expr.metadata(&schema).unwrap());
11331145

11341146
let expr_alias = expr.alias("a placeholder by any other name");
1147+
let expr_alias_field = expr_alias.to_field(&schema).unwrap().1;
11351148
assert_eq!(
1136-
expr_alias.data_type_and_nullable(&schema).unwrap(),
1137-
(DataType::Utf8, true)
1149+
(expr_alias_field.data_type(), expr_alias_field.is_nullable()),
1150+
(&DataType::Utf8, true)
11381151
);
11391152
assert_eq!(placeholder_meta, expr_alias.metadata(&schema).unwrap());
11401153

@@ -1143,14 +1156,17 @@ mod tests {
11431156
"".to_string(),
11441157
Some(Field::new("", DataType::Utf8, false).into()),
11451158
));
1159+
let expr_field = expr.to_field(&schema).unwrap().1;
11461160
assert_eq!(
1147-
expr.data_type_and_nullable(&schema).unwrap(),
1148-
(DataType::Utf8, false)
1161+
(expr_field.data_type(), expr_field.is_nullable()),
1162+
(&DataType::Utf8, false)
11491163
);
1164+
11501165
let expr_alias = expr.alias("a placeholder by any other name");
1166+
let expr_alias_field = expr_alias.to_field(&schema).unwrap().1;
11511167
assert_eq!(
1152-
expr_alias.data_type_and_nullable(&schema).unwrap(),
1153-
(DataType::Utf8, false)
1168+
(expr_alias_field.data_type(), expr_alias_field.is_nullable()),
1169+
(&DataType::Utf8, false)
11541170
);
11551171
}
11561172

datafusion/sql/src/expr/unary_op.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,11 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
3838
UnaryOperator::Plus => {
3939
let operand =
4040
self.sql_expr_to_logical_expr(expr, schema, planner_context)?;
41-
let (data_type, _) = operand.data_type_and_nullable(schema)?;
41+
let field = operand.to_field(schema)?.1;
42+
let data_type = field.data_type();
4243
if data_type.is_numeric()
43-
|| is_interval(&data_type)
44-
|| is_timestamp(&data_type)
44+
|| is_interval(data_type)
45+
|| is_timestamp(data_type)
4546
{
4647
Ok(operand)
4748
} else {

datafusion/sql/src/utils.rs

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -421,8 +421,8 @@ impl RecursiveUnnestRewriter<'_> {
421421
// This is due to the fact that unnest transformation should keep the original
422422
// column name as is, to comply with group by and order by
423423
let placeholder_column = Column::from_name(placeholder_name.clone());
424-
425-
let (data_type, _) = expr_in_unnest.data_type_and_nullable(self.input_schema)?;
424+
let field = expr_in_unnest.to_field(self.input_schema)?.1;
425+
let data_type = field.data_type();
426426

427427
match data_type {
428428
DataType::Struct(inner_fields) => {
@@ -436,12 +436,10 @@ impl RecursiveUnnestRewriter<'_> {
436436
);
437437
self.columns_unnestings
438438
.insert(Column::from_name(placeholder_name.clone()), None);
439-
Ok(
440-
get_struct_unnested_columns(&placeholder_name, &inner_fields)
441-
.into_iter()
442-
.map(Expr::Column)
443-
.collect(),
444-
)
439+
Ok(get_struct_unnested_columns(&placeholder_name, inner_fields)
440+
.into_iter()
441+
.map(Expr::Column)
442+
.collect())
445443
}
446444
DataType::List(_)
447445
| DataType::FixedSizeList(_, _)
@@ -482,8 +480,8 @@ impl TreeNodeRewriter for RecursiveUnnestRewriter<'_> {
482480
/// is used to detect if some recursive unnest expr exists (e.g **unnest(unnest(unnest(3d column))))**
483481
fn f_down(&mut self, expr: Expr) -> Result<Transformed<Expr>> {
484482
if let Expr::Unnest(ref unnest_expr) = expr {
485-
let (data_type, _) =
486-
unnest_expr.expr.data_type_and_nullable(self.input_schema)?;
483+
let field = unnest_expr.expr.to_field(self.input_schema)?.1;
484+
let data_type = field.data_type();
487485
self.consecutive_unnest.push(Some(unnest_expr.clone()));
488486
// if expr inside unnest is a struct, do not consider
489487
// the next unnest as consecutive unnest (if any)

datafusion/substrait/src/logical_plan/consumer/expr/mod.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,7 @@ pub async fn from_substrait_extended_expr(
143143
let expr = consumer
144144
.consume_expression(scalar_expr, &input_schema)
145145
.await?;
146-
let (output_type, expected_nullability) =
147-
expr.data_type_and_nullable(&input_schema)?;
148-
let output_field = Field::new("", output_type, expected_nullability);
146+
let output_field = expr.to_field(&input_schema)?.1;
149147
let mut names_idx = 0;
150148
let output_field = rename_field(
151149
&output_field,

0 commit comments

Comments
 (0)