feat: major internal refactoring for extensions (#45)

BREAKING CHANGE: various substrait_validator::output types relating to
extensions have changed. Name resolution diagnostic messages and codes
also changed, for as far as these were implemented before.

Author: jvanstraten

c/src/lib.rs

@@ -572,6 +572,39 @@ pub extern "C" fn substrait_validator_config_uri_resolver(
     true
 }
 
+/// Sets the maximum recursion depth for URI resolution, in the presence of
+/// transitive dependencies. Setting this to a negative number disables the
+/// limit, setting this to zero disables URI resolution entirely.
+///
+/// Returns whether the function was successful. If false is returned, retrieve
+/// the error message with substrait_validator_get_last_error().
+#[no_mangle]
+pub extern "C" fn substrait_validator_config_max_uri_resolution_depth(
+    config: *mut ConfigHandle,
+    max_depth: i64,
+) -> bool {
+    // Check for nulls.
+    if config.is_null() {
+        set_last_error("received null configuration handle");
+        return false;
+    }
+
+    // UNSAFE: unpack configuration handle. Assumes that the pointer was
+    // created by substrait_validator_config_new(), or behavior is undefined.
+    let config = unsafe { &mut (*config).config };
+
+    // Update configuration and return success.
+    if max_depth < 0 {
+        config.set_max_uri_resolution_depth(None);
+    } else if let Ok(max_depth) = usize::try_from(max_depth) {
+        config.set_max_uri_resolution_depth(Some(max_depth));
+    } else {
+        set_last_error("specified depth is out of range");
+        return false;
+    }
+    true
+}
+
 /// Parse/validation result handle.
 pub struct ResultHandle {
     pub result: substrait_validator::ParseResult,

proto/substrait/validator/validator.proto

@@ -57,11 +57,6 @@ message Node {
     // elsewhere in the plan.
     NodeReference node_reference = 4;
 
-    // Special case for proto_primitive for references to YAML files via a URI.
-    // If resolved, the keys in the toplevel YAML map are represented using
-    // Field messages in data.
-    YamlReference yaml_reference = 5;
-
     // This node represents a YAML map/object. The keys are represented using
     // Field messages in data.
     google.protobuf.Empty yaml_map = 6;
@@ -72,6 +67,14 @@ message Node {
 
     // This node represents a YAML primitive.
     PrimitiveData yaml_primitive = 8;
+
+    // Special case for string primitives that were interpreted and resolved as
+    // a URI. These nodes will have a single child node with path `data` that
+    // represents the parse result of the referred file.
+    string resolved_uri = 9;
+
+    // No longer used. The more generic ResolvedUri type is used instead.
+    YamlReference yaml_reference = 5 [deprecated = true];
   }
 
   // Semantic classification of this node.
@@ -178,6 +181,8 @@ message Node {
 
   // Information about a reference to a YAML file.
   message YamlReference {
+    option deprecated = true;
+
     // URI to the YAML file.
     string uri = 1;
   }

py/src/lib.rs

@@ -124,6 +124,13 @@ impl Config {
                 })
             })
     }
+
+    /// Sets the maximum recursion depth for URI resolution, in the presence of
+    /// transitive dependencies. Setting this to None disables the limit,
+    /// setting this to zero disables URI resolution entirely.
+    pub fn set_max_uri_resolution_depth(&mut self, depth: Option<usize>) {
+        self.config.set_max_uri_resolution_depth(depth);
+    }
 }
 
 /// Represents a Substrait plan parse tree, as parsed by the validator.

py/substrait_validator/__init__.py

@@ -405,6 +405,17 @@ def substrait_version() -> str:
         "application of any --override-uri options)."
     ),
 )
+@click.option(
+    "--uri-depth",
+    type=int,
+    default=None,
+    help=(
+        "Sets the maximum recursion depth for resolving transitive "
+        "dependencies. You can specify a negative number to disable "
+        "the limit, or set this to zero to disable URI resolution "
+        "entirely."
+    ),
+)
 @click.option(
     "--help-diagnostics",
     is_flag=True,
@@ -437,6 +448,7 @@ def cli(  # noqa: C901
     diagnostic_level,
     override_uri,
     use_urllib,
+    uri_depth,
     help_diagnostics,
     print_version,
     print_substrait_version,
@@ -758,6 +770,11 @@ def print_diag(diag, first_prefix="", next_prefix=""):
             fatal(e)
     if use_urllib:
         config.add_urllib_resolver()
+    if uri_depth is not None:
+        if uri_depth < 0:
+            config.set_max_uri_resolution_depth(None)
+        else:
+            config.set_max_uri_resolution_depth(uri_depth)
 
     # Run the parser/validator.
     result = plan_to_result_handle(in_plan, config)

py/tests/test_api.py

@@ -127,6 +127,10 @@ def resolver(s):
     # Disable missing root relation error, so we don't have to supply one.
     config.override_diagnostic_level(5001, "info", "info")
 
+    # Explicitly disable the resolution depth limit, to opt in to URI
+    # resolution.
+    config.set_max_uri_resolution_depth(None)
+
     # Add the resolver.
     config.add_uri_resolver(resolver)
 

py/tests/test_cli.py

@@ -252,6 +252,8 @@ def x(*args):
                 "0",
                 "info",
                 "info",  # all other diagnostics -> info
+                "--uri-depth",
+                "-1",  # opt in to URI resolution, this will be the default
                 *args
             ).exit_code
 

rs/src/export/html/mod.rs

@@ -514,11 +514,11 @@ fn format_node_tree(
             &target.path,
             None,
         ),
-        tree::NodeType::YamlReference(yaml) => {
+        tree::NodeType::ResolvedUri(uri) => {
             format!(
                 "= {} {brief} {}",
-                format_span("value", &yaml.uri),
-                format_span("type", "string, resolved to YAML")
+                format_span("value", &uri),
+                format_span("type", "string, resolved as URI")
             )
         }
         tree::NodeType::YamlMap => format!("{brief} {}", format_span("type", "YAML map")),

rs/src/export/proto.rs

@@ -182,16 +182,12 @@ impl From<&tree::NodeType> for validator::node::NodeType {
                     path: Some((&node.path).into()),
                 })
             }
-            tree::NodeType::YamlReference(info) => {
-                validator::node::NodeType::YamlReference(validator::node::YamlReference {
-                    uri: info.uri.name().unwrap_or_default().to_string(),
-                })
-            }
             tree::NodeType::YamlMap => validator::node::NodeType::YamlMap(()),
             tree::NodeType::YamlArray => validator::node::NodeType::YamlArray(()),
             tree::NodeType::YamlPrimitive(data) => {
                 validator::node::NodeType::YamlPrimitive(data.into())
             }
+            tree::NodeType::ResolvedUri(uri) => validator::node::NodeType::ResolvedUri(uri.clone()),
         }
     }
 }
@@ -292,7 +288,7 @@ impl From<&data::Class> for validator::data_type::Class {
                     validator::data_type::class::Kind::Compound(compound.into())
                 }
                 data::Class::UserDefined(user_defined) => {
-                    validator::data_type::class::Kind::UserDefinedType(user_defined.as_ref().into())
+                    validator::data_type::class::Kind::UserDefinedType(user_defined.into())
                 }
                 data::Class::Unresolved => validator::data_type::class::Kind::UnresolvedType(()),
             }),
@@ -340,10 +336,8 @@ impl From<&data::class::Compound> for i32 {
     }
 }
 
-impl From<&extension::Reference<data::class::UserDefinedDefinition>>
-    for validator::data_type::UserDefinedType
-{
-    fn from(node: &extension::Reference<data::class::UserDefinedDefinition>) -> Self {
+impl From<&extension::simple::type_class::Reference> for validator::data_type::UserDefinedType {
+    fn from(node: &extension::simple::type_class::Reference) -> Self {
         Self {
             uri: node.uri.name().unwrap_or_default().to_string(),
             name: node.name.name().unwrap_or_default().to_string(),
@@ -352,10 +346,10 @@ impl From<&extension::Reference<data::class::UserDefinedDefinition>>
     }
 }
 
-impl From<&data::class::UserDefinedDefinition>
+impl From<&extension::simple::type_class::Definition>
     for validator::data_type::user_defined_type::Definition
 {
-    fn from(node: &data::class::UserDefinedDefinition) -> Self {
+    fn from(node: &extension::simple::type_class::Definition) -> Self {
         Self {
             structure: node
                 .structure
@@ -394,24 +388,24 @@ impl From<&data::Variation> for validator::data_type::Variation {
     }
 }
 
-impl From<&data::variation::UserDefinedDefinition>
+impl From<&extension::simple::type_variation::Definition>
     for validator::data_type::user_defined_variation::Definition
 {
-    fn from(node: &data::variation::UserDefinedDefinition) -> Self {
+    fn from(node: &extension::simple::type_variation::Definition) -> Self {
         Self {
             base_type: None,
             function_behavior: (&node.function_behavior).into(),
         }
     }
 }
 
-impl From<&data::variation::FunctionBehavior> for i32 {
-    fn from(node: &data::variation::FunctionBehavior) -> Self {
+impl From<&extension::simple::type_variation::FunctionBehavior> for i32 {
+    fn from(node: &extension::simple::type_variation::FunctionBehavior) -> Self {
         match node {
-            data::variation::FunctionBehavior::Inherits => {
+            extension::simple::type_variation::FunctionBehavior::Inherits => {
                 validator::data_type::user_defined_variation::FunctionBehavior::Inherits
             }
-            data::variation::FunctionBehavior::Separate => {
+            extension::simple::type_variation::FunctionBehavior::Separate => {
                 validator::data_type::user_defined_variation::FunctionBehavior::Separate
             }
         }

rs/src/input/config.rs

@@ -39,7 +39,6 @@ fn resolve_with_curl(uri: &str) -> Result<Vec<u8>, curl::Error> {
 }
 
 /// Configuration structure.
-#[derive(Default)]
 pub struct Config {
     /// When set, do not generate warnings for unknown protobuf fields that are
     /// set to their protobuf-defined default value.
@@ -80,6 +79,26 @@ pub struct Config {
     /// error. If no downloader is specified, only file:// URLs with an
     /// absolute path are supported.
     pub uri_resolver: Option<UriResolver>,
+
+    /// Optional URI resolution depth. If specified, dependencies are only
+    /// resolved this many levels deep. Setting this to zero effectively
+    /// disables extension URI resolution altogether.
+    pub max_uri_resolution_depth: Option<usize>,
+}
+
+// TODO: enable URI resolution by default once all that works. Then this can
+// be derived again. Also still need to expose the depth option in extensions.
+impl Default for Config {
+    fn default() -> Self {
+        Self {
+            ignore_unknown_fields: Default::default(),
+            allowed_proto_any_urls: Default::default(),
+            diagnostic_level_overrides: Default::default(),
+            uri_overrides: Default::default(),
+            uri_resolver: Default::default(),
+            max_uri_resolution_depth: Some(0),
+        }
+    }
 }
 
 impl Config {
@@ -152,4 +171,11 @@ impl Config {
     pub fn add_curl_uri_resolver(&mut self) {
         self.add_uri_resolver(resolve_with_curl)
     }
+
+    /// Sets the maximum recursion depth for URI resolution, in the presence of
+    /// transitive dependencies. Setting this to None disables the limit,
+    /// setting this to zero disables URI resolution entirely.
+    pub fn set_max_uri_resolution_depth(&mut self, depth: Option<usize>) {
+        self.max_uri_resolution_depth = depth;
+    }
 }

rs/src/output/diagnostic.rs

@@ -218,28 +218,34 @@ pub enum Classification {
     #[strum(props(Description = "invalid YAML value type"))]
     YamlInvalidType = 2008,
 
+    #[strum(props(Description = "cyclic dependency"))]
+    YamlCyclicDependency = 2009,
+
     // Link resolution diagnostics (group 3).
     #[strum(props(HiddenDescription = "link resolution diagnostic"))]
     Link = 3000,
 
     #[strum(props(Description = "failed to resolve anchor"))]
     LinkMissingAnchor = 3001,
 
-    #[strum(props(Description = "failed to resolve function name"))]
-    LinkMissingFunctionName = 3002,
-
-    #[strum(props(Description = "failed to resolve type name"))]
-    LinkMissingTypeName = 3003,
-
-    #[strum(props(Description = "failed to resolve type variation name"))]
-    LinkMissingTypeVariationName = 3004,
-
     #[strum(props(HiddenDescription = "use of anchor zero"))]
     LinkAnchorZero = 3005,
 
     #[strum(props(Description = "failed to resolve type variation name & class pair"))]
     LinkMissingTypeVariationNameAndClass = 3006,
 
+    #[strum(props(Description = "unresolved name lookup"))]
+    LinkUnresolvedName = 3007,
+
+    #[strum(props(Description = "ambiguous name lookup"))]
+    LinkAmbiguousName = 3008,
+
+    #[strum(props(Description = "duplicate definition"))]
+    LinkDuplicateDefinition = 3009,
+
+    #[strum(props(HiddenDescription = "invalid compound vs. simple function name usage"))]
+    LinkCompoundVsSimpleFunctionName = 3010,
+
     // Type-related diagnostics (group 4).
     #[strum(props(HiddenDescription = "type-related diagnostics"))]
     Type = 4000,