From a03ba1f2416fe5aff4e7a42271e23ad27afcf136 Mon Sep 17 00:00:00 2001 From: "eliot.seol" Date: Thu, 13 Aug 2026 16:56:46 -0400 Subject: [PATCH 1/2] fully deprecated the acceptance of value field --- .../src/logical_plan/consumer/rel/read_rel.rs | 155 ++++++------------ 1 file changed, 52 insertions(+), 103 deletions(-) diff --git a/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs b/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs index 78951a3aff549..1da97e0c9a927 100644 --- a/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs +++ b/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs @@ -35,7 +35,6 @@ use substrait::proto::read_rel::local_files::file_or_files::PathType::UriFile; use substrait::proto::{Expression, ReadRel}; use url::Url; -#[expect(deprecated)] pub async fn from_read_rel( consumer: &impl SubstraitConsumer, read: &ReadRel, @@ -114,14 +113,14 @@ pub async fn from_read_rel( .await } Some(ReadType::VirtualTable(vt)) => { - if vt.values.is_empty() && vt.expressions.is_empty() { + if vt.expressions.is_empty() { return Ok(LogicalPlan::EmptyRelation(EmptyRelation { produce_one_row: false, schema: DFSchemaRef::new(substrait_schema), })); } - // Check for produce_one_row pattern in both old (values) and new (expressions) formats. + // Check for produce_one_row pattern in the new expressions format. // A VirtualTable with exactly one row containing only empty/default fields represents // an EmptyRelation with produce_one_row=true. This pattern is used for queries without // a FROM clause (e.g., "SELECT 1 AS one") where a single phantom row is needed to @@ -129,14 +128,9 @@ pub async fn from_read_rel( // the SQL "DUAL" table (see: https://en.wikipedia.org/wiki/DUAL_table) which some // databases provide as a single-row source for selecting constant expressions when no // real table is present. - let is_produce_one_row = (vt.values.len() == 1 - && vt.expressions.is_empty() + let is_produce_one_row = vt.expressions.len() == 1 && substrait_schema.fields().is_empty() - && vt.values[0].fields.is_empty()) - || (vt.expressions.len() == 1 - && vt.values.is_empty() - && substrait_schema.fields().is_empty() - && vt.expressions[0].fields.is_empty()); + && vt.expressions[0].fields.is_empty(); if is_produce_one_row { return Ok(LogicalPlan::EmptyRelation(EmptyRelation { @@ -145,63 +139,58 @@ pub async fn from_read_rel( })); } - let values = if !vt.expressions.is_empty() { - let mut exprs = vec![]; - for row in &vt.expressions { - if row.fields.len() != substrait_schema.fields().len() { - return substrait_err!( - "Field count mismatch: expected {} fields but found {} in virtual table row", - substrait_schema.fields().len(), - row.fields.len() - ); - } - - let mut row_exprs = vec![]; - let mut name_idx = 0; - for expression in &row.fields { - // Top-level names are provided through schema - // Each expression consumes at least one name, and Literals may consume additional names. - name_idx += 1; - let expr = match expression.rex_type.as_ref() { - Some(substrait::proto::expression::RexType::Literal(lit)) => { - // Values literals need 'named_struct.names' so nested struct fields keep their names from the ReadRel base schema. - // This is important for nested struct fields to retain their names. - Expr::Literal( - from_substrait_literal( - consumer, - lit, - &named_struct.names, - &mut name_idx, - )?, - None, - ) - } - _ => { - consumer - .consume_expression(expression, &substrait_schema) - .await? - } - }; - row_exprs.push(expr); - } - - if name_idx != named_struct.names.len() { - return substrait_err!( - "Names list must match exactly to nested schema, but found {} uses for {} names", - name_idx, - named_struct.names.len() - ); - } - exprs.push(row_exprs); + let mut rows = vec![]; + for row in &vt.expressions { + if row.fields.len() != substrait_schema.fields().len() { + return substrait_err!( + "Field count mismatch: expected {} fields but found {} in virtual table row", + substrait_schema.fields().len(), + row.fields.len() + ); } - exprs - } else { - convert_literal_rows(consumer, vt, named_struct)? - }; + + let mut row_exprs = vec![]; + let mut name_idx = 0; + for expression in &row.fields { + // Top-level names are provided through schema + // Each expression consumes at least one name, and Literals may consume additional names. + name_idx += 1; + let expr = match expression.rex_type.as_ref() { + Some(substrait::proto::expression::RexType::Literal(lit)) => { + // Literal expressions need 'named_struct.names' so nested struct fields + // keep their names from the ReadRel base schema. + Expr::Literal( + from_substrait_literal( + consumer, + lit, + &named_struct.names, + &mut name_idx, + )?, + None, + ) + } + _ => { + consumer + .consume_expression(expression, &substrait_schema) + .await? + } + }; + row_exprs.push(expr); + } + + if name_idx != named_struct.names.len() { + return substrait_err!( + "Names list must match exactly to nested schema, but found {} uses for {} names", + name_idx, + named_struct.names.len() + ); + } + rows.push(row_exprs); + } Ok(LogicalPlan::Values(Values { schema: DFSchemaRef::new(substrait_schema), - values, + values: rows, })) } Some(ReadType::LocalFiles(lf)) => { @@ -251,46 +240,6 @@ pub async fn from_read_rel( } } -/// Converts Substrait literal rows from a VirtualTable into DataFusion expressions. -/// -/// This function processes the deprecated `values` field of VirtualTable, converting -/// each literal value into a `Expr::Literal` while tracking and validating the name -/// indices against the provided named struct schema. -fn convert_literal_rows( - consumer: &impl SubstraitConsumer, - vt: &substrait::proto::read_rel::VirtualTable, - named_struct: &substrait::proto::NamedStruct, -) -> datafusion::common::Result>> { - #[expect(deprecated)] - vt.values - .iter() - .map(|row| { - let mut name_idx = 0; - let lits = row - .fields - .iter() - .map(|lit| { - name_idx += 1; // top-level names are provided through schema - Ok(Expr::Literal(from_substrait_literal( - consumer, - lit, - &named_struct.names, - &mut name_idx, - )?, None)) - }) - .collect::>()?; - if name_idx != named_struct.names.len() { - return substrait_err!( - "Names list must match exactly to nested schema, but found {} uses for {} names", - name_idx, - named_struct.names.len() - ); - } - Ok(lits) - }) - .collect::>() -} - pub fn apply_masking( schema: DFSchema, mask_expression: &::core::option::Option, From 8c9b893eafb249c4ea3f39882a367b22ef87b4eb Mon Sep 17 00:00:00 2001 From: eliot1480 <111935436+eliot1480@users.noreply.github.com> Date: Thu, 20 Aug 2026 12:25:05 -0400 Subject: [PATCH 2/2] Update datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs Co-authored-by: Victor Barua --- datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs b/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs index 1da97e0c9a927..e599b10dd44aa 100644 --- a/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs +++ b/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs @@ -120,7 +120,7 @@ pub async fn from_read_rel( })); } - // Check for produce_one_row pattern in the new expressions format. + // Check for produce_one_row pattern // A VirtualTable with exactly one row containing only empty/default fields represents // an EmptyRelation with produce_one_row=true. This pattern is used for queries without // a FROM clause (e.g., "SELECT 1 AS one") where a single phantom row is needed to