Skip to content

Commit 92a3a33

Browse files
authored
fix: with_param_values on LogicalPlan::EmptyRelation returns incorrect schema (#18286)
## Which issue does this PR close? - Part of #18102. This only resolves for `LogicalPlan::EmptyRelation`. ## Rationale for this change `with_param_values` doesn't substitute params' type if it is used on `EmptyRelation`. Thus, causing `SELECT $1, $2` to have incorrect schema after substitution. For example: after replacing `$1 = 1, $2 = "s"`, the schema is `[Null, Null]`, but it should be `[Int64, Utf8]`. This schema type mismatch is resolved before converting to physical plan by the `type_coercion` rule in the `analyzer`. https://github.com/apache/datafusion/blob/814236093c4045bb3d972e4c1124727229743ed9/datafusion/optimizer/src/analyzer/type_coercion.rs#L149 ## What changes are included in this PR? - recompute the schema after replacing param values. ## Are these changes tested? Yes. ## Are there any user-facing changes? No.
1 parent aa74ed0 commit 92a3a33

File tree

3 files changed

+44
-18
lines changed

3 files changed

+44
-18
lines changed

datafusion/core/tests/dataframe/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5423,7 +5423,7 @@ async fn test_dataframe_placeholder_column_parameter() -> Result<()> {
54235423
assert_snapshot!(
54245424
actual,
54255425
@r"
5426-
Projection: Int32(3) AS $1 [$1:Null;N]
5426+
Projection: Int32(3) AS $1 [$1:Int32]
54275427
EmptyRelation: rows=1 []
54285428
"
54295429
);

datafusion/core/tests/sql/select.rs

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
use std::collections::HashMap;
1919

2020
use super::*;
21-
use datafusion::assert_batches_eq;
2221
use datafusion_common::{metadata::ScalarAndMetadata, ParamValues, ScalarValue};
2322
use insta::assert_snapshot;
2423

@@ -343,26 +342,20 @@ async fn test_query_parameters_with_metadata() -> Result<()> {
343342
]))
344343
.unwrap();
345344

346-
// df_with_params_replaced.schema() is not correct here
347-
// https://github.com/apache/datafusion/issues/18102
348-
let batches = df_with_params_replaced.clone().collect().await.unwrap();
349-
let schema = batches[0].schema();
350-
345+
let schema = df_with_params_replaced.schema();
351346
assert_eq!(schema.field(0).data_type(), &DataType::UInt32);
352347
assert_eq!(schema.field(0).metadata(), &metadata1);
353348
assert_eq!(schema.field(1).data_type(), &DataType::Utf8);
354349
assert_eq!(schema.field(1).metadata(), &metadata2);
355350

356-
assert_batches_eq!(
357-
[
358-
"+----+-----+",
359-
"| $1 | $2 |",
360-
"+----+-----+",
361-
"| 1 | two |",
362-
"+----+-----+",
363-
],
364-
&batches
365-
);
351+
let batches = df_with_params_replaced.collect().await.unwrap();
352+
assert_snapshot!(batches_to_sort_string(&batches), @r"
353+
+----+-----+
354+
| $1 | $2 |
355+
+----+-----+
356+
| 1 | two |
357+
+----+-----+
358+
");
366359

367360
Ok(())
368361
}

datafusion/expr/src/logical_plan/plan.rs

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1471,7 +1471,10 @@ impl LogicalPlan {
14711471
// Preserve name to avoid breaking column references to this expression
14721472
Ok(transformed_expr.update_data(|expr| original_name.restore(expr)))
14731473
}
1474-
})
1474+
})?
1475+
// always recompute the schema to ensure the changed in the schema's field should be
1476+
// poplulated to the plan's parent
1477+
.map_data(|plan| plan.recompute_schema())
14751478
})
14761479
.map(|res| res.data)
14771480
}
@@ -4248,6 +4251,7 @@ mod tests {
42484251
use super::*;
42494252
use crate::builder::LogicalTableSource;
42504253
use crate::logical_plan::table_scan;
4254+
use crate::select_expr::SelectExpr;
42514255
use crate::test::function_stub::{count, count_udaf};
42524256
use crate::{
42534257
binary_expr, col, exists, in_subquery, lit, placeholder, scalar_subquery,
@@ -4826,6 +4830,35 @@ mod tests {
48264830
.expect_err("prepared field metadata mismatch unexpectedly succeeded");
48274831
}
48284832

4833+
#[test]
4834+
fn test_replace_placeholder_empty_relation_valid_schema() {
4835+
// SELECT $1, $2;
4836+
let plan = LogicalPlanBuilder::empty(false)
4837+
.project(vec![
4838+
SelectExpr::from(placeholder("$1")),
4839+
SelectExpr::from(placeholder("$2")),
4840+
])
4841+
.unwrap()
4842+
.build()
4843+
.unwrap();
4844+
4845+
// original
4846+
assert_snapshot!(plan.display_indent_schema(), @r"
4847+
Projection: $1, $2 [$1:Null;N, $2:Null;N]
4848+
EmptyRelation: rows=0 []
4849+
");
4850+
4851+
let plan = plan
4852+
.with_param_values(vec![ScalarValue::from(1i32), ScalarValue::from("s")])
4853+
.unwrap();
4854+
4855+
// replaced
4856+
assert_snapshot!(plan.display_indent_schema(), @r#"
4857+
Projection: Int32(1) AS $1, Utf8("s") AS $2 [$1:Int32, $2:Utf8]
4858+
EmptyRelation: rows=0 []
4859+
"#);
4860+
}
4861+
48294862
#[test]
48304863
fn test_nullable_schema_after_grouping_set() {
48314864
let schema = Schema::new(vec![

0 commit comments

Comments
 (0)