From 6487dfe18bb091ac11437ea4042c5fb18d6828f3 Mon Sep 17 00:00:00 2001 From: Wendell Smith Date: Mon, 13 Apr 2026 15:00:57 -0400 Subject: [PATCH 1/2] refactor: extract extract_mapping helper to simplify from_extensions --- src/extensions/simple.rs | 56 ++++++++++---------- src/textify/plan.rs | 111 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 28 deletions(-) diff --git a/src/extensions/simple.rs b/src/extensions/simple.rs index 3e9933bf..3e958611 100644 --- a/src/extensions/simple.rs +++ b/src/extensions/simple.rs @@ -134,6 +134,31 @@ pub struct SimpleExtensions { extensions: BTreeMap<(u32, ExtensionKind), (u32, CompoundName)>, } +/// Extract the kind, URN reference, anchor, and name from an extension mapping. +fn extract_mapping(mapping: &Option) -> Option<(ExtensionKind, u32, u32, String)> { + match mapping { + Some(MappingType::ExtensionType(t)) => Some(( + ExtensionKind::Type, + t.extension_urn_reference, + t.type_anchor, + t.name.clone(), + )), + Some(MappingType::ExtensionFunction(f)) => Some(( + ExtensionKind::Function, + f.extension_urn_reference, + f.function_anchor, + f.name.clone(), + )), + Some(MappingType::ExtensionTypeVariation(v)) => Some(( + ExtensionKind::TypeVariation, + v.extension_urn_reference, + v.type_variation_anchor, + v.name.clone(), + )), + None => None, + } +} + impl SimpleExtensions { pub fn new() -> Self { Self::default() @@ -157,34 +182,9 @@ impl SimpleExtensions { } for extension in extensions { - match &extension.mapping_type { - Some(MappingType::ExtensionType(t)) => { - if let Err(e) = exts.add_extension( - ExtensionKind::Type, - t.extension_urn_reference, - t.type_anchor, - t.name.clone(), - ) { - errors.push(e); - } - } - Some(MappingType::ExtensionFunction(f)) => { - if let Err(e) = exts.add_extension( - ExtensionKind::Function, - f.extension_urn_reference, - f.function_anchor, - f.name.clone(), - ) { - errors.push(e); - } - } - Some(MappingType::ExtensionTypeVariation(v)) => { - if let Err(e) = exts.add_extension( - ExtensionKind::TypeVariation, - v.extension_urn_reference, - v.type_variation_anchor, - v.name.clone(), - ) { + match extract_mapping(&extension.mapping_type) { + Some((kind, urn_ref, anchor, name)) => { + if let Err(e) = exts.add_extension(kind, urn_ref, anchor, name) { errors.push(e); } } diff --git a/src/textify/plan.rs b/src/textify/plan.rs index 01773e87..36703bcd 100644 --- a/src/textify/plan.rs +++ b/src/textify/plan.rs @@ -227,4 +227,115 @@ Project[$0, $1, add($0, $1)] "Output:\n---\n{output}\n---\nExpected:\n---\n{expected}\n---" ); } + + /// Reproduces #96: when a plan only has the deprecated `extension_uri_reference` + /// field set (not `extension_urn_reference`), the formatter should still resolve + /// the correct URN anchor. + #[test] + fn test_deprecated_extension_uri_reference() { + let mut plan = proto::Plan::default(); + + // Add extension URN with anchor 1 + plan.extension_urns.push(pext::SimpleExtensionUrn { + extension_urn_anchor: 1, + urn: "/functions_comparison.yaml".to_string(), + }); + + // Use ONLY the deprecated extension_uri_reference field (tag 1), + // leaving extension_urn_reference (tag 4) at default 0. + // This is what happens when deserializing JSON from older producers. + plan.extensions.push(pext::SimpleExtensionDeclaration { + #[allow(deprecated)] + mapping_type: Some(MappingType::ExtensionFunction(ExtensionFunction { + extension_urn_reference: 0, + extension_uri_reference: 1, + function_anchor: 1, + name: "is_not_null:any".to_string(), + })), + }); + + // Create a simple Read + let read_rel = ReadRel { + read_type: Some(ReadType::NamedTable(NamedTable { + names: vec!["my_table".to_string()], + ..Default::default() + })), + base_schema: Some(NamedStruct { + names: vec!["x".to_string()], + r#struct: Some(Struct { + types: vec![Type { + kind: Some(Kind::I64(proto::r#type::I64 { + nullability: Nullability::Nullable as i32, + type_variation_reference: 0, + })), + }], + ..Default::default() + }), + }), + ..Default::default() + }; + + // Create a Filter using the function + let filter_rel = proto::FilterRel { + input: Some(Box::new(proto::Rel { + rel_type: Some(proto::rel::RelType::Read(Box::new(read_rel))), + })), + condition: Some(Box::new(Expression { + rex_type: Some(RexType::ScalarFunction(ScalarFunction { + function_reference: 1, + arguments: vec![FunctionArgument { + arg_type: Some(ArgType::Value(Expression { + rex_type: Some(RexType::Selection(Box::new( + FieldIndex(0).to_field_reference(), + ))), + })), + }], + output_type: None, + options: vec![], + #[allow(deprecated)] + args: vec![], + })), + })), + common: None, + advanced_extension: None, + }; + + // Wrap in Root + plan.relations.push(proto::PlanRel { + rel_type: Some(proto::plan_rel::RelType::Root(proto::RelRoot { + input: Some(proto::Rel { + rel_type: Some(proto::rel::RelType::Filter(Box::new(filter_rel))), + }), + names: vec!["x".to_string()], + })), + }); + + let options = OutputOptions::default(); + let extension_registry = ExtensionRegistry::new(); + let (writer, errors) = PlanWriter::::new(&options, &plan, &extension_registry); + let mut output = String::new(); + write!(output, "{writer}").unwrap(); + + let errors: Vec<_> = errors.into(); + assert!(errors.is_empty(), "Expected no errors, got: {errors:?}"); + + let expected = r#" +=== Extensions +URNs: + @ 1: /functions_comparison.yaml +Functions: + # 1 @ 1: is_not_null:any + +=== Plan +Root[x] + Filter[is_not_null($0) => $0] + Read[my_table => x:i64?] +"# + .trim_start(); + + assert_eq!( + output, expected, + "Output:\n---\n{output}\n---\nExpected:\n---\n{expected}\n---" + ); + } } From a20acf97defb4cb551e8875e7b1f7638e72cc02f Mon Sep 17 00:00:00 2001 From: Wendell Smith Date: Mon, 13 Apr 2026 20:53:28 -0400 Subject: [PATCH 2/2] fix: detect and report deprecated URI-based extension fields (#96) --- src/extensions/simple.rs | 80 +++++++++++++++-- src/textify/plan.rs | 180 +++++++++++++++------------------------ tests/json_parsing.rs | 108 ++++++++++++++++++++++- 3 files changed, 249 insertions(+), 119 deletions(-) diff --git a/src/extensions/simple.rs b/src/extensions/simple.rs index 3e958611..8689ad56 100644 --- a/src/extensions/simple.rs +++ b/src/extensions/simple.rs @@ -135,21 +135,65 @@ pub struct SimpleExtensions { } /// Extract the kind, URN reference, anchor, and name from an extension mapping. -fn extract_mapping(mapping: &Option) -> Option<(ExtensionKind, u32, u32, String)> { +/// +/// Declarations that only use the deprecated URI reference field are skipped +/// here. Plan-level formatting reports those fields separately; treating their +/// default URN reference as real would reintroduce the misleading +/// "Missing URN anchor 0" diagnostic. +enum ExtractedMapping { + Urn(ExtensionKind, u32, u32, String), + DeprecatedUriOnly, +} + +#[allow(deprecated)] +fn extract_mapping(mapping: &Option) -> Option { match mapping { - Some(MappingType::ExtensionType(t)) => Some(( + Some(MappingType::ExtensionType(t)) if t.extension_urn_reference != 0 => { + Some(ExtractedMapping::Urn( + ExtensionKind::Type, + t.extension_urn_reference, + t.type_anchor, + t.name.clone(), + )) + } + Some(MappingType::ExtensionType(t)) if t.extension_uri_reference != 0 => { + Some(ExtractedMapping::DeprecatedUriOnly) + } + Some(MappingType::ExtensionType(t)) => Some(ExtractedMapping::Urn( ExtensionKind::Type, t.extension_urn_reference, t.type_anchor, t.name.clone(), )), - Some(MappingType::ExtensionFunction(f)) => Some(( + Some(MappingType::ExtensionFunction(f)) if f.extension_urn_reference != 0 => { + Some(ExtractedMapping::Urn( + ExtensionKind::Function, + f.extension_urn_reference, + f.function_anchor, + f.name.clone(), + )) + } + Some(MappingType::ExtensionFunction(f)) if f.extension_uri_reference != 0 => { + Some(ExtractedMapping::DeprecatedUriOnly) + } + Some(MappingType::ExtensionFunction(f)) => Some(ExtractedMapping::Urn( ExtensionKind::Function, f.extension_urn_reference, f.function_anchor, f.name.clone(), )), - Some(MappingType::ExtensionTypeVariation(v)) => Some(( + Some(MappingType::ExtensionTypeVariation(v)) if v.extension_urn_reference != 0 => { + Some(ExtractedMapping::Urn( + ExtensionKind::TypeVariation, + v.extension_urn_reference, + v.type_variation_anchor, + v.name.clone(), + )) + } + Some(MappingType::ExtensionTypeVariation(v)) if v.extension_uri_reference != 0 => { + Some(ExtractedMapping::DeprecatedUriOnly) + } + Some(MappingType::ExtensionTypeVariation(v)) => Some(ExtractedMapping::Urn( ExtensionKind::TypeVariation, v.extension_urn_reference, v.type_variation_anchor, @@ -183,11 +227,12 @@ impl SimpleExtensions { for extension in extensions { match extract_mapping(&extension.mapping_type) { - Some((kind, urn_ref, anchor, name)) => { + Some(ExtractedMapping::Urn(kind, urn_ref, anchor, name)) => { if let Err(e) = exts.add_extension(kind, urn_ref, anchor, name) { errors.push(e); } } + Some(ExtractedMapping::DeprecatedUriOnly) => {} None => { errors.push(InsertError::MissingMappingType); } @@ -600,6 +645,22 @@ mod tests { } } + fn new_ext_fn_uri_only( + anchor: u32, + uri_ref: u32, + name: &str, + ) -> pext::SimpleExtensionDeclaration { + pext::SimpleExtensionDeclaration { + #[allow(deprecated)] + mapping_type: Some(MappingType::ExtensionFunction(ExtensionFunction { + extension_urn_reference: Default::default(), + extension_uri_reference: uri_ref, + function_anchor: anchor, + name: name.to_string(), + })), + } + } + fn new_ext_type(anchor: u32, urn_ref: u32, name: &str) -> pext::SimpleExtensionDeclaration { #[allow(deprecated)] pext::SimpleExtensionDeclaration { @@ -750,6 +811,15 @@ mod tests { assert_eq!(err, &InsertError::MissingMappingType); } + #[test] + fn test_from_extensions_skips_deprecated_uri_only_mappings() { + let extensions = vec![new_ext_fn_uri_only(10, 1, "func")]; + let (exts, errs) = SimpleExtensions::from_extensions(vec![], &extensions); + + assert_no_errors(&errs); + assert!(exts.is_empty()); + } + #[test] fn test_find_by_name() { let urns = vec![new_urn(1, "urn1")]; diff --git a/src/textify/plan.rs b/src/textify/plan.rs index 36703bcd..741de3b3 100644 --- a/src/textify/plan.rs +++ b/src/textify/plan.rs @@ -5,7 +5,7 @@ use substrait::proto; use super::Textify; use crate::extensions::{ExtensionRegistry, SimpleExtensions}; use crate::parser::PLAN_HEADER; -use crate::textify::foundation::ErrorAccumulator; +use crate::textify::foundation::{ErrorAccumulator, FormatError, FormatErrorType, PlanError}; use crate::textify::{OutputOptions, ScopedContext}; #[derive(Debug, Clone)] @@ -31,6 +31,10 @@ impl<'a, E: ErrorAccumulator + Default + Clone> PlanWriter<'a, E> { errors.push(err.into()); } + for err in check_deprecated_uri_fields(plan) { + errors.push(err); + } + let relations = plan.relations.as_slice(); ( @@ -85,6 +89,69 @@ impl<'a, E: ErrorAccumulator + Default> fmt::Display for PlanWriter<'a, E> { } } +/// Check for deprecated URI-based extension fields in a plan. +/// +/// Substrait migrated from URI-based extensions (YAML file paths identified by +/// `extensionUris` / `extensionUriReference`) to URN-based extensions (structured +/// identifiers using `extensionUrns` / `extensionUrnReference`). Plans from older +/// producers may still use the URI fields; we report them clearly so users know +/// what to update. +#[allow(deprecated)] +fn check_deprecated_uri_fields(plan: &proto::Plan) -> Vec { + use substrait::proto::extensions::simple_extension_declaration::MappingType; + + let mut errors = Vec::new(); + + if !plan.extension_uris.is_empty() { + let n = plan.extension_uris.len(); + let noun = if n == 1 { "entry" } else { "entries" }; + errors.push(FormatError::Format(PlanError { + message: "extensions", + lookup: Some("extensionUris".into()), + description: format!( + "Plan uses unsupported deprecated extensionUris ({n} {noun}). \ + Update the input JSON to use extensionUrns (URN-based identifiers) \ + instead of extensionUris (YAML file paths)." + ) + .into(), + error_type: FormatErrorType::InvalidValue, + })); + } + + let deprecated_refs: Vec = plan + .extensions + .iter() + .filter_map(|ext| match &ext.mapping_type { + Some(MappingType::ExtensionFunction(f)) if f.extension_uri_reference != 0 => { + Some(format!("function #{}", f.function_anchor)) + } + Some(MappingType::ExtensionType(t)) if t.extension_uri_reference != 0 => { + Some(format!("type #{}", t.type_anchor)) + } + Some(MappingType::ExtensionTypeVariation(v)) if v.extension_uri_reference != 0 => { + Some(format!("type variation #{}", v.type_variation_anchor)) + } + _ => None, + }) + .collect(); + + if !deprecated_refs.is_empty() { + errors.push(FormatError::Format(PlanError { + message: "extensions", + lookup: Some("extensionUriReference".into()), + description: format!( + "Extension declarations use unsupported deprecated extensionUriReference \ + instead of extensionUrnReference: {}", + deprecated_refs.join(", ") + ) + .into(), + error_type: FormatErrorType::InvalidValue, + })); + } + + errors +} + #[cfg(test)] mod tests { use std::fmt::Write; @@ -227,115 +294,4 @@ Project[$0, $1, add($0, $1)] "Output:\n---\n{output}\n---\nExpected:\n---\n{expected}\n---" ); } - - /// Reproduces #96: when a plan only has the deprecated `extension_uri_reference` - /// field set (not `extension_urn_reference`), the formatter should still resolve - /// the correct URN anchor. - #[test] - fn test_deprecated_extension_uri_reference() { - let mut plan = proto::Plan::default(); - - // Add extension URN with anchor 1 - plan.extension_urns.push(pext::SimpleExtensionUrn { - extension_urn_anchor: 1, - urn: "/functions_comparison.yaml".to_string(), - }); - - // Use ONLY the deprecated extension_uri_reference field (tag 1), - // leaving extension_urn_reference (tag 4) at default 0. - // This is what happens when deserializing JSON from older producers. - plan.extensions.push(pext::SimpleExtensionDeclaration { - #[allow(deprecated)] - mapping_type: Some(MappingType::ExtensionFunction(ExtensionFunction { - extension_urn_reference: 0, - extension_uri_reference: 1, - function_anchor: 1, - name: "is_not_null:any".to_string(), - })), - }); - - // Create a simple Read - let read_rel = ReadRel { - read_type: Some(ReadType::NamedTable(NamedTable { - names: vec!["my_table".to_string()], - ..Default::default() - })), - base_schema: Some(NamedStruct { - names: vec!["x".to_string()], - r#struct: Some(Struct { - types: vec![Type { - kind: Some(Kind::I64(proto::r#type::I64 { - nullability: Nullability::Nullable as i32, - type_variation_reference: 0, - })), - }], - ..Default::default() - }), - }), - ..Default::default() - }; - - // Create a Filter using the function - let filter_rel = proto::FilterRel { - input: Some(Box::new(proto::Rel { - rel_type: Some(proto::rel::RelType::Read(Box::new(read_rel))), - })), - condition: Some(Box::new(Expression { - rex_type: Some(RexType::ScalarFunction(ScalarFunction { - function_reference: 1, - arguments: vec![FunctionArgument { - arg_type: Some(ArgType::Value(Expression { - rex_type: Some(RexType::Selection(Box::new( - FieldIndex(0).to_field_reference(), - ))), - })), - }], - output_type: None, - options: vec![], - #[allow(deprecated)] - args: vec![], - })), - })), - common: None, - advanced_extension: None, - }; - - // Wrap in Root - plan.relations.push(proto::PlanRel { - rel_type: Some(proto::plan_rel::RelType::Root(proto::RelRoot { - input: Some(proto::Rel { - rel_type: Some(proto::rel::RelType::Filter(Box::new(filter_rel))), - }), - names: vec!["x".to_string()], - })), - }); - - let options = OutputOptions::default(); - let extension_registry = ExtensionRegistry::new(); - let (writer, errors) = PlanWriter::::new(&options, &plan, &extension_registry); - let mut output = String::new(); - write!(output, "{writer}").unwrap(); - - let errors: Vec<_> = errors.into(); - assert!(errors.is_empty(), "Expected no errors, got: {errors:?}"); - - let expected = r#" -=== Extensions -URNs: - @ 1: /functions_comparison.yaml -Functions: - # 1 @ 1: is_not_null:any - -=== Plan -Root[x] - Filter[is_not_null($0) => $0] - Read[my_table => x:i64?] -"# - .trim_start(); - - assert_eq!( - output, expected, - "Output:\n---\n{output}\n---\nExpected:\n---\n{expected}\n---" - ); - } } diff --git a/tests/json_parsing.rs b/tests/json_parsing.rs index 47f71559..8c181969 100644 --- a/tests/json_parsing.rs +++ b/tests/json_parsing.rs @@ -5,7 +5,8 @@ //! and the compiled descriptor binary are all generated at build time by //! `build.rs` using protox and prost-build. -use substrait_explain::cli::{Cli, Commands, Format}; +use substrait::proto::Plan; +use substrait_explain::cli::{Cli, Commands, Format, Outcome}; use substrait_explain::extensions::{ Explainable, ExtensionArgs, ExtensionColumn, ExtensionError, ExtensionRegistry, ExtensionRelationType, ExtensionValue, @@ -68,7 +69,7 @@ fn build_registry() -> ExtensionRegistry { r } -fn format_plan(plan: &substrait::proto::Plan) -> String { +fn format_plan(plan: &Plan) -> String { let registry = build_registry(); let (text, errors) = format_with_registry(plan, &OutputOptions::default(), ®istry); assert!( @@ -82,6 +83,37 @@ const PLAN_TEXT: &str = include_str!("json_parsing/plan.substrait"); const PLAN_PBJSON: &str = include_str!("json_parsing/plan_pbjson.json"); // rust json const PLAN_PROTOJSON: &str = include_str!("json_parsing/plan_protojson.json"); // go json +const DEPRECATED_EXTENSION_URI_JSON: &str = r#"{ + "extensionUris": [ + { "extensionUriAnchor": 3, "uri": "/functions_comparison.yaml" } + ], + "extensions": [{ + "extensionFunction": { + "extensionUriReference": 3, + "functionAnchor": 7, + "name": "is_not_null:any" + } + }], + "relations": [{ + "root": { + "input": { + "read": { + "common": { "direct": {} }, + "baseSchema": { + "names": ["x"], + "struct": { + "types": [{ "i64": { "nullability": "NULLABILITY_NULLABLE" } }], + "nullability": "NULLABILITY_REQUIRED" + } + }, + "namedTable": { "names": ["my_table"] } + } + }, + "names": ["x"] + } + }] +}"#; + #[test] fn test_text_path() { let registry = build_registry(); @@ -260,3 +292,75 @@ fn test_cli_parses_standard_plan_json() { let result = String::from_utf8(output).unwrap(); assert!(result.contains("Read[data => a:i64, b:string?]")); } + +/// Reproduces #96: JSON plans using the deprecated `extensionUris` and +/// `extensionUriReference` fields should produce a clear error, not a +/// mysterious "Missing URN anchor 0" message. +#[test] +fn test_deprecated_extension_uris_produces_clear_error() { + let plan: Plan = serde_json::from_str(DEPRECATED_EXTENSION_URI_JSON).unwrap(); + let (text, errors) = substrait_explain::format(&plan); + + // Should still produce best-effort output (relations render fine) + assert!( + text.contains("Read[my_table => x:i64?]"), + "Expected best-effort plan output, got:\n{text}" + ); + + // Should produce clear errors about both deprecated fields + assert!( + !errors.is_empty(), + "deprecated URI fields should be reported in the error channel" + ); + let error_text = format!("{errors:?}"); + + assert!( + error_text.contains("deprecated extensionUris"), + "Expected error about deprecated extensionUris, got:\n{error_text}" + ); + assert!( + error_text.contains("deprecated extensionUriReference"), + "Expected error about deprecated extensionUriReference, got:\n{error_text}" + ); + assert!( + error_text.contains("function #7"), + "Expected deprecated reference error to identify the function anchor, got:\n{error_text}" + ); + assert!( + !error_text.contains("Missing URN anchor 0"), + "Deprecated URI-only declarations should not also emit the old confusing error:\n{error_text}" + ); +} + +#[test] +fn test_cli_reports_deprecated_extension_uris_as_formatting_issues() { + let cli = make_cli(Format::Json); + let mut output = Vec::new(); + + let outcome = cli + .run_with_io( + std::io::Cursor::new(DEPRECATED_EXTENSION_URI_JSON), + &mut output, + &ExtensionRegistry::default(), + ) + .expect("CLI should still produce best-effort output"); + + let result = String::from_utf8(output).unwrap(); + assert!( + result.contains("Read[my_table => x:i64?]"), + "Expected best-effort plan output, got:\n{result}" + ); + + let Outcome::HadFormattingIssues(errors) = outcome else { + panic!("Expected deprecated URI fields to produce formatting issues, got {outcome:?}"); + }; + let error_text = format!("{errors:?}"); + assert!( + error_text.contains("deprecated extensionUris"), + "Expected error about deprecated extensionUris, got:\n{error_text}" + ); + assert!( + error_text.contains("deprecated extensionUriReference"), + "Expected error about deprecated extensionUriReference, got:\n{error_text}" + ); +}