diff --git a/Cargo.lock b/Cargo.lock index 06c12a9e0757f..19058c7ca921a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1016,6 +1016,12 @@ dependencies = [ "vsimd", ] +[[package]] +name = "beef" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3a8241f3ebb85c056b509d4327ad0358fbbba6ffb340bf388f26350aeda225b1" + [[package]] name = "bigdecimal" version = "0.4.10" @@ -2610,7 +2616,7 @@ dependencies = [ "arrow", "datafusion-common", "doc-comment", - "pbjson 0.9.0", + "pbjson", "prost", "serde", ] @@ -2621,7 +2627,7 @@ version = "55.1.0" dependencies = [ "datafusion-common", "datafusion-proto-common", - "pbjson 0.9.0", + "pbjson", "prost", "serde", "serde_json", @@ -3256,7 +3262,7 @@ dependencies = [ name = "gen" version = "0.1.0" dependencies = [ - "pbjson-build 0.9.0", + "pbjson-build", "prost-build", ] @@ -3264,7 +3270,7 @@ dependencies = [ name = "gen-common" version = "0.1.0" dependencies = [ - "pbjson-build 0.9.0", + "pbjson-build", "prost-build", ] @@ -3782,6 +3788,26 @@ dependencies = [ "icu_properties", ] +[[package]] +name = "include_dir" +version = "0.7.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "923d117408f1e49d914f1a379a309cffe4f18c05cf4e3d12e613a15fc81bd0dd" +dependencies = [ + "glob", + "include_dir_macros", +] + +[[package]] +name = "include_dir_macros" +version = "0.7.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7cab85a7ed0bd5f0e76d93846e0147172bed2e2d3f859bcc33a8d9699cad1a75" +dependencies = [ + "proc-macro2", + "quote", +] + [[package]] name = "indexmap" version = "1.9.3" @@ -4186,6 +4212,72 @@ version = "0.4.34" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f9f8bd3e56ce4dfc153cf470fffbfa98c7620958b312ca5c3a4b8d5181fd13c6" +[[package]] +name = "logos" +version = "0.15.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ff472f899b4ec2d99161c51f60ff7075eeb3097069a36050d8037a6325eb8154" +dependencies = [ + "logos-derive 0.15.1", +] + +[[package]] +name = "logos" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb2c55a318a87600ea870ff8c2012148b44bf18b74fad48d0f835c38c7d07c5f" +dependencies = [ + "logos-derive 0.16.1", +] + +[[package]] +name = "logos-codegen" +version = "0.15.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "192a3a2b90b0c05b27a0b2c43eecdb7c415e29243acc3f89cc8247a5b693045c" +dependencies = [ + "beef", + "fnv", + "lazy_static", + "proc-macro2", + "quote", + "regex-syntax", + "rustc_version", + "syn 2.0.119", +] + +[[package]] +name = "logos-codegen" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "58b3ffaa284e1350d017a57d04ada118c4583cf260c8fb01e0fe28a2e9cf8970" +dependencies = [ + "fnv", + "proc-macro2", + "quote", + "regex-automata", + "regex-syntax", + "syn 2.0.119", +] + +[[package]] +name = "logos-derive" +version = "0.15.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "605d9697bcd5ef3a42d38efc51541aa3d6a4a25f7ab6d1ed0da5ac632a26b470" +dependencies = [ + "logos-codegen 0.15.1", +] + +[[package]] +name = "logos-derive" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52d3a9855747c17eaf4383823f135220716ab49bea5fbea7dd42cc9a92f8aa31" +dependencies = [ + "logos-codegen 0.16.1", +] + [[package]] name = "loom" version = "0.7.2" @@ -4255,6 +4347,28 @@ version = "2.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf8baf1c55e62ffcace7a9f06f4bd9cd3f0c4beb022d3b367256b91b87513d98" +[[package]] +name = "miette" +version = "7.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5f98efec8807c63c752b5bd61f862c165c115b0a35685bdcfd9238c7aeb592b7" +dependencies = [ + "cfg-if", + "miette-derive", + "unicode-width 0.1.14", +] + +[[package]] +name = "miette-derive" +version = "7.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "db5b29714e950dbb20d5e6f74f9dcec4edbcc1067bb7f8ed198c097b8c1a818b" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] + [[package]] name = "mimalloc" version = "0.1.52" @@ -4697,16 +4811,6 @@ dependencies = [ "syn 2.0.119", ] -[[package]] -name = "pbjson" -version = "0.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "898bac3fa00d0ba57a4e8289837e965baa2dee8c3749f3b11d45a64b4223d9c3" -dependencies = [ - "base64 0.22.1", - "serde", -] - [[package]] name = "pbjson" version = "0.9.0" @@ -4717,18 +4821,6 @@ dependencies = [ "serde", ] -[[package]] -name = "pbjson-build" -version = "0.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af22d08a625a2213a78dbb0ffa253318c5c79ce3133d32d296655a7bdfb02095" -dependencies = [ - "heck", - "itertools 0.14.0", - "prost", - "prost-types", -] - [[package]] name = "pbjson-build" version = "0.9.0" @@ -4743,14 +4835,14 @@ dependencies = [ [[package]] name = "pbjson-types" -version = "0.8.0" +version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e748e28374f10a330ee3bb9f29b828c0ac79831a32bab65015ad9b661ead526" +checksum = "a14e2757d877c0f607a82ce1b8560e224370f159d66c5d52eb55ea187ef0350e" dependencies = [ "bytes", "chrono", - "pbjson 0.8.0", - "pbjson-build 0.8.0", + "pbjson", + "pbjson-build", "prost", "prost-build", "serde", @@ -4985,6 +5077,16 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "prettyplease" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2bfe0f4c752e450fc2faf62654f1c134747922825d5b04ca717b8874f41a40c0" +dependencies = [ + "proc-macro2", + "syn 3.0.6", +] + [[package]] name = "proc-macro-crate" version = "3.5.0" @@ -5024,7 +5126,7 @@ dependencies = [ "log", "multimap", "petgraph", - "prettyplease", + "prettyplease 0.2.37", "prost", "prost-types", "regex", @@ -5045,6 +5147,18 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "prost-reflect" +version = "0.16.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "01b80ea363c31af2de2b92e3c07ed1156628f7838c4afb4df75ee78a37fedbd1" +dependencies = [ + "logos 0.16.1", + "miette", + "prost", + "prost-types", +] + [[package]] name = "prost-types" version = "0.14.4" @@ -5063,6 +5177,33 @@ dependencies = [ "cmake", ] +[[package]] +name = "protox" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4f25a07a73c6717f0b9bbbd685918f5df9815f7efba450b83d9c9dea41f0e3a1" +dependencies = [ + "bytes", + "miette", + "prost", + "prost-reflect", + "prost-types", + "protox-parse", + "thiserror", +] + +[[package]] +name = "protox-parse" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "072eee358134396a4643dff81cfff1c255c9fbd3fb296be14bdb6a26f9156366" +dependencies = [ + "logos 0.15.1", + "miette", + "prost-types", + "thiserror", +] + [[package]] name = "psm" version = "0.1.31" @@ -5382,9 +5523,9 @@ checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" [[package]] name = "regress" -version = "0.10.5" +version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2057b2325e68a893284d1538021ab90279adac1139957ca2a74426c6f118fb48" +checksum = "158a764437582235e3501f683b93a0a6f8d825d04a789dbe5ed30b8799b8908a" dependencies = [ "hashbrown 0.16.1", "memchr", @@ -6217,28 +6358,56 @@ dependencies = [ [[package]] name = "substrait" -version = "0.63.0" +version = "0.65.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47f793c23baf7aa21919e0c2253ac3a47a306ced93359f036629a2d959dfe284" +dependencies = [ + "indexmap 2.14.2", + "prost", + "semver", + "serde", + "serde_json", + "substrait-extensions", + "substrait-prost", + "toml", +] + +[[package]] +name = "substrait-extensions" +version = "0.102.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e620ff4d5c02fd6f7752931aa74b16a26af66a63022cc1ad412c77edbe0bab47" +checksum = "d33afcbcf85005f788199c2de7983caef24d858c6909391208f8aad2516db46d" dependencies = [ "heck", + "include_dir", "indexmap 2.14.2", - "pbjson 0.8.0", - "pbjson-build 0.8.0", + "prettyplease 0.3.0", + "regress", + "schemars 0.8.22", + "serde", + "serde_json", + "serde_yaml", + "syn 3.0.6", + "typify", + "walkdir", +] + +[[package]] +name = "substrait-prost" +version = "0.102.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee6b74c488901400ff424a98b3505a105dd041de1aa9c355dad9f7f6115b4521" +dependencies = [ + "pbjson", + "pbjson-build", "pbjson-types", - "prettyplease", "prost", "prost-build", "prost-types", "protobuf-src", - "regress", - "schemars 0.8.22", - "semver", + "protox", "serde", "serde_json", - "serde_yaml", - "syn 2.0.119", - "typify", "walkdir", ] @@ -6787,9 +6956,9 @@ checksum = "40ce102ab67701b8526c123c1bab5cbe42d7040ccfd0f64af1a385808d2f43de" [[package]] name = "typify" -version = "0.5.0" +version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6d5bcc6f62eb1fa8aa4098f39b29f93dcb914e17158b76c50360911257aa629" +checksum = "8cdc2e612ea322c6e232d46a0b34607c8eb28978fd6060ecfb139f2a50db8d5f" dependencies = [ "typify-impl", "typify-macro", @@ -6797,9 +6966,9 @@ dependencies = [ [[package]] name = "typify-impl" -version = "0.5.0" +version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a1eb359f7ffa4f9ebe947fa11a1b2da054564502968db5f317b7e37693cb2240" +checksum = "691591f49550c0d371bc441d019c30ce241d4116aee7d68df7a9840d6c70bf8f" dependencies = [ "heck", "log", @@ -6817,9 +6986,9 @@ dependencies = [ [[package]] name = "typify-macro" -version = "0.5.0" +version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "911c32f3c8514b048c1b228361bebb5e6d73aeec01696e8cc0e82e2ffef8ab7a" +checksum = "d41aea893c49cf95661389207b8af0c6254ff48b2c3ae1e4f3704777dbdfcf03" dependencies = [ "proc-macro2", "quote", @@ -7514,7 +7683,7 @@ dependencies = [ "anyhow", "heck", "indexmap 2.14.2", - "prettyplease", + "prettyplease 0.2.37", "syn 2.0.119", "wasm-metadata", "wit-bindgen-core", @@ -7528,7 +7697,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0c0f9bfd77e6a48eccf51359e3ae77140a7f50b1e2ebfe62422d8afdaffab17a" dependencies = [ "anyhow", - "prettyplease", + "prettyplease 0.2.37", "proc-macro2", "quote", "syn 2.0.119", diff --git a/datafusion/substrait/Cargo.toml b/datafusion/substrait/Cargo.toml index a0f203cec8db6..3958196af378f 100644 --- a/datafusion/substrait/Cargo.toml +++ b/datafusion/substrait/Cargo.toml @@ -42,9 +42,9 @@ half = { workspace = true } itertools = { workspace = true } object_store = { workspace = true } # We need to match the version in substrait, so we don't use the workspace version here -pbjson-types = { version = "0.8.0" } +pbjson-types = { version = "0.9.0" } prost = { workspace = true } -substrait = { version = "0.63.0", features = ["serde"] } +substrait = { version = "0.65.0", features = ["serde"] } url = { workspace = true } tokio = { workspace = true, features = ["fs"] } @@ -59,6 +59,7 @@ insta = { workspace = true } default = ["physical"] physical = ["datafusion/parquet"] protoc = ["substrait/protoc"] +protox = ["substrait/protox"] [package.metadata.docs.rs] # Use default features ("physical") for docs, plus "protoc". "protoc" is needed diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/field_reference.rs b/datafusion/substrait/src/logical_plan/consumer/expr/field_reference.rs index be084f360358a..993c9ca91e4b5 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/field_reference.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/field_reference.rs @@ -74,7 +74,19 @@ fn resolve_outer_reference( outer_ref: &substrait::proto::expression::field_reference::OuterReference, field_idx: usize, ) -> datafusion::common::Result { - let steps_out = outer_ref.steps_out as usize; + use substrait::proto::expression::field_reference::outer_reference::OuterReferenceType; + // `StepsOut` is deprecated in favour of `RelReference`, but a relation + // reference needs anchors that DataFusion does not assign. + #[expect(deprecated)] + let steps_out = match outer_ref.outer_reference_type { + Some(OuterReferenceType::StepsOut(steps_out)) => steps_out as usize, + Some(OuterReferenceType::RelReference(_)) => { + return not_impl_err!( + "OuterReference by relation reference is not supported" + ); + } + None => return substrait_err!("OuterReference without a reference type"), + }; let Some(outer_schema) = consumer.get_outer_schema(steps_out) else { return substrait_err!( "OuterReference with steps_out={steps_out} \ diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs index b0756ef060ecf..501e47f5a023b 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs @@ -27,9 +27,7 @@ use crate::variation_const::{ INTERVAL_DAY_TIME_TYPE_REF, INTERVAL_MONTH_DAY_NANO_TYPE_NAME, INTERVAL_MONTH_DAY_NANO_TYPE_REF, INTERVAL_YEAR_MONTH_TYPE_REF, LARGE_CONTAINER_TYPE_VARIATION_REF, TIME_32_TYPE_VARIATION_REF, - TIME_64_TYPE_VARIATION_REF, TIMESTAMP_MICRO_TYPE_VARIATION_REF, - TIMESTAMP_MILLI_TYPE_VARIATION_REF, TIMESTAMP_NANO_TYPE_VARIATION_REF, - TIMESTAMP_SECOND_TYPE_VARIATION_REF, UNSIGNED_INTEGER_TYPE_VARIATION_REF, + TIME_64_TYPE_VARIATION_REF, UNSIGNED_INTEGER_TYPE_VARIATION_REF, VIEW_CONTAINER_TYPE_VARIATION_REF, }; use datafusion::arrow::array::{AsArray, MapArray, new_empty_array}; @@ -48,7 +46,6 @@ use substrait::proto::expression::Literal; use substrait::proto::expression::literal::user_defined::{TypeAnchorType, Val}; use substrait::proto::expression::literal::{ IntervalCompound, IntervalDayToSecond, IntervalYearToMonth, LiteralType, - interval_day_to_second, }; pub async fn from_literal( @@ -104,28 +101,6 @@ pub(crate) fn from_substrait_literal( }, Some(LiteralType::Fp32(f)) => ScalarValue::Float32(Some(*f)), Some(LiteralType::Fp64(f)) => ScalarValue::Float64(Some(*f)), - #[expect(deprecated)] - Some(LiteralType::Timestamp(t)) => { - // Kept for backwards compatibility, new plans should use PrecisionTimestamp(Tz) instead - #[expect(deprecated)] - match lit.type_variation_reference { - TIMESTAMP_SECOND_TYPE_VARIATION_REF => { - ScalarValue::TimestampSecond(Some(*t), None) - } - TIMESTAMP_MILLI_TYPE_VARIATION_REF => { - ScalarValue::TimestampMillisecond(Some(*t), None) - } - TIMESTAMP_MICRO_TYPE_VARIATION_REF => { - ScalarValue::TimestampMicrosecond(Some(*t), None) - } - TIMESTAMP_NANO_TYPE_VARIATION_REF => { - ScalarValue::TimestampNanosecond(Some(*t), None) - } - others => { - return substrait_err!("Unknown type variation reference {others}"); - } - } - } Some(LiteralType::PrecisionTimestamp(pt)) => match pt.precision { 0 => ScalarValue::TimestampSecond(Some(pt.value), None), 3 => ScalarValue::TimestampMillisecond(Some(pt.value), None), @@ -381,29 +356,17 @@ pub(crate) fn from_substrait_literal( days, seconds, subseconds, - precision_mode, + precision, })) => { - use interval_day_to_second::PrecisionMode; // DF only supports millisecond precision, so for any more granular type we lose precision - let milliseconds = match precision_mode { - #[expect(deprecated)] - Some(PrecisionMode::Microseconds(ms)) => ms / 1000, - None => { - if *subseconds != 0 { - return substrait_err!( - "Cannot set subseconds field of IntervalDayToSecond without setting precision" - ); - } else { - 0_i32 - } - } - Some(PrecisionMode::Precision(0)) => *subseconds as i32 * 1000, - Some(PrecisionMode::Precision(3)) => *subseconds as i32, - Some(PrecisionMode::Precision(6)) => (subseconds / 1000) as i32, - Some(PrecisionMode::Precision(9)) => (subseconds / 1000 / 1000) as i32, - _ => { + let milliseconds = match precision { + 0 => *subseconds as i32 * 1000, + 3 => *subseconds as i32, + 6 => (subseconds / 1000) as i32, + 9 => (subseconds / 1000 / 1000) as i32, + p => { return not_impl_err!( - "Unsupported Substrait interval day to second precision mode: {precision_mode:?}" + "Unsupported Substrait interval day to second precision: {p}" ); } }; @@ -423,8 +386,7 @@ pub(crate) fn from_substrait_literal( days, seconds, subseconds, - precision_mode: - Some(interval_day_to_second::PrecisionMode::Precision(p)), + precision: p, }), ) => { if *p < 0 || *p > 9 { @@ -607,9 +569,7 @@ mod tests { days: 3, seconds: 4, subseconds: 5, - precision_mode: Some( - interval_day_to_second::PrecisionMode::Precision(6), - ), + precision: 6, }), })), }; diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/mod.rs b/datafusion/substrait/src/logical_plan/consumer/expr/mod.rs index 623bb715cd838..5b8ca8396f114 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/mod.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/mod.rs @@ -62,6 +62,9 @@ pub async fn from_substrait_rex( ) -> datafusion::common::Result { match &expression.rex_type { Some(t) => match t { + RexType::ExecutionContextVariable(_) => { + not_impl_err!("Execution context variables are not supported") + } RexType::Literal(expr) => consumer.consume_literal(expr).await, RexType::Selection(expr) => { consumer.consume_field_reference(expr, input_schema).await @@ -92,8 +95,6 @@ pub async fn from_substrait_rex( consumer.consume_subquery(expr.as_ref(), input_schema).await } RexType::Nested(expr) => consumer.consume_nested(expr, input_schema).await, - #[expect(deprecated)] - RexType::Enum(expr) => consumer.consume_enum(expr, input_schema).await, RexType::DynamicParameter(expr) => { consumer.consume_dynamic_parameter(expr, input_schema).await } @@ -216,14 +217,14 @@ mod tests { async fn window_function_with_range_unit_and_no_order_by() -> datafusion::common::Result<()> { let substrait = Expression { - rex_type: Some(RexType::WindowFunction( + rex_type: Some(RexType::WindowFunction(Box::new( substrait::proto::expression::WindowFunction { function_reference: 0, bounds_type: BoundsType::Range as i32, sorts: vec![], ..Default::default() }, - )), + ))), }; let mut consumer = test_consumer(); @@ -247,12 +248,12 @@ mod tests { #[tokio::test] async fn window_function_with_count() -> datafusion::common::Result<()> { let substrait = Expression { - rex_type: Some(RexType::WindowFunction( + rex_type: Some(RexType::WindowFunction(Box::new( substrait::proto::expression::WindowFunction { function_reference: 0, ..Default::default() }, - )), + ))), }; let mut consumer = test_consumer(); @@ -274,13 +275,13 @@ mod tests { #[tokio::test] async fn window_function_with_invalid_invocation() { let substrait = Expression { - rex_type: Some(RexType::WindowFunction( + rex_type: Some(RexType::WindowFunction(Box::new( substrait::proto::expression::WindowFunction { function_reference: 0, invocation: 3, ..Default::default() }, - )), + ))), }; let mut consumer = test_consumer(); diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/window_function.rs b/datafusion/substrait/src/logical_plan/consumer/expr/window_function.rs index d39b325a54827..b0cbcbf7c4128 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/window_function.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/window_function.rs @@ -27,11 +27,14 @@ use datafusion::logical_expr::expr::WindowFunctionParams; use datafusion::logical_expr::{ Expr, WindowFrameBound, WindowFrameUnits, WindowFunctionDefinition, expr, }; +use substrait::proto::Expression; use substrait::proto::aggregate_function::AggregationInvocation; use substrait::proto::expression::WindowFunction; +use substrait::proto::expression::literal::LiteralType; use substrait::proto::expression::window_function::{Bound, BoundsType}; use substrait::proto::expression::{ - window_function::bound as SubstraitBound, window_function::bound::Kind as BoundKind, + Literal, RexType, window_function::bound as SubstraitBound, + window_function::bound::Kind as BoundKind, }; pub async fn from_window_function( @@ -85,8 +88,8 @@ pub async fn from_window_function( }; let window_frame = datafusion::logical_expr::WindowFrame::new_bounds( bound_units, - from_substrait_bound(window.lower_bound.as_ref(), true)?, - from_substrait_bound(window.upper_bound.as_ref(), false)?, + from_substrait_bound(window.lower_bound.as_deref(), true)?, + from_substrait_bound(window.upper_bound.as_deref(), false)?, ); window_frame.regularize_order_bys(&mut order_by)?; @@ -139,20 +142,32 @@ fn from_substrait_bound( BoundKind::CurrentRow(SubstraitBound::CurrentRow {}) => { Ok(WindowFrameBound::CurrentRow) } - BoundKind::Preceding(SubstraitBound::Preceding { offset }) => { - if *offset <= 0 { + BoundKind::Preceding(bound) => { + #[expect(deprecated)] + let offset = + bound_offset(bound.offset, bound.offset_expr.as_deref())?; + let Some(offset) = offset else { + return Ok(WindowFrameBound::CurrentRow); + }; + if offset <= 0 { return plan_err!("Preceding bound must be positive"); } Ok(WindowFrameBound::Preceding(ScalarValue::UInt64(Some( - *offset as u64, + offset as u64, )))) } - BoundKind::Following(SubstraitBound::Following { offset }) => { - if *offset <= 0 { + BoundKind::Following(bound) => { + #[expect(deprecated)] + let offset = + bound_offset(bound.offset, bound.offset_expr.as_deref())?; + let Some(offset) = offset else { + return Ok(WindowFrameBound::CurrentRow); + }; + if offset <= 0 { return plan_err!("Following bound must be positive"); } Ok(WindowFrameBound::Following(ScalarValue::UInt64(Some( - *offset as u64, + offset as u64, )))) } BoundKind::Unbounded(SubstraitBound::Unbounded {}) => { @@ -174,3 +189,76 @@ fn from_substrait_bound( } } } + +/// Reads the distance of a window frame bound. +/// +/// The specification requires a consumer to use `offset_expr` when it is set and +/// to ignore `offset`, and defines a zero `offset_expr` as equivalent to +/// CurrentRow, which `None` reports here. DataFusion frame bounds hold a +/// literal, so an expression that is not an int64 literal cannot be +/// represented. +fn bound_offset( + offset: i64, + offset_expr: Option<&Expression>, +) -> datafusion::common::Result> { + match offset_expr { + Some(Expression { + rex_type: + Some(RexType::Literal(Literal { + literal_type: Some(LiteralType::I64(value)), + .. + })), + }) => Ok((*value != 0).then_some(*value)), + Some(_) => not_impl_err!( + "Window frame bound offsets other than int64 literals are not supported" + ), + None => Ok(Some(offset)), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn i64_literal(value: i64) -> Expression { + Expression { + rex_type: Some(RexType::Literal(Literal { + literal_type: Some(LiteralType::I64(value)), + ..Default::default() + })), + } + } + + /// A zero `offset_expr` is defined as equivalent to CurrentRow, so it is + /// read as such rather than rejected as a non-positive distance. + #[test] + fn zero_offset_expression_reads_as_current_row() { + #[expect(deprecated)] + let bound = Bound { + kind: Some(BoundKind::Preceding(Box::new(SubstraitBound::Preceding { + offset: 0, + offset_expr: Some(Box::new(i64_literal(0))), + }))), + }; + assert_eq!( + from_substrait_bound(Some(&bound), true).unwrap(), + WindowFrameBound::CurrentRow + ); + } + + /// When `offset_expr` is set the consumer must use it and ignore `offset`. + #[test] + fn offset_expression_wins_over_the_deprecated_offset() { + #[expect(deprecated)] + let bound = Bound { + kind: Some(BoundKind::Preceding(Box::new(SubstraitBound::Preceding { + offset: 7, + offset_expr: Some(Box::new(i64_literal(3))), + }))), + }; + assert_eq!( + from_substrait_bound(Some(&bound), true).unwrap(), + WindowFrameBound::Preceding(ScalarValue::UInt64(Some(3))) + ); + } +} diff --git a/datafusion/substrait/src/logical_plan/consumer/rel/aggregate_rel.rs b/datafusion/substrait/src/logical_plan/consumer/rel/aggregate_rel.rs index 982a87d6d5e83..fcf122e5ef2b4 100644 --- a/datafusion/substrait/src/logical_plan/consumer/rel/aggregate_rel.rs +++ b/datafusion/substrait/src/logical_plan/consumer/rel/aggregate_rel.rs @@ -17,7 +17,7 @@ use crate::logical_plan::consumer::{NameTracker, SubstraitConsumer}; use crate::logical_plan::consumer::{from_substrait_agg_func, from_substrait_sorts}; -use datafusion::common::{Column, DFSchemaRef, internal_err, not_impl_err}; +use datafusion::common::{Column, internal_err, not_impl_err, substrait_datafusion_err}; use datafusion::logical_expr::builder::project; use datafusion::logical_expr::{ Aggregate, Expr, GroupingSet, LogicalPlan, LogicalPlanBuilder, @@ -45,26 +45,16 @@ pub async fn from_aggregate_rel( match agg.groupings.len() { 0 => {} 1 => { - group_exprs.extend_from_slice( - &from_substrait_grouping( - consumer, - &agg.groupings[0], - &ref_group_exprs, - input.schema(), - ) - .await?, - ); + group_exprs.extend_from_slice(&from_substrait_grouping( + &agg.groupings[0], + &ref_group_exprs, + )?); } _ => { let mut grouping_sets = vec![]; for grouping in &agg.groupings { - let grouping_set = from_substrait_grouping( - consumer, - grouping, - &ref_group_exprs, - input.schema(), - ) - .await?; + let grouping_set = + from_substrait_grouping(grouping, &ref_group_exprs)?; grouping_sets.push(grouping_set); } // Single-element grouping expression of type Expr::GroupingSet. @@ -170,24 +160,22 @@ fn reorder_grouping_set_output( project(plan, exprs) } -#[expect(deprecated)] -async fn from_substrait_grouping( - consumer: &impl SubstraitConsumer, +/// A grouping set names the expressions it groups by index into the +/// relation-level `grouping_expressions`. +fn from_substrait_grouping( grouping: &Grouping, expressions: &[Expr], - input_schema: &DFSchemaRef, ) -> datafusion::common::Result> { - let mut group_exprs = vec![]; - if !grouping.grouping_expressions.is_empty() { - for e in &grouping.grouping_expressions { - let expr = consumer.consume_expression(e, input_schema).await?; - group_exprs.push(expr); - } - return Ok(group_exprs); - } - for idx in &grouping.expression_references { - let e = &expressions[*idx as usize]; - group_exprs.push(e.clone()); - } - Ok(group_exprs) + grouping + .expression_references + .iter() + .map(|idx| { + expressions.get(*idx as usize).cloned().ok_or_else(|| { + substrait_datafusion_err!( + "Grouping references expression {idx} but the aggregate declares {}", + expressions.len() + ) + }) + }) + .collect() } diff --git a/datafusion/substrait/src/logical_plan/consumer/rel/fetch_rel.rs b/datafusion/substrait/src/logical_plan/consumer/rel/fetch_rel.rs index 12a8a77199b1a..6f96d5b9d307f 100644 --- a/datafusion/substrait/src/logical_plan/consumer/rel/fetch_rel.rs +++ b/datafusion/substrait/src/logical_plan/consumer/rel/fetch_rel.rs @@ -18,8 +18,8 @@ use crate::logical_plan::consumer::SubstraitConsumer; use async_recursion::async_recursion; use datafusion::common::{DFSchema, DFSchemaRef, not_impl_err}; -use datafusion::logical_expr::{LogicalPlan, LogicalPlanBuilder, lit}; -use substrait::proto::{FetchRel, fetch_rel}; +use datafusion::logical_expr::{LogicalPlan, LogicalPlanBuilder}; +use substrait::proto::FetchRel; #[async_recursion] pub async fn from_fetch_rel( @@ -29,23 +29,14 @@ pub async fn from_fetch_rel( if let Some(input) = fetch.input.as_ref() { let input = LogicalPlanBuilder::from(consumer.consume_rel(input).await?); let empty_schema = DFSchemaRef::new(DFSchema::empty()); - let offset = match &fetch.offset_mode { - #[expect(deprecated)] - Some(fetch_rel::OffsetMode::Offset(offset)) => Some(lit(*offset)), - Some(fetch_rel::OffsetMode::OffsetExpr(expr)) => { - Some(consumer.consume_expression(expr, &empty_schema).await?) - } + // Unset offset is treated as 0 and unset count signals that ALL records + // should be returned, so an absent expression maps to None either way. + let offset = match &fetch.offset_expr { + Some(expr) => Some(consumer.consume_expression(expr, &empty_schema).await?), None => None, }; - let count = match &fetch.count_mode { - #[expect(deprecated)] - Some(fetch_rel::CountMode::Count(count)) => { - // -1 means that ALL records should be returned, equivalent to None - (*count != -1).then(|| lit(*count)) - } - Some(fetch_rel::CountMode::CountExpr(expr)) => { - Some(consumer.consume_expression(expr, &empty_schema).await?) - } + let count = match &fetch.count_expr { + Some(expr) => Some(consumer.consume_expression(expr, &empty_schema).await?), None => None, }; input.limit_by_expr(offset, count)?.build() diff --git a/datafusion/substrait/src/logical_plan/consumer/rel/mod.rs b/datafusion/substrait/src/logical_plan/consumer/rel/mod.rs index 038ada115b9d8..ec82fbc642d92 100644 --- a/datafusion/substrait/src/logical_plan/consumer/rel/mod.rs +++ b/datafusion/substrait/src/logical_plan/consumer/rel/mod.rs @@ -134,6 +134,8 @@ fn retrieve_rel_common(rel: &Rel) -> Option<&RelCommon> { match rel.rel_type.as_ref() { None => None, Some(rt) => match rt { + RelType::LateralJoin(l) => l.common.as_ref(), + RelType::TopN(t) => t.common.as_ref(), RelType::Read(r) => r.common.as_ref(), RelType::Filter(f) => f.common.as_ref(), RelType::Fetch(f) => f.common.as_ref(), @@ -155,7 +157,7 @@ fn retrieve_rel_common(rel: &Rel) -> Option<&RelCommon> { RelType::Window(w) => w.common.as_ref(), RelType::Exchange(e) => e.common.as_ref(), RelType::Expand(e) => e.common.as_ref(), - RelType::Update(_) => None, + RelType::Update(u) => u.common.as_ref(), }, } } diff --git a/datafusion/substrait/src/logical_plan/consumer/substrait_consumer.rs b/datafusion/substrait/src/logical_plan/consumer/substrait_consumer.rs index 36ef39561e7c3..e3e2d3c9db0e2 100644 --- a/datafusion/substrait/src/logical_plan/consumer/substrait_consumer.rs +++ b/datafusion/substrait/src/logical_plan/consumer/substrait_consumer.rs @@ -40,8 +40,8 @@ use std::collections::{HashMap, VecDeque}; use std::sync::{Arc, RwLock}; use substrait::proto::expression as substrait_expression; use substrait::proto::expression::{ - Enum, FieldReference, IfThen, Literal, MultiOrList, Nested, ScalarFunction, - SingularOrList, SwitchExpression, WindowFunction, + FieldReference, IfThen, Literal, MultiOrList, Nested, ScalarFunction, SingularOrList, + SwitchExpression, WindowFunction, }; use substrait::proto::{self, Type}; use substrait::proto::{ @@ -388,14 +388,6 @@ pub trait SubstraitConsumer: Send + Sync + Sized { from_nested(self, expr, input_schema).await } - async fn consume_enum( - &self, - _expr: &Enum, - _input_schema: &DFSchema, - ) -> datafusion::common::Result { - not_impl_err!("Enum expression not supported") - } - async fn consume_dynamic_parameter( &self, expr: &DynamicParameter, diff --git a/datafusion/substrait/src/logical_plan/consumer/types.rs b/datafusion/substrait/src/logical_plan/consumer/types.rs index e588734782741..6f9f61f2e4cec 100644 --- a/datafusion/substrait/src/logical_plan/consumer/types.rs +++ b/datafusion/substrait/src/logical_plan/consumer/types.rs @@ -27,14 +27,12 @@ use crate::variation_const::{ INTERVAL_DAY_TIME_TYPE_REF, INTERVAL_MONTH_DAY_NANO_TYPE_NAME, INTERVAL_MONTH_DAY_NANO_TYPE_REF, INTERVAL_YEAR_MONTH_TYPE_REF, LARGE_CONTAINER_TYPE_VARIATION_REF, TIME_32_TYPE_VARIATION_REF, - TIME_64_TYPE_VARIATION_REF, TIMESTAMP_MICRO_TYPE_VARIATION_REF, - TIMESTAMP_MILLI_TYPE_VARIATION_REF, TIMESTAMP_NANO_TYPE_VARIATION_REF, - TIMESTAMP_SECOND_TYPE_VARIATION_REF, UNSIGNED_INTEGER_TYPE_VARIATION_REF, + TIME_64_TYPE_VARIATION_REF, UNSIGNED_INTEGER_TYPE_VARIATION_REF, VIEW_CONTAINER_TYPE_VARIATION_REF, }; use crate::variation_const::{FLOAT_16_TYPE_NAME, NULL_TYPE_NAME}; use datafusion::arrow::datatypes::{ - DataType, Field, FieldRef, Fields, IntervalUnit, Schema, TimeUnit, + DataType, Field, FieldRef, Fields, IntervalUnit, Schema, }; use datafusion::common::{ DFSchema, not_impl_err, substrait_datafusion_err, substrait_err, @@ -131,28 +129,6 @@ pub fn from_substrait_type( }, r#type::Kind::Fp32(_) => Ok(DataType::Float32), r#type::Kind::Fp64(_) => Ok(DataType::Float64), - #[expect(deprecated)] - r#type::Kind::Timestamp(ts) => { - // Kept for backwards compatibility, new plans should use PrecisionTimestamp(Tz) instead - #[expect(deprecated)] - match ts.type_variation_reference { - TIMESTAMP_SECOND_TYPE_VARIATION_REF => { - Ok(DataType::Timestamp(TimeUnit::Second, None)) - } - TIMESTAMP_MILLI_TYPE_VARIATION_REF => { - Ok(DataType::Timestamp(TimeUnit::Millisecond, None)) - } - TIMESTAMP_MICRO_TYPE_VARIATION_REF => { - Ok(DataType::Timestamp(TimeUnit::Microsecond, None)) - } - TIMESTAMP_NANO_TYPE_VARIATION_REF => { - Ok(DataType::Timestamp(TimeUnit::Nanosecond, None)) - } - v => not_impl_err!( - "Unsupported Substrait type variation {v} of type {s_kind:?}" - ), - } - } r#type::Kind::PrecisionTimestamp(pts) => { let unit = from_substrait_precision(pts.precision, "PrecisionTimestamp")?; Ok(DataType::Timestamp(unit, None)) @@ -398,13 +374,7 @@ fn type_is_nullable(dt: &Type) -> datafusion::common::Result { r#type::Kind::I64(integer) => integer.nullability, r#type::Kind::Fp32(float) => float.nullability, r#type::Kind::Fp64(float) => float.nullability, - #[expect(deprecated)] - r#type::Kind::Timestamp(timestamp) => timestamp.nullability, r#type::Kind::Date(date) => date.nullability, - #[expect(deprecated)] - r#type::Kind::Time(time) => time.nullability, - #[expect(deprecated)] - r#type::Kind::TimestampTz(timestamp) => timestamp.nullability, r#type::Kind::IntervalYear(interval) => interval.nullability, r#type::Kind::IntervalDay(interval) => interval.nullability, r#type::Kind::IntervalCompound(interval) => interval.nullability, @@ -423,8 +393,7 @@ fn type_is_nullable(dt: &Type) -> datafusion::common::Result { r#type::Kind::Map(map) => map.nullability, r#type::Kind::Func(func) => func.nullability, r#type::Kind::UserDefined(user_defined) => user_defined.nullability, - #[expect(deprecated)] - r#type::Kind::UserDefinedTypeReference(_) => r#type::Nullability::Required as i32, + r#type::Kind::Unbound(_) => r#type::Nullability::Nullable as i32, r#type::Kind::Alias(alias) => alias.nullability, }; @@ -652,17 +621,6 @@ mod tests { assert!(fields[0].metadata().is_empty()); } - #[test] - fn type_is_nullable_user_defined_type_reference_is_required() { - // The deprecated `UserDefinedTypeReference` variant doesn't carry a - // nullability field; the consumer hardcodes Required (non-null). - #[expect(deprecated)] - let dt = Type { - kind: Some(Kind::UserDefinedTypeReference(0)), - }; - assert!(!type_is_nullable(&dt).unwrap()); - } - #[test] fn type_is_nullable_missing_kind_defaults_to_nullable() { // Defensive: a Type whose kind is None is treated as nullable. diff --git a/datafusion/substrait/src/logical_plan/producer/expr/aggregate_function.rs b/datafusion/substrait/src/logical_plan/producer/expr/aggregate_function.rs index d2e495c31df80..829072a7bf7fb 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/aggregate_function.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/aggregate_function.rs @@ -56,7 +56,6 @@ pub fn from_aggregate_function( let function_anchor = producer.register_function(func.name().to_string()); let (_, output_field) = Expr::AggregateFunction(agg_fn.clone()).to_field(schema)?; let output_type = to_substrait_type_from_field(producer, &output_field)?; - #[expect(deprecated)] Ok(Measure { measure: Some(AggregateFunction { function_reference: function_anchor, @@ -68,7 +67,6 @@ pub fn from_aggregate_function( false => AggregationInvocation::All as i32, }, phase: AggregationPhase::Unspecified as i32, - args: vec![], options: vec![], }), filter: match filter { diff --git a/datafusion/substrait/src/logical_plan/producer/expr/field_reference.rs b/datafusion/substrait/src/logical_plan/producer/expr/field_reference.rs index 12ed92c64fe57..d12d825b9c206 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/field_reference.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/field_reference.rs @@ -19,6 +19,7 @@ use crate::logical_plan::producer::SubstraitProducer; use datafusion::common::{Column, DFSchemaRef, substrait_err}; use datafusion::logical_expr::Expr; use substrait::proto::Expression; +use substrait::proto::expression::field_reference::outer_reference::OuterReferenceType; use substrait::proto::expression::field_reference::{ OuterReference, ReferenceType, RootReference, RootType, }; @@ -101,8 +102,11 @@ pub fn from_outer_reference_column( if let Some(index) = outer_schema.maybe_index_of_column(col) { return substrait_field_ref_with_root( index, + #[expect(deprecated)] RootType::OuterReference(OuterReference { - steps_out: steps_out as u32, + outer_reference_type: Some(OuterReferenceType::StepsOut( + steps_out as u32, + )), }), ); } diff --git a/datafusion/substrait/src/logical_plan/producer/expr/literal.rs b/datafusion/substrait/src/logical_plan/producer/expr/literal.rs index f26984ad4a394..2f808146778b1 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/literal.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/literal.rs @@ -26,7 +26,6 @@ use crate::variation_const::{ use datafusion::arrow::array::{Array, GenericListArray, OffsetSizeTrait}; use datafusion::arrow::temporal_conversions::NANOSECONDS; use datafusion::common::{ScalarValue, exec_err, not_impl_err}; -use substrait::proto::expression::literal::interval_day_to_second::PrecisionMode; use substrait::proto::expression::literal::map::KeyValue; use substrait::proto::expression::literal::{ Decimal, IntervalCompound, IntervalDayToSecond, IntervalYearToMonth, List, @@ -217,7 +216,7 @@ pub(crate) fn to_substrait_literal( days: i.days, seconds: (i.nanoseconds / NANOSECONDS) as i32, subseconds: i.nanoseconds % NANOSECONDS, - precision_mode: Some(PrecisionMode::Precision(9)), // nanoseconds + precision: 9, // nanoseconds }), }), DEFAULT_TYPE_VARIATION_REF, @@ -227,7 +226,7 @@ pub(crate) fn to_substrait_literal( days: i.days, seconds: i.milliseconds / 1000, subseconds: (i.milliseconds % 1000) as i64, - precision_mode: Some(PrecisionMode::Precision(3)), // 3 for milliseconds + precision: 3, // 3 for milliseconds }), DEFAULT_TYPE_VARIATION_REF, ), diff --git a/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs b/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs index 75720395aae7c..5736d1cc2b849 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs @@ -121,14 +121,12 @@ pub fn from_higher_order_function( output_field.is_nullable(), )?; - #[expect(deprecated)] Ok(Expression { rex_type: Some(RexType::ScalarFunction(ScalarFunction { function_reference: function_anchor, arguments, output_type: Some(output_type), options: vec![], - args: vec![], })), }) } @@ -152,14 +150,12 @@ fn from_function( let output_type = to_substrait_type(producer, output_type, output_nullability)?; let function_anchor = producer.register_function(name.to_string()); - #[expect(deprecated)] Ok(Expression { rex_type: Some(RexType::ScalarFunction(ScalarFunction { function_reference: function_anchor, arguments, output_type: Some(output_type), options: vec![], - args: vec![], })), }) } @@ -288,13 +284,11 @@ fn make_substrait_like_expr( }, ]; - #[expect(deprecated)] let substrait_like = Expression { rex_type: Some(RexType::ScalarFunction(ScalarFunction { function_reference: function_anchor, arguments, output_type: None, - args: vec![], options: vec![], })), }; @@ -302,7 +296,6 @@ fn make_substrait_like_expr( if negated { let function_anchor = producer.register_function("not".to_string()); - #[expect(deprecated)] Ok(Expression { rex_type: Some(RexType::ScalarFunction(ScalarFunction { function_reference: function_anchor, @@ -310,7 +303,6 @@ fn make_substrait_like_expr( arg_type: Some(ArgType::Value(substrait_like)), }], output_type: None, - args: vec![], options: vec![], })), }) @@ -338,7 +330,6 @@ fn to_substrait_unary_scalar_fn( }], output_type: Some(output_type.clone()), options: vec![], - ..Default::default() })), }) } @@ -352,7 +343,6 @@ pub fn make_binary_op_scalar_func( output_type: &Type, ) -> Expression { let function_anchor = producer.register_function(operator_to_name(op).to_string()); - #[expect(deprecated)] Expression { rex_type: Some(RexType::ScalarFunction(ScalarFunction { function_reference: function_anchor, @@ -365,7 +355,6 @@ pub fn make_binary_op_scalar_func( }, ], output_type: Some(output_type.clone()), - args: vec![], options: vec![], })), } diff --git a/datafusion/substrait/src/logical_plan/producer/expr/window_function.rs b/datafusion/substrait/src/logical_plan/producer/expr/window_function.rs index d35771bf099d3..4b3121e777501 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/window_function.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/window_function.rs @@ -17,15 +17,17 @@ use crate::logical_plan::producer::SubstraitProducer; use crate::logical_plan::producer::utils::substrait_sort_field; +use crate::variation_const::DEFAULT_TYPE_VARIATION_REF; use datafusion::common::{DFSchemaRef, ScalarValue, not_impl_err}; use datafusion::logical_expr::expr::{WindowFunction, WindowFunctionParams}; use datafusion::logical_expr::{WindowFrame, WindowFrameBound, WindowFrameUnits}; use substrait::proto::aggregate_function::AggregationInvocation; -use substrait::proto::expression::RexType; use substrait::proto::expression::WindowFunction as SubstraitWindowFunction; +use substrait::proto::expression::literal::LiteralType; use substrait::proto::expression::window_function::bound as SubstraitBound; use substrait::proto::expression::window_function::bound::Kind as BoundKind; use substrait::proto::expression::window_function::{Bound, BoundsType}; +use substrait::proto::expression::{Literal, RexType}; use substrait::proto::function_argument::ArgType; use substrait::proto::{Expression, FunctionArgument, SortField}; @@ -99,9 +101,8 @@ fn make_substrait_window_function( bounds_type: BoundsType, distinct: bool, ) -> Expression { - #[expect(deprecated)] Expression { - rex_type: Some(RexType::WindowFunction(SubstraitWindowFunction { + rex_type: Some(RexType::WindowFunction(Box::new(SubstraitWindowFunction { function_reference, arguments, partitions, @@ -114,11 +115,10 @@ fn make_substrait_window_function( } else { AggregationInvocation::All as i32 }, - lower_bound: Some(bounds.0), - upper_bound: Some(bounds.1), - args: vec![], + lower_bound: Some(Box::new(bounds.0)), + upper_bound: Some(Box::new(bounds.1)), bounds_type: bounds_type as i32, - })), + }))), } } @@ -152,16 +152,56 @@ fn to_substrait_bound(bound: &WindowFrameBound) -> datafusion::common::Result Ok(Bound { kind: Some(BoundKind::CurrentRow(SubstraitBound::CurrentRow {})), }), - WindowFrameBound::Preceding(s) => Ok(Bound { - kind: Some(BoundKind::Preceding(SubstraitBound::Preceding { - offset: to_substrait_bound_offset(s)?, - })), - }), - WindowFrameBound::Following(s) => Ok(Bound { - kind: Some(BoundKind::Following(SubstraitBound::Following { - offset: to_substrait_bound_offset(s)?, - })), - }), + WindowFrameBound::Preceding(s) => { + let offset = to_substrait_bound_offset(s)?; + if offset == 0 { + // A zero distance is equivalent to CurrentRow, and `offset` + // cannot represent zero, so the specification asks producers to + // emit CurrentRow instead of a zero bound. + return Ok(Bound { + kind: Some(BoundKind::CurrentRow(SubstraitBound::CurrentRow {})), + }); + } + #[expect(deprecated)] + Ok(Bound { + kind: Some(BoundKind::Preceding(Box::new(SubstraitBound::Preceding { + // `offset` carries the int64-literal equivalent for + // consumers that do not read `offset_expr` yet. + offset, + offset_expr: Some(Box::new(bound_offset_expr(offset))), + }))), + }) + } + WindowFrameBound::Following(s) => { + let offset = to_substrait_bound_offset(s)?; + if offset == 0 { + // A zero distance is equivalent to CurrentRow, and `offset` + // cannot represent zero, so the specification asks producers to + // emit CurrentRow instead of a zero bound. + return Ok(Bound { + kind: Some(BoundKind::CurrentRow(SubstraitBound::CurrentRow {})), + }); + } + #[expect(deprecated)] + Ok(Bound { + kind: Some(BoundKind::Following(Box::new(SubstraitBound::Following { + offset, + offset_expr: Some(Box::new(bound_offset_expr(offset))), + }))), + }) + } + } +} + +/// Builds the int64 literal expression that a window frame bound offset carries +/// in `offset_expr`. +fn bound_offset_expr(offset: i64) -> Expression { + Expression { + rex_type: Some(RexType::Literal(Literal { + nullable: false, + type_variation_reference: DEFAULT_TYPE_VARIATION_REF, + literal_type: Some(LiteralType::I64(offset)), + })), } } @@ -188,6 +228,25 @@ mod tests { use datafusion::common::assert_contains; #[test] + fn zero_distance_bounds_become_current_row() { + // `offset` cannot represent zero and the specification asks producers to + // emit CurrentRow rather than a zero bound, so neither field is written. + let frame = WindowFrame::new_bounds( + WindowFrameUnits::Rows, + WindowFrameBound::Preceding(ScalarValue::UInt64(Some(0))), + WindowFrameBound::Following(ScalarValue::UInt64(Some(0))), + ); + let (lower, upper) = to_substrait_bounds(&frame).unwrap(); + for bound in [lower, upper] { + assert_eq!( + bound.kind, + Some(BoundKind::CurrentRow(SubstraitBound::CurrentRow {})) + ); + } + } + + #[test] + #[expect(deprecated)] fn window_frame_offsets() { for value in [ ScalarValue::UInt8(Some(1)), @@ -207,9 +266,10 @@ mod tests { let (_, bound) = to_substrait_bounds(&frame).unwrap(); assert_eq!( bound.kind, - Some(BoundKind::Following(SubstraitBound::Following { - offset: 1 - })) + Some(BoundKind::Following(Box::new(SubstraitBound::Following { + offset: 1, + offset_expr: Some(Box::new(bound_offset_expr(1))), + }))) ); } diff --git a/datafusion/substrait/src/logical_plan/producer/plan.rs b/datafusion/substrait/src/logical_plan/producer/plan.rs index 3b58720dba832..f6bdaa3e69fa7 100644 --- a/datafusion/substrait/src/logical_plan/producer/plan.rs +++ b/datafusion/substrait/src/logical_plan/producer/plan.rs @@ -20,7 +20,7 @@ use crate::logical_plan::producer::{ }; use datafusion::execution::SessionState; use datafusion::logical_expr::{LogicalPlan, SubqueryAlias}; -use substrait::proto::{Plan, PlanRel, Rel, RelRoot, plan_rel}; +use substrait::proto::{ExecutionBehavior, Plan, PlanRel, Rel, RelRoot, plan_rel}; use substrait::version; /// Convert DataFusion LogicalPlan to Substrait Plan @@ -51,6 +51,9 @@ pub fn to_substrait_plan( expected_type_urls: vec![], parameter_bindings: vec![], type_aliases: vec![], + // DataFusion does not emit execution context variables, so the + // evaluation mode does not govern anything in the plans it produces. + execution_behavior: Some(ExecutionBehavior::default()), })) } diff --git a/datafusion/substrait/src/logical_plan/producer/rel/aggregate_rel.rs b/datafusion/substrait/src/logical_plan/producer/rel/aggregate_rel.rs index 7b6c113ccec0f..1f0d8ac89c4bc 100644 --- a/datafusion/substrait/src/logical_plan/producer/rel/aggregate_rel.rs +++ b/datafusion/substrait/src/logical_plan/producer/rel/aggregate_rel.rs @@ -67,6 +67,7 @@ fn grouping_set_output_mapping(grouping_count: usize, measure_count: usize) -> R emit_kind: Some(EmitKind::Emit(rel_common::Emit { output_mapping })), hint: None, advanced_extension: None, + rel_anchor: None, } } @@ -83,15 +84,16 @@ pub fn from_distinct( .map(substrait_field_ref) .collect::>>()?; - #[expect(deprecated)] + // The keys are declared once on the relation and the single grouping + // set references them by index. + let expression_references = (0..grouping.len() as u32).collect(); Ok(Box::new(Rel { rel_type: Some(RelType::Aggregate(Box::new(AggregateRel { common: None, input: Some(input), - grouping_expressions: vec![], + grouping_expressions: grouping, groupings: vec![Grouping { - grouping_expressions: grouping, - expression_references: vec![], + expression_references, }], measures: vec![], advanced_extension: None, @@ -179,11 +181,9 @@ pub fn parse_flat_grouping_exprs( ref_group_exprs: &mut Vec, ) -> datafusion::common::Result { let mut expression_references = vec![]; - let mut grouping_expressions = vec![]; for e in exprs { let rex = producer.handle_expr(e, schema)?; - grouping_expressions.push(rex.clone()); let reference = ref_group_exprs.iter().position(|existing| existing == &rex); let reference = reference.unwrap_or_else(|| { ref_group_exprs.push(rex); @@ -191,9 +191,7 @@ pub fn parse_flat_grouping_exprs( }); expression_references.push(reference as u32); } - #[expect(deprecated)] Ok(Grouping { - grouping_expressions, expression_references, }) } diff --git a/datafusion/substrait/src/logical_plan/producer/rel/fetch_rel.rs b/datafusion/substrait/src/logical_plan/producer/rel/fetch_rel.rs index e878b3816ff42..76418af562c6a 100644 --- a/datafusion/substrait/src/logical_plan/producer/rel/fetch_rel.rs +++ b/datafusion/substrait/src/logical_plan/producer/rel/fetch_rel.rs @@ -20,7 +20,7 @@ use datafusion::common::DFSchema; use datafusion::logical_expr::Limit; use std::sync::Arc; use substrait::proto::rel::RelType; -use substrait::proto::{FetchRel, Rel, fetch_rel}; +use substrait::proto::{FetchRel, Rel}; pub fn from_limit( producer: &mut impl SubstraitProducer, @@ -28,26 +28,24 @@ pub fn from_limit( ) -> datafusion::common::Result> { let input = producer.handle_plan(limit.input.as_ref())?; let empty_schema = Arc::new(DFSchema::empty()); - let offset_mode = limit + let offset_expr = limit .skip .as_ref() .map(|expr| producer.handle_expr(expr.as_ref(), &empty_schema)) .transpose()? - .map(Box::new) - .map(fetch_rel::OffsetMode::OffsetExpr); - let count_mode = limit + .map(Box::new); + let count_expr = limit .fetch .as_ref() .map(|expr| producer.handle_expr(expr.as_ref(), &empty_schema)) .transpose()? - .map(Box::new) - .map(fetch_rel::CountMode::CountExpr); + .map(Box::new); Ok(Box::new(Rel { rel_type: Some(RelType::Fetch(Box::new(FetchRel { common: None, input: Some(input), - offset_mode, - count_mode, + offset_expr, + count_expr, advanced_extension: None, }))), })) diff --git a/datafusion/substrait/src/logical_plan/producer/rel/project_rel.rs b/datafusion/substrait/src/logical_plan/producer/rel/project_rel.rs index 33920cdf86f7a..d610704e5bb82 100644 --- a/datafusion/substrait/src/logical_plan/producer/rel/project_rel.rs +++ b/datafusion/substrait/src/logical_plan/producer/rel/project_rel.rs @@ -39,6 +39,7 @@ pub fn from_projection( let common = RelCommon { emit_kind: Some(emit_kind), hint: None, + rel_anchor: None, advanced_extension: None, }; @@ -73,6 +74,7 @@ pub fn from_window( let common = RelCommon { emit_kind: Some(emit_kind), hint: None, + rel_anchor: None, advanced_extension: None, }; let project_rel = Box::new(ProjectRel { diff --git a/datafusion/substrait/src/logical_plan/producer/rel/read_rel.rs b/datafusion/substrait/src/logical_plan/producer/rel/read_rel.rs index 900273bf8e6d7..a12460f785a46 100644 --- a/datafusion/substrait/src/logical_plan/producer/rel/read_rel.rs +++ b/datafusion/substrait/src/logical_plan/producer/rel/read_rel.rs @@ -144,7 +144,6 @@ pub fn from_empty_relation( ReadType::VirtualTable(VirtualTable { expressions: vec![NestedStruct { fields }], - ..Default::default() }) } else { ReadType::VirtualTable(VirtualTable::default()) @@ -178,10 +177,7 @@ pub fn from_values( best_effort_filter: None, projection: None, advanced_extension: None, - read_type: Some(ReadType::VirtualTable(VirtualTable { - expressions, - ..Default::default() - })), + read_type: Some(ReadType::VirtualTable(VirtualTable { expressions })), }))), })) } diff --git a/datafusion/substrait/src/logical_plan/producer/rel/sort_rel.rs b/datafusion/substrait/src/logical_plan/producer/rel/sort_rel.rs index d4520a4c37b14..43d27ea792e69 100644 --- a/datafusion/substrait/src/logical_plan/producer/rel/sort_rel.rs +++ b/datafusion/substrait/src/logical_plan/producer/rel/sort_rel.rs @@ -21,7 +21,7 @@ use datafusion::logical_expr::Sort; use substrait::proto::expression::literal::LiteralType; use substrait::proto::expression::{Literal, RexType}; use substrait::proto::rel::RelType; -use substrait::proto::{Expression, FetchRel, Rel, SortRel, fetch_rel}; +use substrait::proto::{Expression, FetchRel, Rel, SortRel}; pub fn from_sort( producer: &mut impl SubstraitProducer, @@ -46,20 +46,19 @@ pub fn from_sort( match fetch { Some(amount) => { - let count_mode = - Some(fetch_rel::CountMode::CountExpr(Box::new(Expression { - rex_type: Some(RexType::Literal(Literal { - nullable: false, - type_variation_reference: DEFAULT_TYPE_VARIATION_REF, - literal_type: Some(LiteralType::I64(*amount as i64)), - })), - }))); + let count_expr = Some(Box::new(Expression { + rex_type: Some(RexType::Literal(Literal { + nullable: false, + type_variation_reference: DEFAULT_TYPE_VARIATION_REF, + literal_type: Some(LiteralType::I64(*amount as i64)), + })), + })); Ok(Box::new(Rel { rel_type: Some(RelType::Fetch(Box::new(FetchRel { common: None, input: Some(sort_rel), - offset_mode: None, - count_mode, + offset_expr: None, + count_expr, advanced_extension: None, }))), })) diff --git a/datafusion/substrait/src/logical_plan/producer/utils.rs b/datafusion/substrait/src/logical_plan/producer/utils.rs index e8310f4acd31e..13984eb6b5d5a 100644 --- a/datafusion/substrait/src/logical_plan/producer/utils.rs +++ b/datafusion/substrait/src/logical_plan/producer/utils.rs @@ -93,7 +93,6 @@ pub(crate) fn negate( ) -> Expression { let function_anchor = producer.register_function("not".to_string()); - #[expect(deprecated)] Expression { rex_type: Some(substrait::proto::expression::RexType::ScalarFunction( substrait::proto::expression::ScalarFunction { @@ -104,7 +103,6 @@ pub(crate) fn negate( )), }], output_type: None, - args: vec![], options: vec![], }, )), diff --git a/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_grouping_expressions.substrait.json b/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_grouping_expressions.substrait.json index 15c0b0505fa68..2901f7e537ed7 100644 --- a/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_grouping_expressions.substrait.json +++ b/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_grouping_expressions.substrait.json @@ -25,18 +25,19 @@ }, "groupings": [ { - "groupingExpressions": [ - { - "literal": { - "i32": 1 - } - }, - { - "literal": { - "i32": 1 - } - } - ] + "expressionReferences": [0, 1] + } + ], + "groupingExpressions": [ + { + "literal": { + "i32": 1 + } + }, + { + "literal": { + "i32": 1 + } } ], "measures": [] diff --git a/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_measures.substrait.json b/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_measures.substrait.json index 620d55e93ee1e..5186ace7f1e03 100644 --- a/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_measures.substrait.json +++ b/datafusion/substrait/tests/testdata/test_plans/aggregate_identical_measures.substrait.json @@ -39,8 +39,9 @@ } }, "groupings": [{ - "groupingExpressions": [] + "expressionReferences": [] }], + "groupingExpressions": [], "measures": [ { "measure": { diff --git a/datafusion/substrait/tests/testdata/test_plans/aggregate_no_project.substrait.json b/datafusion/substrait/tests/testdata/test_plans/aggregate_no_project.substrait.json index ed8675b968269..1394e3ca2bbf9 100644 --- a/datafusion/substrait/tests/testdata/test_plans/aggregate_no_project.substrait.json +++ b/datafusion/substrait/tests/testdata/test_plans/aggregate_no_project.substrait.json @@ -46,15 +46,8 @@ }, "groupings": [ { - "groupingExpressions": [ - { - "selection": { - "directReference": { - "structField": {} - }, - "rootReference": {} - } - } + "expressionReferences": [ + 0 ] } ], @@ -80,6 +73,16 @@ ] } } + ], + "groupingExpressions": [ + { + "selection": { + "directReference": { + "structField": {} + }, + "rootReference": {} + } + } ] } }, diff --git a/datafusion/substrait/tests/testdata/test_plans/aggregate_sorted_no_project.substrait.json b/datafusion/substrait/tests/testdata/test_plans/aggregate_sorted_no_project.substrait.json index d5170223cd65b..2ef9c86eee2c0 100644 --- a/datafusion/substrait/tests/testdata/test_plans/aggregate_sorted_no_project.substrait.json +++ b/datafusion/substrait/tests/testdata/test_plans/aggregate_sorted_no_project.substrait.json @@ -46,16 +46,17 @@ }, "groupings": [ { - "groupingExpressions": [ - { - "selection": { - "directReference": { - "structField": {} - }, - "rootReference": {} - } - } - ] + "expressionReferences": [0] + } + ], + "groupingExpressions": [ + { + "selection": { + "directReference": { + "structField": {} + }, + "rootReference": {} + } } ], "measures": [ diff --git a/datafusion/substrait/tests/testdata/test_plans/join_with_expression_key.json b/datafusion/substrait/tests/testdata/test_plans/join_with_expression_key.json index 8a81a9a0c780f..f1cda34c7715a 100644 --- a/datafusion/substrait/tests/testdata/test_plans/join_with_expression_key.json +++ b/datafusion/substrait/tests/testdata/test_plans/join_with_expression_key.json @@ -226,28 +226,29 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } + "expressionReferences": [0, 1] + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 1 } - }, { - "selection": { - "directReference": { - "structField": { - "field": 2 - } - }, - "rootReference": { - } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 2 } - }] - }], + }, + "rootReference": { + } + } + }], "measures": [{ "measure": { "functionReference": 2, @@ -531,28 +532,29 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } + "expressionReferences": [0, 1] + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 1 } - }, { - "selection": { - "directReference": { - "structField": { - "field": 2 - } - }, - "rootReference": { - } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 2 } - }] - }], + }, + "rootReference": { + } + } + }], "measures": [{ "measure": { "functionReference": 2, @@ -679,18 +681,19 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } + }], "measures": [{ "measure": { "functionReference": 2, diff --git a/datafusion/substrait/tests/testdata/test_plans/multilayer_aggregate.substrait.json b/datafusion/substrait/tests/testdata/test_plans/multilayer_aggregate.substrait.json index 1f47b916daf0f..3e5f9f4aedc6f 100644 --- a/datafusion/substrait/tests/testdata/test_plans/multilayer_aggregate.substrait.json +++ b/datafusion/substrait/tests/testdata/test_plans/multilayer_aggregate.substrait.json @@ -52,43 +52,27 @@ "input": { "read": { "common": { - "direct": {} + "direct": { + } }, "baseSchema": { - "names": [ - "product" - ], + "names": ["product"], "struct": { - "types": [ - { - "string": { - "nullability": "NULLABILITY_REQUIRED" - } + "types": [{ + "string": { + "nullability": "NULLABILITY_REQUIRED" } - ], + }], "nullability": "NULLABILITY_REQUIRED" } }, "namedTable": { - "names": [ - "sales" - ] + "names": ["sales"] } } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 0 - } - }, - "rootReference": { - } - } - }], - "expressionReferences": [] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -119,22 +103,21 @@ "options": [] } }], - "groupingExpressions": [] + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 0 + } + }, + "rootReference": { + } + } + }] } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 0 - } - }, - "rootReference": { - } - } - }], - "expressionReferences": [] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -165,7 +148,17 @@ "options": [] } }], - "groupingExpressions": [] + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 0 + } + }, + "rootReference": { + } + } + }] } }, "expressions": [{ diff --git a/datafusion/substrait/tests/testdata/test_plans/multiple_joins.json b/datafusion/substrait/tests/testdata/test_plans/multiple_joins.json index 15c0313b43b54..7de3d1d042562 100644 --- a/datafusion/substrait/tests/testdata/test_plans/multiple_joins.json +++ b/datafusion/substrait/tests/testdata/test_plans/multiple_joins.json @@ -72,46 +72,28 @@ } }, "virtualTable": { - "expressions": [ - { - "fields": [ - { - "literal": { - "i64": "1", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - }, - { - "fields": [ - { - "literal": { - "i64": "2", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - } - ] + "expressions": [{ + "fields": [{ + "literal": { + "i64": "1", + "nullable": true, + "typeVariationReference": 0 + } + }] + }, { + "fields": [{ + "literal": { + "i64": "2", + "nullable": true, + "typeVariationReference": 0 + } + }] + }] } } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 0 - } - }, - "rootReference": { - } - } - }], - "expressionReferences": [] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -130,7 +112,17 @@ "options": [] } }], - "groupingExpressions": [] + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 0 + } + }, + "rootReference": { + } + } + }] } }, "right": { @@ -164,73 +156,63 @@ } }, "virtualTable": { - "expressions": [ - { - "fields": [ - { - "literal": { - "i64": "1", - "nullable": true, - "typeVariationReference": 0 - } - }, - { - "literal": { - "string": "info", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - }, - { - "fields": [ - { - "literal": { - "i64": "2", - "nullable": true, - "typeVariationReference": 0 - } - }, - { - "literal": { - "string": "low", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - } - ] + "expressions": [{ + "fields": [{ + "literal": { + "i64": "1", + "nullable": true, + "typeVariationReference": 0 + } + }, { + "literal": { + "string": "info", + "nullable": true, + "typeVariationReference": 0 + } + }] + }, { + "fields": [{ + "literal": { + "i64": "2", + "nullable": true, + "typeVariationReference": 0 + } + }, { + "literal": { + "string": "low", + "nullable": true, + "typeVariationReference": 0 + } + }] + }] } } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 0 - } - }, - "rootReference": { + "expressionReferences": [0, 1] + }], + "measures": [], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 0 } + }, + "rootReference": { } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 } + }, + "rootReference": { } - }], - "expressionReferences": [] - }], - "measures": [], - "groupingExpressions": [] + } + }] } }, "expression": { @@ -300,46 +282,28 @@ } }, "virtualTable": { - "expressions": [ - { - "fields": [ - { - "literal": { - "i64": "1", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - }, - { - "fields": [ - { - "literal": { - "i64": "2", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - } - ] + "expressions": [{ + "fields": [{ + "literal": { + "i64": "1", + "nullable": true, + "typeVariationReference": 0 + } + }] + }, { + "fields": [{ + "literal": { + "i64": "2", + "nullable": true, + "typeVariationReference": 0 + } + }] + }] } } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 0 - } - }, - "rootReference": { - } - } - }], - "expressionReferences": [] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -358,7 +322,17 @@ "options": [] } }], - "groupingExpressions": [] + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 0 + } + }, + "rootReference": { + } + } + }] } }, "expression": { @@ -428,46 +402,28 @@ } }, "virtualTable": { - "expressions": [ - { - "fields": [ - { - "literal": { - "i64": "1", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - }, - { - "fields": [ - { - "literal": { - "i64": "2", - "nullable": true, - "typeVariationReference": 0 - } - } - ] - } - ] + "expressions": [{ + "fields": [{ + "literal": { + "i64": "1", + "nullable": true, + "typeVariationReference": 0 + } + }] + }, { + "fields": [{ + "literal": { + "i64": "2", + "nullable": true, + "typeVariationReference": 0 + } + }] + }] } } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - "field": 0 - } - }, - "rootReference": { - } - } - }], - "expressionReferences": [] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -486,7 +442,17 @@ "options": [] } }], - "groupingExpressions": [] + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + "field": 0 + } + }, + "rootReference": { + } + } + }] } }, "expression": { diff --git a/datafusion/substrait/tests/testdata/test_plans/select_count_from_select_1.substrait.json b/datafusion/substrait/tests/testdata/test_plans/select_count_from_select_1.substrait.json index b0d4ba4813bcf..9e223e16e38f1 100644 --- a/datafusion/substrait/tests/testdata/test_plans/select_count_from_select_1.substrait.json +++ b/datafusion/substrait/tests/testdata/test_plans/select_count_from_select_1.substrait.json @@ -60,7 +60,6 @@ }, "groupings": [ { - "groupingExpressions": [], "expressionReferences": [] } ], diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_01_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_01_plan.json index 3738a50a62382..b51e4996f657e 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_01_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_01_plan.json @@ -472,26 +472,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0, 1] }], "measures": [{ "measure": { @@ -686,6 +667,26 @@ }, "invocation": "AGGREGATION_INVOCATION_ALL" } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_02_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_02_plan.json index f6c5e802a5e36..20ce5529da83f 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_02_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_02_plan.json @@ -1148,7 +1148,11 @@ }] } }, - "count": "100" + "countExpr": { + "literal": { + "i64": "100" + } + } } }, "names": ["S_ACCTBAL", "S_NAME", "N_NAME", "P_PARTKEY", "P_MFGR", "S_ADDRESS", "S_PHONE", "S_COMMENT"] diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_03_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_03_plan.json index d4dea1d03c466..e90059e784353 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_03_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_03_plan.json @@ -602,36 +602,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 2 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0, 1, 2] }], "measures": [{ "measure": { @@ -659,6 +630,36 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 2 + } + }, + "rootReference": { + } + } }] } }, @@ -733,7 +734,11 @@ }] } }, - "count": "10" + "countExpr": { + "literal": { + "i64": "10" + } + } } }, "names": ["L_ORDERKEY", "REVENUE", "O_ORDERDATE", "O_SHIPPRIORITY"] diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_04_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_04_plan.json index 3e665f50f3207..b25b47a86193e 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_04_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_04_plan.json @@ -418,16 +418,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -440,6 +431,16 @@ }, "invocation": "AGGREGATION_INVOCATION_ALL" } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_05_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_05_plan.json index d42975d3326d8..791cdfd367ff0 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_05_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_05_plan.json @@ -850,16 +850,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -887,6 +878,16 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_07_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_07_plan.json index 82740fb3d87bd..62e385af3878f 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_07_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_07_plan.json @@ -988,36 +988,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 2 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0, 1, 2] }], "measures": [{ "measure": { @@ -1045,6 +1016,36 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 2 + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_08_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_08_plan.json index 8c886f84ed166..cd53224b51159 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_08_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_08_plan.json @@ -1167,16 +1167,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -1230,6 +1221,16 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_09_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_09_plan.json index 04b367a0b5bfe..6d54d90ae2a17 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_09_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_09_plan.json @@ -873,26 +873,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0, 1] }], "measures": [{ "measure": { @@ -920,6 +901,26 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_10_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_10_plan.json index 2daa1dabb4238..a9265f981f263 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_10_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_10_plan.json @@ -720,76 +720,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 2 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 3 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 4 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 5 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 6 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0, 1, 2, 3, 4, 5, 6] }], "measures": [{ "measure": { @@ -817,6 +748,76 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 2 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 3 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 4 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 5 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 6 + } + }, + "rootReference": { + } + } }] } }, @@ -918,7 +919,11 @@ }] } }, - "count": "20" + "countExpr": { + "literal": { + "i64": "20" + } + } } }, "names": ["C_CUSTKEY", "C_NAME", "REVENUE", "C_ACCTBAL", "N_NAME", "C_ADDRESS", "C_PHONE", "C_COMMENT"] diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_11_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_11_plan.json index d79b065403d54..8434543e26563 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_11_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_11_plan.json @@ -384,16 +384,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -421,6 +412,16 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_12_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_12_plan.json index db3100052704c..abd112f405794 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_12_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_12_plan.json @@ -711,16 +711,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -770,6 +761,16 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_13_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_13_plan.json index 19b80b0aac73a..e73087b69cee2 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_13_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_13_plan.json @@ -326,16 +326,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -361,6 +352,16 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, @@ -378,16 +379,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -400,6 +392,16 @@ }, "invocation": "AGGREGATION_INVOCATION_ALL" } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_16_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_16_plan.json index bf97fb9185711..138bc91097c75 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_16_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_16_plan.json @@ -754,36 +754,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 2 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0, 1, 2] }], "measures": [{ "measure": { @@ -809,6 +780,36 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 2 + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_18_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_18_plan.json index 7f0ff438db78e..a6455b458ee29 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_18_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_18_plan.json @@ -441,16 +441,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -478,6 +469,16 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, @@ -678,56 +679,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 1 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 2 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 3 - } - }, - "rootReference": { - } - } - }, { - "selection": { - "directReference": { - "structField": { - "field": 4 - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0, 1, 2, 3, 4] }], "measures": [{ "measure": { @@ -755,6 +707,56 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 1 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 2 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 3 + } + }, + "rootReference": { + } + } + }, { + "selection": { + "directReference": { + "structField": { + "field": 4 + } + }, + "rootReference": { + } + } }] } }, @@ -787,7 +789,11 @@ }] } }, - "count": "100" + "countExpr": { + "literal": { + "i64": "100" + } + } } }, "names": ["C_NAME", "C_CUSTKEY", "O_ORDERKEY", "O_ORDERDATE", "O_TOTALPRICE", "EXPR$5"] diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_21_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_21_plan.json index c3d4fc3bcb873..9d4b1dab24c54 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_21_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_21_plan.json @@ -988,16 +988,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -1010,6 +1001,16 @@ }, "invocation": "AGGREGATION_INVOCATION_ALL" } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, @@ -1041,7 +1042,11 @@ }] } }, - "count": "100" + "countExpr": { + "literal": { + "i64": "100" + } + } } }, "names": ["S_NAME", "NUMWAIT"] diff --git a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_22_plan.json b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_22_plan.json index fcd61b23ae2de..e1bcc23bfdba6 100644 --- a/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_22_plan.json +++ b/datafusion/substrait/tests/testdata/tpch_substrait_plans/query_22_plan.json @@ -1438,16 +1438,7 @@ } }, "groupings": [{ - "groupingExpressions": [{ - "selection": { - "directReference": { - "structField": { - } - }, - "rootReference": { - } - } - }] + "expressionReferences": [0] }], "measures": [{ "measure": { @@ -1486,6 +1477,16 @@ } }] } + }], + "groupingExpressions": [{ + "selection": { + "directReference": { + "structField": { + } + }, + "rootReference": { + } + } }] } }, diff --git a/datafusion/substrait/tests/utils.rs b/datafusion/substrait/tests/utils.rs index 89363931f1594..1e1f3fec1b569 100644 --- a/datafusion/substrait/tests/utils.rs +++ b/datafusion/substrait/tests/utils.rs @@ -157,7 +157,6 @@ pub mod test { Ok(()) } - #[expect(deprecated)] fn collect_schemas_from_rel(&mut self, rel: &Rel) -> Result<()> { let rel_type = rel .rel_type @@ -195,10 +194,8 @@ pub mod test { } RelType::Aggregate(a) => { self.apply(a.input.as_ref().map(|b| b.as_ref()))?; - for grouping in a.groupings.iter() { - for expr in grouping.grouping_expressions.iter() { - self.collect_schemas_from_expr(expr)? - } + for expr in a.grouping_expressions.iter() { + self.collect_schemas_from_expr(expr)? } for measure in a.measures.iter() { if let Some(agg_fn) = measure.measure.as_ref() { @@ -483,9 +480,7 @@ pub mod test { } } RexType::DynamicParameter(_) => {} - // Enum is deprecated - #[expect(deprecated)] - RexType::Enum(_) => {} + RexType::ExecutionContextVariable(_) => {} RexType::Lambda(_) | RexType::LambdaInvocation(_) => {} } Ok(())