diff --git a/src/extensions/simple.rs b/src/extensions/simple.rs index 3e9933bf..8689ad56 100644 --- a/src/extensions/simple.rs +++ b/src/extensions/simple.rs @@ -134,6 +134,75 @@ pub struct SimpleExtensions { extensions: BTreeMap<(u32, ExtensionKind), (u32, CompoundName)>, } +/// Extract the kind, URN reference, anchor, and name from an extension mapping. +/// +/// 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)) 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)) 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)) 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, + v.name.clone(), + )), + None => None, + } +} + impl SimpleExtensions { pub fn new() -> Self { Self::default() @@ -157,37 +226,13 @@ 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(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 01773e87..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; 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}" + ); +}