From 770adadd16eac3fcc3e04736774dae50a5ea0a4e Mon Sep 17 00:00:00 2001 From: "gordon.hamilton" Date: Wed, 19 Aug 2026 09:26:18 -0400 Subject: [PATCH 1/4] fix: making version non-default --- API.md | 2 + GRAMMAR.md | 41 +++++++++++- src/cli.rs | 56 +++++++++++++--- src/lib.rs | 5 +- src/parser/mod.rs | 4 +- src/parser/structural.rs | 49 ++++++++++---- src/textify/foundation.rs | 11 ++++ src/textify/plan.rs | 30 ++++++--- tests/json_parsing.rs | 13 +++- tests/plan_roundtrip.rs | 132 ++++++++++++++++++++++++++++++++++---- 10 files changed, 296 insertions(+), 47 deletions(-) diff --git a/API.md b/API.md index c18c1311..a646b3b4 100644 --- a/API.md +++ b/API.md @@ -364,6 +364,7 @@ cat plan.substrait | substrait-explain convert -f text -t json > plan.json - `-i, --input ` - Input file (default: stdin) - `-o, --output ` - Output file (default: stdout) - `--show-literal-types` - Show type annotations on literals +- `--show-plan-version` - Show the plan's Substrait version - `--verbose` - Show detailed progress information #### Validate Command @@ -385,6 +386,7 @@ substrait-explain validate -i plan.substrait --verbose - `-i, --input ` - Input file (default: stdin) - `-o, --output ` - Output file (default: stdout) +- `--show-plan-version` - Show the plan's Substrait version - `--verbose` - Show detailed progress information ### Examples diff --git a/GRAMMAR.md b/GRAMMAR.md index 8e275566..43c48e27 100644 --- a/GRAMMAR.md +++ b/GRAMMAR.md @@ -116,8 +116,16 @@ The header carries the version number as `major.minor.patch` (three non-negative integers). The indented `producer:` and `git_hash:` lines are optional and may appear in either order beneath the header. -The `=== Version` section as a whole is optional; a document with no version -section is valid and denotes a plan without a declared version. +The section is optional, and a document that leaves it out gets the Substrait +version `substrait-explain` was built against, with `substrait-explain` recorded +as the producer. `null` in place of the version number means the plan has no +version at all: + +```text +=== Version null +``` + +Output leaves the section out unless `OutputOptions::show_version` asks for it. ```rust # use substrait_explain::Parser; @@ -136,6 +144,35 @@ Root[result] # assert_eq!(version.producer, "my-optimizer"); ``` +```rust +# use substrait_explain::{Parser, default_plan_version}; +# +# let null_version = r#" +=== Version null +=== Plan +Root[result] + Read[orders => quantity:i32?] +# "#; +# +# let plan = Parser::parse(null_version).unwrap(); +# assert!(plan.version.is_none()); +# +# let no_section = r#" +# === Plan +# Root[result] +# Read[orders => quantity:i32?] +# "#; +# +# let plan = Parser::parse(no_section).unwrap(); +# assert_eq!(plan.version, Some(default_plan_version())); +# +# // nothing and `null` are different inputs, and stay different. +# assert_ne!( +# Parser::parse(null_version).unwrap().version, +# Parser::parse(no_section).unwrap().version +# ); +``` + #### Extension format ```text diff --git a/src/cli.rs b/src/cli.rs index c8257006..412e6fdb 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -78,13 +78,14 @@ impl Cli { from, to, show_literal_types, + show_plan_version, verbose, } => { let reader = get_reader(input) .with_context(|| format!("Failed to open input file: {input}"))?; let writer = get_writer(output) .with_context(|| format!("Failed to create output file: {output}"))?; - let options = self.create_output_options(*show_literal_types); + let options = self.create_output_options(*show_literal_types, *show_plan_version); let from_format = self.resolve_input_format(from, input)?; let to_format = self.resolve_output_format(to, output)?; self.run_convert_with_io( @@ -101,13 +102,14 @@ impl Cli { Commands::Validate { input, output, + show_plan_version, verbose, } => { let reader = get_reader(input) .with_context(|| format!("Failed to open input file: {input}"))?; let writer = get_writer(output) .with_context(|| format!("Failed to create output file: {output}"))?; - self.run_validate_with_io(reader, writer, *verbose, registry) + self.run_validate_with_io(reader, writer, *show_plan_version, *verbose, registry) } } } @@ -126,10 +128,11 @@ impl Cli { from, to, show_literal_types, + show_plan_version, verbose, .. } => { - let options = self.create_output_options(*show_literal_types); + let options = self.create_output_options(*show_literal_types, *show_plan_version); let from_format = self.resolve_input_format(from, input)?; let to_format = self.resolve_output_format(to, output)?; self.run_convert_with_io( @@ -143,18 +146,27 @@ impl Cli { ) } - Commands::Validate { verbose, .. } => { - self.run_validate_with_io(reader, writer, *verbose, registry) - } + Commands::Validate { + show_plan_version, + verbose, + .. + } => self.run_validate_with_io(reader, writer, *show_plan_version, *verbose, registry), } } - fn create_output_options(&self, show_literal_types: bool) -> OutputOptions { + fn create_output_options( + &self, + show_literal_types: bool, + show_plan_version: bool, + ) -> OutputOptions { let mut options = OutputOptions::default(); if show_literal_types { options.literal_types = Visibility::Always; } + if show_plan_version { + options.show_version = Visibility::Always; + } options } @@ -228,6 +240,7 @@ impl Cli { &self, reader: R, writer: W, + show_plan_version: bool, verbose: bool, registry: &ExtensionRegistry, ) -> Result { @@ -235,8 +248,11 @@ impl Cli { .read_plan(reader, registry) .with_context(|| "Failed to parse input as Substrait text format")?; + // `--show-literal-types` is a convert option; validate only varies whether + // the version section is part of the round-tripped output. + let options = self.create_output_options(false, show_plan_version); let outcome = Format::Text - .write_plan(writer, &plan, &OutputOptions::default(), registry) + .write_plan(writer, &plan, &options, registry) .with_context(|| "Failed to format plan as Substrait text format")?; if verbose && matches!(outcome, Outcome::Success) { @@ -280,6 +296,9 @@ pub enum Commands { /// Show literal types (text output only) #[arg(long)] show_literal_types: bool, + /// Show the plan's Substrait version (text output only) + #[arg(long)] + show_plan_version: bool, /// Verbose output #[arg(short, long)] verbose: bool, @@ -292,6 +311,9 @@ pub enum Commands { /// Output file (use - for stdout) #[arg(short, long, default_value = "-")] output: String, + /// Show the plan's Substrait version (text output only) + #[arg(long)] + show_plan_version: bool, /// Verbose output #[arg(short, long)] verbose: bool, @@ -516,6 +538,7 @@ Root[result] from: Some(Format::Text), to: Some(Format::Text), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -542,6 +565,7 @@ Root[result] from: Some(Format::Text), to: Some(Format::Json), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -569,6 +593,7 @@ Root[result] from: Some(Format::Text), to: Some(Format::Json), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -588,6 +613,7 @@ Root[result] from: Some(Format::Json), to: Some(Format::Text), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -613,6 +639,7 @@ Root[result] from: Some(Format::Text), to: Some(Format::Protobuf), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -637,6 +664,7 @@ Root[result] command: Commands::Validate { input: String::new(), output: String::new(), + show_plan_version: false, verbose: false, }, }; @@ -660,6 +688,7 @@ Root[result] command: Commands::Validate { input: String::new(), output: String::new(), + show_plan_version: false, verbose: false, }, }; @@ -686,6 +715,7 @@ Root[result] from: Some(Format::Text), to: Some(Format::Text), show_literal_types: true, + show_plan_version: false, verbose: false, }, }; @@ -739,6 +769,7 @@ Root[result] from: None, // Auto-detect from extension to: None, // Auto-detect from extension show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -765,6 +796,7 @@ Root[result] from: None, // Should fail auto-detection to: None, show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -791,6 +823,7 @@ Root[result] from: None, to: None, // Should fail auto-detection show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -817,6 +850,7 @@ Root[result] from: Some(Format::Text), // Explicit override to: Some(Format::Text), // Explicit override show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -842,6 +876,7 @@ Root[result] from: Some(Format::Text), to: Some(Format::Protobuf), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -861,6 +896,7 @@ Root[result] from: Some(Format::Protobuf), to: Some(Format::Text), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -952,6 +988,7 @@ Root[val] from: Some(Format::Text), to: Some(Format::Text), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -975,6 +1012,7 @@ Root[val] from: Some(Format::Text), to: Some(Format::Json), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; @@ -995,6 +1033,7 @@ Root[val] command: Commands::Validate { input: String::new(), output: String::new(), + show_plan_version: false, verbose: false, }, }; @@ -1018,6 +1057,7 @@ Root[val] from: Some(Format::Text), to: Some(Format::Text), show_literal_types: false, + show_plan_version: false, verbose: false, }, }; diff --git a/src/lib.rs b/src/lib.rs index 1dc70596..a3083905 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,7 +24,7 @@ pub mod json; // Re-export commonly used types for easier access pub use parser::{ ExpectedExtensionLine, ExtensionParseError, MessageParseError, ParseContext, ParseError, - ParseResult, Parser, + ParseResult, Parser, default_plan_version, }; use substrait::proto::Plan; use textify::foundation::ErrorQueue; @@ -41,6 +41,9 @@ use textify::plan::PlanWriter; /// - A plan section starting with "=== Plan" /// - Indented relation definitions /// +/// A document with no version section gets [`default_plan_version`]; +/// `=== Version null` gets no version. +/// /// # Example /// ```rust /// use substrait_explain::parse; diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 72d62a6b..dba930da 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -15,5 +15,5 @@ pub(crate) use common::{ pub use errors::{ParseContext, ParseError, ParseResult}; pub use extensions::{ExpectedExtensionLine, ExtensionParseError}; pub(crate) use relations::RelationParsePair; -pub use structural::Parser; -pub(crate) use structural::{PLAN_HEADER, VERSION_HEADER}; +pub(crate) use structural::{PLAN_HEADER, VERSION_HEADER, VERSION_NULL}; +pub use structural::{Parser, default_plan_version}; diff --git a/src/parser/structural.rs b/src/parser/structural.rs index 1fc989a3..7b213fd5 100644 --- a/src/parser/structural.rs +++ b/src/parser/structural.rs @@ -13,6 +13,7 @@ use substrait::proto::{ AggregateRel, CrossRel, FetchRel, FilterRel, JoinRel, Plan, PlanRel, ProjectRel, ReadRel, Rel, RelRoot, SortRel, Version, plan_rel, }; +use substrait::version::version_with_producer; use crate::extensions::any::Any; use crate::extensions::{AddendumKind, ExtensionRegistry, SimpleExtensions, simple}; @@ -30,6 +31,8 @@ use crate::parser::{ErrorKind, ExpressionParser, RelationParsePair, Rule, unwrap pub const PLAN_HEADER: &str = "=== Plan"; pub const VERSION_HEADER: &str = "=== Version"; +/// Marks a plan with no version at all: `=== Version null`. +pub const VERSION_NULL: &str = "null"; /// Represents an input line, trimmed of leading two-space indents and final /// whitespace. Contains the number of indents and the trimmed line. @@ -164,6 +167,14 @@ impl<'a> LineNode<'a> { } } +/// The [`Version`] a plan gets when the text is empty. +/// +/// Substrait's `plan.proto` documents `Plan.version` as "optional up to 0.17.0, +/// required for later versions". +pub fn default_plan_version() -> Version { + version_with_producer(concat!("substrait-explain ", env!("CARGO_PKG_VERSION"))) +} + #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum State { // The initial state, before we have parsed any lines. @@ -895,7 +906,8 @@ pub struct Parser<'a> { /// [`next_chunk`](Self::next_chunk). `None` before parsing starts and once /// the input is exhausted. cursor: Option>, - /// The plan version from an optional `=== Version` section + /// The plan version. Starts as [`default_plan_version`], which a + /// `=== Version` section overrides and `=== Version null` clears. version: Option, extension_parser: ExtensionParser, extension_registry: ExtensionRegistry, @@ -917,6 +929,9 @@ impl<'a> Parser<'a> { /// - A plan section starting with "=== Plan" /// - Indented relation definitions /// + /// A document with no version(`=== Version null`) section gets + /// [`default_plan_version`](crate::default_plan_version); + /// /// # Examples /// /// Simple parsing: @@ -946,7 +961,7 @@ impl<'a> Parser<'a> { line_no: 1, state: State::Initial, cursor: None, - version: None, + version: Some(default_plan_version()), extension_parser: ExtensionParser::default(), extension_registry: ExtensionRegistry::new(), relation_parser: RelationParser::default(), @@ -1097,7 +1112,7 @@ impl<'a> Parser<'a> { if let Some(rest) = line.strip_prefix(VERSION_HEADER) && (rest.is_empty() || rest.starts_with(' ')) { - self.version = Some(self.parse_version_header(line)?); + self.version = self.parse_version_header(line)?; return Ok(State::Version); } @@ -1111,15 +1126,20 @@ impl<'a> Parser<'a> { )) } - /// Parse the `=== Version ..` header line and store - /// the resulting [`Version`], leaving `producer` / `git_hash` empty - fn parse_version_header(&self, line: &str) -> Result { + /// Parse the `=== Version ..` header line and return + /// the resulting [`Version`], leaving `producer` / `git_hash` empty, or + /// `None` for `=== Version null` + fn parse_version_header(&self, line: &str) -> Result, ParseError> { let ctx = || ParseContext::new(self.line_no, line.to_string()); let rest = line .strip_prefix(VERSION_HEADER) .expect("version header prefix checked by caller") .trim(); + if rest == VERSION_NULL { + return Ok(None); + } + let mut numbers = rest.split('.'); let mut next_number = |field: &str| -> Result { let part = numbers.next().filter(|p| !p.is_empty()).ok_or_else(|| { @@ -1148,12 +1168,12 @@ impl<'a> Parser<'a> { )); } - Ok(Version { + Ok(Some(Version { major_number, minor_number, patch_number, ..Default::default() - }) + })) } /// Parses a single line from the version section: an indented @@ -1184,10 +1204,15 @@ impl<'a> Parser<'a> { ) })?; let value = value.trim().to_string(); - let version = self - .version - .as_mut() - .expect("version is set on entry to the version section"); + let Some(version) = self.version.as_mut() else { + return Err(ParseError::ValidationError( + ctx(), + format!( + "unexpected detail line {line:?} under \ + '{VERSION_HEADER} {VERSION_NULL}'; a null version has no fields" + ), + )); + }; match key.trim() { "producer" => version.producer = value, "git_hash" => version.git_hash = value, diff --git a/src/textify/foundation.rs b/src/textify/foundation.rs index 03aa4fe3..b4000937 100644 --- a/src/textify/foundation.rs +++ b/src/textify/foundation.rs @@ -27,6 +27,15 @@ pub enum Visibility { /// OutputOptions holds the options for textifying a Substrait type. #[derive(Debug, Clone)] pub struct OutputOptions { + /// Show the `=== Version` section. + /// + /// If `Required`, the section is shown only when the plan carries a version + /// that is not entirely empty, and empty `producer` / `git_hash` fields are + /// left out. + /// + /// If `Always`, the section is shown even when the plan has no version - as + /// `=== Version null`. + pub show_version: Visibility, /// Show the extension URNs in the output. pub show_extension_urns: bool, /// Show the extensions in the output. By default, simple extensions are @@ -61,6 +70,7 @@ pub struct OutputOptions { impl Default for OutputOptions { fn default() -> Self { Self { + show_version: Visibility::Never, show_extension_urns: false, show_simple_extensions: false, show_simple_extension_anchors: Visibility::Required, @@ -80,6 +90,7 @@ impl OutputOptions { /// reconstructing a plan. pub fn verbose() -> Self { Self { + show_version: Visibility::Always, show_extension_urns: true, show_simple_extensions: true, show_simple_extension_anchors: Visibility::Always, diff --git a/src/textify/plan.rs b/src/textify/plan.rs index 36d66f07..472812ab 100644 --- a/src/textify/plan.rs +++ b/src/textify/plan.rs @@ -4,9 +4,9 @@ use substrait::proto; use super::Textify; use crate::extensions::{ExtensionRegistry, SimpleExtensions}; -use crate::parser::{PLAN_HEADER, VERSION_HEADER}; +use crate::parser::{PLAN_HEADER, VERSION_HEADER, VERSION_NULL}; use crate::textify::foundation::ErrorAccumulator; -use crate::textify::{OutputOptions, ScopedContext}; +use crate::textify::{OutputOptions, ScopedContext, Visibility}; #[derive(Debug, Clone)] pub(crate) struct PlanWriter<'a, E: ErrorAccumulator + Default> { @@ -56,15 +56,27 @@ impl<'a, E: ErrorAccumulator + Default + Clone> PlanWriter<'a, E> { ) } - /// Write the `=== Version` section. Emits nothing unless the plan - /// carries a version that is not entirely empty + /// Write the `=== Version` section, as directed by + /// [`OutputOptions::show_version`]. + /// + /// `Never` writes nothing. `Required` writes the section only when the plan + /// carries a version that is not entirely empty. `Always` writes it + /// unconditionally, as `=== Version null` when the plan has no version. pub(crate) fn write_version(&self, w: &mut impl fmt::Write) -> fmt::Result { - let Some(version) = self.version else { - return Ok(()); + let always = match self.options.show_version { + Visibility::Never => return Ok(()), + Visibility::Required => false, + Visibility::Always => true, + }; + + let version = match self.version { + // Record the absence explicitly; otherwise reading this output back + // in would substitute a default version for an unset one. + None if always => return writeln!(w, "{VERSION_HEADER} {VERSION_NULL}"), + None => return Ok(()), + Some(v) if !always && v == &proto::Version::default() => return Ok(()), + Some(v) => v, }; - if version == &proto::Version::default() { - return Ok(()); - } writeln!( w, diff --git a/tests/json_parsing.rs b/tests/json_parsing.rs index 55da726d..cc870d11 100644 --- a/tests/json_parsing.rs +++ b/tests/json_parsing.rs @@ -14,7 +14,7 @@ use substrait_explain::extensions::{ ExtensionRegistry, }; use substrait_explain::json::{build_descriptor_pool, parse_json}; -use substrait_explain::{OutputOptions, Parser, format_with_registry}; +use substrait_explain::{OutputOptions, Parser, default_plan_version, format_with_registry}; mod example_protos { #![allow( @@ -102,6 +102,16 @@ fn test_text_path() { .parse_plan(PLAN_TEXT) .expect("failed to parse text plan"); + // The text fixture has no `=== Version` section, so parsing fills in the + // Substrait version substrait-explain is built against. Assert that here and + // drop it before the comparison; baking the number into the fixture would + // mean re-generating it on every substrait dependency bump. + assert_eq!(plan.version, Some(default_plan_version())); + let plan = proto::Plan { + version: None, + ..plan + }; + let serialized = serde_json::to_string_pretty(&plan).expect("failed to serialize"); assert_eq!( serialized.trim(), @@ -193,6 +203,7 @@ fn make_cli(from: Format) -> Cli { from: Some(from), to: Some(Format::Text), show_literal_types: false, + show_plan_version: false, verbose: false, }, } diff --git a/tests/plan_roundtrip.rs b/tests/plan_roundtrip.rs index f038b8be..a6730f8b 100644 --- a/tests/plan_roundtrip.rs +++ b/tests/plan_roundtrip.rs @@ -8,7 +8,11 @@ mod common; use common::{assert_roundtrip_canonical, roundtrip_plan}; -use substrait_explain::{ParseError, Parser}; +use substrait::proto::Version; +use substrait_explain::{ + OutputOptions, ParseError, Parser, Visibility, default_plan_version, format, + format_with_options, +}; #[test] fn test_simple_plan_roundtrip() { @@ -1121,6 +1125,23 @@ Root[sum] roundtrip_plan(plan); } +/// Round-trip `input` with the version section at the given visibility, leaving +/// every other option at its default. +fn roundtrip_showing_version(input: &str, show_version: Visibility) { + let plan = Parser::parse(input).expect("parse failed"); + let options = OutputOptions { + show_version, + ..OutputOptions::default() + }; + let (text, errors) = format_with_options(&plan, &options); + + assert!( + errors.is_empty(), + "unexpected formatting errors: {errors:?}" + ); + assert_eq!(text.trim(), input.trim()); +} + #[test] fn test_version_semver_only_roundtrip() { let plan = r#"=== Version 0.55.0 @@ -1128,7 +1149,7 @@ fn test_version_semver_only_roundtrip() { Root[a] Read[t => a:i64]"#; - roundtrip_plan(plan); + roundtrip_showing_version(plan, Visibility::Required); let parsed = Parser::parse(plan).unwrap(); let version = parsed.version.expect("version should be set"); @@ -1157,7 +1178,7 @@ Root[sum] Project[add($0, $1):i64] Read[t => a:i64, b:i64]"#; - roundtrip_plan(plan); + roundtrip_showing_version(plan, Visibility::Required); let version = Parser::parse(plan).unwrap().version.unwrap(); assert_eq!( @@ -1182,26 +1203,58 @@ fn test_version_git_hash_only_roundtrip() { Root[a] Read[t => a:i64]"#; - roundtrip_plan(plan); + roundtrip_showing_version(plan, Visibility::Required); } -/// A plan with no version section parses to `version: None` and emits no -/// `=== Version` line. +/// A document with no version section gets the built-in version, which default +/// output leaves out - so the text round-trips unchanged - and verbose output +/// shows. #[test] fn test_version_absent() { let plan = r#"=== Plan Root[a] Read[t => a:i64]"#; + roundtrip_plan(plan); + let parsed = Parser::parse(plan).unwrap(); - assert!(parsed.version.is_none()); + assert_eq!(parsed.version, Some(default_plan_version())); - let (text, errors) = substrait_explain::format(&parsed); + let (text, errors) = format(&parsed); assert!(errors.is_empty()); assert!( !text.contains("=== Version"), "no version section expected, got:\n{text}" ); + + let (verbose_text, errors) = format_with_options(&parsed, &OutputOptions::verbose()); + assert!(errors.is_empty()); + let built_in = default_plan_version(); + let expected = format!( + "=== Version {}.{}.{}", + built_in.major_number, built_in.minor_number, built_in.patch_number + ); + assert!( + verbose_text.starts_with(&expected), + "expected verbose output to start with {expected:?}, got:\n{verbose_text}" + ); +} + +/// Default output drops the version section, so a versioned plan and an +/// unversioned one print the same way. The version is not recoverable from that +/// output; the plan parsed back from it gets the built-in version instead. +#[test] +fn test_version_dropped_by_default() { + let canonical = r#"=== Plan +Root[a] + Read[t => a:i64]"#; + let versioned = r#"=== Version 0.52.0 + producer: some-optimizer +=== Plan +Root[a] + Read[t => a:i64]"#; + + assert_roundtrip_canonical(canonical, versioned); } /// An all-default `Version` (0.0.0 with no producer/git_hash) is treated as @@ -1215,10 +1268,15 @@ Root[a] // The parser preserves the (empty) version it was given... let parsed = Parser::parse(plan).unwrap(); - assert!(parsed.version.is_some()); - - // ...but the formatter suppresses an all-default version. - let (text, errors) = substrait_explain::format(&parsed); + assert_eq!(parsed.version, Some(Version::default())); + + // ...but `Required` suppresses an all-default version, as does the default + // `Never`. + let options = OutputOptions { + show_version: Visibility::Required, + ..OutputOptions::default() + }; + let (text, errors) = format_with_options(&parsed, &options); assert!(errors.is_empty()); assert!( !text.contains("=== Version"), @@ -1226,6 +1284,56 @@ Root[a] ); } +/// `Always` writes the header even for an all-default version, which `Required` +/// hides. +#[test] +fn test_version_all_zero_shown_when_always() { + let plan = r#"=== Version 0.0.0 +=== Plan +Root[a] + Read[t => a:i64]"#; + + roundtrip_showing_version(plan, Visibility::Always); + + let parsed = Parser::parse(plan).unwrap(); + assert_eq!(parsed.version, Some(Version::default())); +} + +/// `=== Version null` says the plan has no version. +#[test] +fn test_version_null_roundtrip() { + let plan = r#"=== Version null +=== Plan +Root[a] + Read[t => a:i64]"#; + + roundtrip_showing_version(plan, Visibility::Always); + + let parsed = Parser::parse(plan).unwrap(); + assert!( + parsed.version.is_none(), + "expected no version, got {:?}", + parsed.version + ); +} + +/// A null version has no fields, so detail lines under it are a contradiction. +#[test] +fn test_version_null_with_detail_errors() { + let plan = r#"=== Version null + producer: some-optimizer +=== Plan +Root[a] + Read[t => a:i64]"#; + + let err = Parser::parse(plan).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("null version has no fields"), + "expected a null-version error, got: {msg}" + ); +} + /// An unparseable version number is a hard error (structural, not lenient). #[test] fn test_version_invalid_number_errors() { From 8b2e86baae91050802b1795adde395d724f8f398 Mon Sep 17 00:00:00 2001 From: "gordon.hamilton" Date: Fri, 21 Aug 2026 11:06:04 -0400 Subject: [PATCH 2/4] fix: according to review comments --- API.md | 8 +- GRAMMAR.md | 24 +----- src/cli.rs | 112 ++++++++----------------- src/lib.rs | 3 - src/parser/structural.rs | 7 +- src/textify/foundation.rs | 13 +-- src/textify/plan.rs | 19 +++-- tests/common/mod.rs | 8 +- tests/json_parsing.rs | 15 +--- tests/json_parsing/plan.substrait | 1 + tests/json_parsing/plan_pbjson.json | 3 + tests/json_parsing/plan_protojson.json | 3 + tests/plan_roundtrip.rs | 66 ++++++--------- 13 files changed, 108 insertions(+), 174 deletions(-) diff --git a/API.md b/API.md index a646b3b4..2b2a8c4b 100644 --- a/API.md +++ b/API.md @@ -363,8 +363,7 @@ cat plan.substrait | substrait-explain convert -f text -t json > plan.json - `-t, --to ` - Output format (default: text) - `-i, --input ` - Input file (default: stdin) - `-o, --output ` - Output file (default: stdout) -- `--show-literal-types` - Show type annotations on literals -- `--show-plan-version` - Show the plan's Substrait version +- `--detailed` - Show more detail on plans, including type annotations and plan version - `--verbose` - Show detailed progress information #### Validate Command @@ -386,7 +385,6 @@ substrait-explain validate -i plan.substrait --verbose - `-i, --input ` - Input file (default: stdin) - `-o, --output ` - Output file (default: stdout) -- `--show-plan-version` - Show the plan's Substrait version - `--verbose` - Show detailed progress information ### Examples @@ -396,8 +394,8 @@ substrait-explain validate -i plan.substrait --verbose substrait-explain validate -i example-plans/basic.substrait substrait-explain validate -i example-plans/simple.substrait -# Convert with verbose output and type information -substrait-explain convert -f text -t json --show-literal-types --verbose -i example-plans/basic.substrait +# Convert with verbose output and full plan detail +substrait-explain convert -f text -t json --detailed --verbose -i example-plans/basic.substrait # Roundtrip test: text → protobuf → text substrait-explain convert -f text -t protobuf -i plan.substrait -o plan.pb diff --git a/GRAMMAR.md b/GRAMMAR.md index 43c48e27..06a79d9c 100644 --- a/GRAMMAR.md +++ b/GRAMMAR.md @@ -125,8 +125,6 @@ version at all: === Version null ``` -Output leaves the section out unless `OutputOptions::show_version` asks for it. - ```rust # use substrait_explain::Parser; # @@ -145,32 +143,16 @@ Root[result] ``` ```rust -# use substrait_explain::{Parser, default_plan_version}; +# use substrait_explain::Parser; # -# let null_version = r#" +# let plan_text = r#" === Version null === Plan Root[result] Read[orders => quantity:i32?] # "#; # -# let plan = Parser::parse(null_version).unwrap(); -# assert!(plan.version.is_none()); -# -# let no_section = r#" -# === Plan -# Root[result] -# Read[orders => quantity:i32?] -# "#; -# -# let plan = Parser::parse(no_section).unwrap(); -# assert_eq!(plan.version, Some(default_plan_version())); -# -# // nothing and `null` are different inputs, and stay different. -# assert_ne!( -# Parser::parse(null_version).unwrap().version, -# Parser::parse(no_section).unwrap().version -# ); +# Parser::parse(plan_text).unwrap(); ``` #### Extension format diff --git a/src/cli.rs b/src/cli.rs index 412e6fdb..69b531b4 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -10,9 +10,7 @@ use prost::Message; use substrait::proto::Plan; use crate::extensions::ExtensionRegistry; -use crate::{ - FormatError, OutputOptions, Visibility, format_with_registry, json, parse_with_registry, -}; +use crate::{FormatError, OutputOptions, format_with_registry, json, parse_with_registry}; /// The outcome of a CLI operation. /// @@ -77,15 +75,14 @@ impl Cli { output, from, to, - show_literal_types, - show_plan_version, + detailed, verbose, } => { let reader = get_reader(input) .with_context(|| format!("Failed to open input file: {input}"))?; let writer = get_writer(output) .with_context(|| format!("Failed to create output file: {output}"))?; - let options = self.create_output_options(*show_literal_types, *show_plan_version); + let options = self.create_output_options(*detailed); let from_format = self.resolve_input_format(from, input)?; let to_format = self.resolve_output_format(to, output)?; self.run_convert_with_io( @@ -102,14 +99,13 @@ impl Cli { Commands::Validate { input, output, - show_plan_version, verbose, } => { let reader = get_reader(input) .with_context(|| format!("Failed to open input file: {input}"))?; let writer = get_writer(output) .with_context(|| format!("Failed to create output file: {output}"))?; - self.run_validate_with_io(reader, writer, *show_plan_version, *verbose, registry) + self.run_validate_with_io(reader, writer, *verbose, registry) } } } @@ -127,12 +123,11 @@ impl Cli { output, from, to, - show_literal_types, - show_plan_version, + detailed, verbose, .. } => { - let options = self.create_output_options(*show_literal_types, *show_plan_version); + let options = self.create_output_options(*detailed); let from_format = self.resolve_input_format(from, input)?; let to_format = self.resolve_output_format(to, output)?; self.run_convert_with_io( @@ -146,29 +141,18 @@ impl Cli { ) } - Commands::Validate { - show_plan_version, - verbose, - .. - } => self.run_validate_with_io(reader, writer, *show_plan_version, *verbose, registry), + Commands::Validate { verbose, .. } => { + self.run_validate_with_io(reader, writer, *verbose, registry) + } } } - fn create_output_options( - &self, - show_literal_types: bool, - show_plan_version: bool, - ) -> OutputOptions { - let mut options = OutputOptions::default(); - - if show_literal_types { - options.literal_types = Visibility::Always; + fn create_output_options(&self, detailed: bool) -> OutputOptions { + if detailed { + OutputOptions::verbose() + } else { + OutputOptions::default() } - if show_plan_version { - options.show_version = Visibility::Always; - } - - options } fn resolve_input_format(&self, format: &Option, input_path: &str) -> Result { @@ -240,7 +224,6 @@ impl Cli { &self, reader: R, writer: W, - show_plan_version: bool, verbose: bool, registry: &ExtensionRegistry, ) -> Result { @@ -248,11 +231,8 @@ impl Cli { .read_plan(reader, registry) .with_context(|| "Failed to parse input as Substrait text format")?; - // `--show-literal-types` is a convert option; validate only varies whether - // the version section is part of the round-tripped output. - let options = self.create_output_options(false, show_plan_version); let outcome = Format::Text - .write_plan(writer, &plan, &options, registry) + .write_plan(writer, &plan, &OutputOptions::default(), registry) .with_context(|| "Failed to format plan as Substrait text format")?; if verbose && matches!(outcome, Outcome::Success) { @@ -293,12 +273,9 @@ pub enum Commands { /// Output format: text, json, yaml, protobuf/proto/pb (auto-detected from file extension if not specified) #[arg(short = 't', long)] to: Option, - /// Show literal types (text output only) - #[arg(long)] - show_literal_types: bool, - /// Show the plan's Substrait version (text output only) + /// Show more detail on plans, including type annotations and plan version #[arg(long)] - show_plan_version: bool, + detailed: bool, /// Verbose output #[arg(short, long)] verbose: bool, @@ -311,9 +288,6 @@ pub enum Commands { /// Output file (use - for stdout) #[arg(short, long, default_value = "-")] output: String, - /// Show the plan's Substrait version (text output only) - #[arg(long)] - show_plan_version: bool, /// Verbose output #[arg(short, long)] verbose: bool, @@ -537,8 +511,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -564,8 +537,7 @@ Root[result] output: "output.json".to_string(), from: Some(Format::Text), to: Some(Format::Json), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -592,8 +564,7 @@ Root[result] output: "output.json".to_string(), from: Some(Format::Text), to: Some(Format::Json), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -612,8 +583,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Json), to: Some(Format::Text), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -638,8 +608,7 @@ Root[result] output: "output.pb".to_string(), from: Some(Format::Text), to: Some(Format::Protobuf), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -664,7 +633,6 @@ Root[result] command: Commands::Validate { input: String::new(), output: String::new(), - show_plan_version: false, verbose: false, }, }; @@ -688,7 +656,6 @@ Root[result] command: Commands::Validate { input: String::new(), output: String::new(), - show_plan_version: false, verbose: false, }, }; @@ -714,8 +681,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: true, - show_plan_version: false, + detailed: true, verbose: false, }, }; @@ -723,9 +689,13 @@ Root[result] cli.run_with_io(input, &mut output, &ExtensionRegistry::default()) .unwrap(); + // `--detailed` adds what the default output leaves out: the version + // section, read types, and nullability. let output_content = String::from_utf8(output).unwrap(); + assert!(output_content.contains("=== Version")); assert!(output_content.contains("=== Plan")); assert!(output_content.contains("Root[result]")); + assert!(output_content.contains("Read[data => a:i64, b:string]")); } #[test] @@ -768,8 +738,7 @@ Root[result] output: "output.json".to_string(), from: None, // Auto-detect from extension to: None, // Auto-detect from extension - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -795,8 +764,7 @@ Root[result] output: "output.json".to_string(), from: None, // Should fail auto-detection to: None, - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -822,8 +790,7 @@ Root[result] output: "output.unknown".to_string(), from: None, to: None, // Should fail auto-detection - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -849,8 +816,7 @@ Root[result] output: "output.pb".to_string(), // Would auto-detect as Protobuf from: Some(Format::Text), // Explicit override to: Some(Format::Text), // Explicit override - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -875,8 +841,7 @@ Root[result] output: "output.pb".to_string(), from: Some(Format::Text), to: Some(Format::Protobuf), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -895,8 +860,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Protobuf), to: Some(Format::Text), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -987,8 +951,7 @@ Root[val] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -1011,8 +974,7 @@ Root[val] output: "output.json".to_string(), from: Some(Format::Text), to: Some(Format::Json), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; @@ -1033,7 +995,6 @@ Root[val] command: Commands::Validate { input: String::new(), output: String::new(), - show_plan_version: false, verbose: false, }, }; @@ -1056,8 +1017,7 @@ Root[val] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, }; diff --git a/src/lib.rs b/src/lib.rs index a3083905..1e626ca3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -41,9 +41,6 @@ use textify::plan::PlanWriter; /// - A plan section starting with "=== Plan" /// - Indented relation definitions /// -/// A document with no version section gets [`default_plan_version`]; -/// `=== Version null` gets no version. -/// /// # Example /// ```rust /// use substrait_explain::parse; diff --git a/src/parser/structural.rs b/src/parser/structural.rs index 7b213fd5..b7f028a2 100644 --- a/src/parser/structural.rs +++ b/src/parser/structural.rs @@ -929,9 +929,6 @@ impl<'a> Parser<'a> { /// - A plan section starting with "=== Plan" /// - Indented relation definitions /// - /// A document with no version(`=== Version null`) section gets - /// [`default_plan_version`](crate::default_plan_version); - /// /// # Examples /// /// Simple parsing: @@ -961,6 +958,10 @@ impl<'a> Parser<'a> { line_no: 1, state: State::Initial, cursor: None, + // We set the version to our substrait-explain default. + // An explicitly set version (either null or with a + // version number / attributes) will override this. + // `None` here means 'explicitly unset'. version: Some(default_plan_version()), extension_parser: ExtensionParser::default(), extension_registry: ExtensionRegistry::new(), diff --git a/src/textify/foundation.rs b/src/textify/foundation.rs index b4000937..b565f1f3 100644 --- a/src/textify/foundation.rs +++ b/src/textify/foundation.rs @@ -29,12 +29,13 @@ pub enum Visibility { pub struct OutputOptions { /// Show the `=== Version` section. /// - /// If `Required`, the section is shown only when the plan carries a version - /// that is not entirely empty, and empty `producer` / `git_hash` fields are - /// left out. + /// If `Never`, the section is left out entirely. /// - /// If `Always`, the section is shown even when the plan has no version - as - /// `=== Version null`. + /// If `Required` - the default - the plan's version is shown, unless it is + /// empty or the [`default_plan_version`](crate::default_plan_version)(add by the parser). + /// + /// If `Always`, the section is shown even when the plan has no version at + /// all, as `=== Version null`. pub show_version: Visibility, /// Show the extension URNs in the output. pub show_extension_urns: bool, @@ -70,7 +71,7 @@ pub struct OutputOptions { impl Default for OutputOptions { fn default() -> Self { Self { - show_version: Visibility::Never, + show_version: Visibility::Required, show_extension_urns: false, show_simple_extensions: false, show_simple_extension_anchors: Visibility::Required, diff --git a/src/textify/plan.rs b/src/textify/plan.rs index 472812ab..d2c17782 100644 --- a/src/textify/plan.rs +++ b/src/textify/plan.rs @@ -4,7 +4,7 @@ use substrait::proto; use super::Textify; use crate::extensions::{ExtensionRegistry, SimpleExtensions}; -use crate::parser::{PLAN_HEADER, VERSION_HEADER, VERSION_NULL}; +use crate::parser::{PLAN_HEADER, VERSION_HEADER, VERSION_NULL, default_plan_version}; use crate::textify::foundation::ErrorAccumulator; use crate::textify::{OutputOptions, ScopedContext, Visibility}; @@ -57,11 +57,14 @@ impl<'a, E: ErrorAccumulator + Default + Clone> PlanWriter<'a, E> { } /// Write the `=== Version` section, as directed by - /// [`OutputOptions::show_version`]. + /// [`OutputOptions::show_version`]: /// - /// `Never` writes nothing. `Required` writes the section only when the plan - /// carries a version that is not entirely empty. `Always` writes it - /// unconditionally, as `=== Version null` when the plan has no version. + /// - [`Never`](Visibility::Never): write nothing. + /// - [`Required`](Visibility::Required): write the plan's version, unless it + /// is empty or the [`default_plan_version`] the parser adds when the text + /// has no version section. + /// - [`Always`](Visibility::Always): write the section even when the plan has + /// no version at all, as `=== Version null`. pub(crate) fn write_version(&self, w: &mut impl fmt::Write) -> fmt::Result { let always = match self.options.show_version { Visibility::Never => return Ok(()), @@ -74,7 +77,11 @@ impl<'a, E: ErrorAccumulator + Default + Clone> PlanWriter<'a, E> { // in would substitute a default version for an unset one. None if always => return writeln!(w, "{VERSION_HEADER} {VERSION_NULL}"), None => return Ok(()), - Some(v) if !always && v == &proto::Version::default() => return Ok(()), + Some(v) + if !always && (v == &proto::Version::default() || v == &default_plan_version()) => + { + return Ok(()); + } Some(v) => v, }; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index a12783df..d74d5e10 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -2,17 +2,21 @@ use substrait::proto; use substrait::proto::{plan_rel, rel}; -use substrait_explain::{Parser, format, parse}; +use substrait_explain::{OutputOptions, Parser, format, format_with_options, parse}; /// Roundtrip a plan and verify that the output is the same as the input, after /// being parsed to a Substrait plan and then back to text. pub fn roundtrip_plan(input: &str) { + roundtrip_plan_with_options(input, &OutputOptions::default()); +} + +pub fn roundtrip_plan_with_options(input: &str, options: &OutputOptions) { let plan = Parser::parse(input).unwrap_or_else(|e| { println!("Error parsing plan:\n{e}"); panic!("{e}"); }); - let (actual, errors) = format(&plan); + let (actual, errors) = format_with_options(&plan, options); if !errors.is_empty() { println!("Formatting errors:"); diff --git a/tests/json_parsing.rs b/tests/json_parsing.rs index cc870d11..d490961c 100644 --- a/tests/json_parsing.rs +++ b/tests/json_parsing.rs @@ -14,7 +14,7 @@ use substrait_explain::extensions::{ ExtensionRegistry, }; use substrait_explain::json::{build_descriptor_pool, parse_json}; -use substrait_explain::{OutputOptions, Parser, default_plan_version, format_with_registry}; +use substrait_explain::{OutputOptions, Parser, format_with_registry}; mod example_protos { #![allow( @@ -102,16 +102,6 @@ fn test_text_path() { .parse_plan(PLAN_TEXT) .expect("failed to parse text plan"); - // The text fixture has no `=== Version` section, so parsing fills in the - // Substrait version substrait-explain is built against. Assert that here and - // drop it before the comparison; baking the number into the fixture would - // mean re-generating it on every substrait dependency bump. - assert_eq!(plan.version, Some(default_plan_version())); - let plan = proto::Plan { - version: None, - ..plan - }; - let serialized = serde_json::to_string_pretty(&plan).expect("failed to serialize"); assert_eq!( serialized.trim(), @@ -202,8 +192,7 @@ fn make_cli(from: Format) -> Cli { output: "-".to_string(), from: Some(from), to: Some(Format::Text), - show_literal_types: false, - show_plan_version: false, + detailed: false, verbose: false, }, } diff --git a/tests/json_parsing/plan.substrait b/tests/json_parsing/plan.substrait index 6fb25923..6c897544 100644 --- a/tests/json_parsing/plan.substrait +++ b/tests/json_parsing/plan.substrait @@ -1,3 +1,4 @@ +=== Version 0.55.0 === Plan Root[customer_id, amount] ExtensionLeaf:ParquetScan[path='data/sales.parquet', batch_size=2048 => customer_id:i64, amount:fp64] diff --git a/tests/json_parsing/plan_pbjson.json b/tests/json_parsing/plan_pbjson.json index bf385369..9c5b62e9 100644 --- a/tests/json_parsing/plan_pbjson.json +++ b/tests/json_parsing/plan_pbjson.json @@ -1,4 +1,7 @@ { + "version": { + "minorNumber": 55 + }, "relations": [ { "root": { diff --git a/tests/json_parsing/plan_protojson.json b/tests/json_parsing/plan_protojson.json index 97424a43..4134cb0f 100644 --- a/tests/json_parsing/plan_protojson.json +++ b/tests/json_parsing/plan_protojson.json @@ -1,4 +1,7 @@ { + "version": { + "minorNumber": 55 + }, "relations": [ { "root": { diff --git a/tests/plan_roundtrip.rs b/tests/plan_roundtrip.rs index a6730f8b..4b19f806 100644 --- a/tests/plan_roundtrip.rs +++ b/tests/plan_roundtrip.rs @@ -7,7 +7,7 @@ mod common; -use common::{assert_roundtrip_canonical, roundtrip_plan}; +use common::{assert_roundtrip_canonical, roundtrip_plan, roundtrip_plan_with_options}; use substrait::proto::Version; use substrait_explain::{ OutputOptions, ParseError, Parser, Visibility, default_plan_version, format, @@ -1125,23 +1125,6 @@ Root[sum] roundtrip_plan(plan); } -/// Round-trip `input` with the version section at the given visibility, leaving -/// every other option at its default. -fn roundtrip_showing_version(input: &str, show_version: Visibility) { - let plan = Parser::parse(input).expect("parse failed"); - let options = OutputOptions { - show_version, - ..OutputOptions::default() - }; - let (text, errors) = format_with_options(&plan, &options); - - assert!( - errors.is_empty(), - "unexpected formatting errors: {errors:?}" - ); - assert_eq!(text.trim(), input.trim()); -} - #[test] fn test_version_semver_only_roundtrip() { let plan = r#"=== Version 0.55.0 @@ -1149,7 +1132,7 @@ fn test_version_semver_only_roundtrip() { Root[a] Read[t => a:i64]"#; - roundtrip_showing_version(plan, Visibility::Required); + roundtrip_plan(plan); let parsed = Parser::parse(plan).unwrap(); let version = parsed.version.expect("version should be set"); @@ -1178,7 +1161,7 @@ Root[sum] Project[add($0, $1):i64] Read[t => a:i64, b:i64]"#; - roundtrip_showing_version(plan, Visibility::Required); + roundtrip_plan(plan); let version = Parser::parse(plan).unwrap().version.unwrap(); assert_eq!( @@ -1203,7 +1186,7 @@ fn test_version_git_hash_only_roundtrip() { Root[a] Read[t => a:i64]"#; - roundtrip_showing_version(plan, Visibility::Required); + roundtrip_plan(plan); } /// A document with no version section gets the built-in version, which default @@ -1240,21 +1223,19 @@ Root[a] ); } -/// Default output drops the version section, so a versioned plan and an -/// unversioned one print the same way. The version is not recoverable from that -/// output; the plan parsed back from it gets the built-in version instead. #[test] -fn test_version_dropped_by_default() { - let canonical = r#"=== Plan -Root[a] - Read[t => a:i64]"#; - let versioned = r#"=== Version 0.52.0 +fn test_version_maintained() { + let plan = r#"=== Version 0.52.0 producer: some-optimizer === Plan Root[a] Read[t => a:i64]"#; - assert_roundtrip_canonical(canonical, versioned); + roundtrip_plan(plan); + + let version = Parser::parse(plan).unwrap().version.unwrap(); + assert_eq!(version.minor_number, 52); + assert_eq!(version.producer, "some-optimizer"); } /// An all-default `Version` (0.0.0 with no producer/git_hash) is treated as @@ -1270,13 +1251,8 @@ Root[a] let parsed = Parser::parse(plan).unwrap(); assert_eq!(parsed.version, Some(Version::default())); - // ...but `Required` suppresses an all-default version, as does the default - // `Never`. - let options = OutputOptions { - show_version: Visibility::Required, - ..OutputOptions::default() - }; - let (text, errors) = format_with_options(&parsed, &options); + // ...but an all-default version says nothing, so it is not written out. + let (text, errors) = format(&parsed); assert!(errors.is_empty()); assert!( !text.contains("=== Version"), @@ -1293,7 +1269,13 @@ fn test_version_all_zero_shown_when_always() { Root[a] Read[t => a:i64]"#; - roundtrip_showing_version(plan, Visibility::Always); + roundtrip_plan_with_options( + plan, + &OutputOptions { + show_version: Visibility::Always, + ..OutputOptions::default() + }, + ); let parsed = Parser::parse(plan).unwrap(); assert_eq!(parsed.version, Some(Version::default())); @@ -1307,7 +1289,13 @@ fn test_version_null_roundtrip() { Root[a] Read[t => a:i64]"#; - roundtrip_showing_version(plan, Visibility::Always); + roundtrip_plan_with_options( + plan, + &OutputOptions { + show_version: Visibility::Always, + ..OutputOptions::default() + }, + ); let parsed = Parser::parse(plan).unwrap(); assert!( From 135e038e7097face617b2a17cb7a2f730594776e Mon Sep 17 00:00:00 2001 From: gord02 Date: Mon, 24 Aug 2026 11:27:44 -0400 Subject: [PATCH 3/4] fix: applying suggestions from code review Co-authored-by: Wendell Smith Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/parser/structural.rs | 6 +++++- src/textify/foundation.rs | 3 +-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/parser/structural.rs b/src/parser/structural.rs index b7f028a2..5fb7da1a 100644 --- a/src/parser/structural.rs +++ b/src/parser/structural.rs @@ -167,7 +167,7 @@ impl<'a> LineNode<'a> { } } -/// The [`Version`] a plan gets when the text is empty. +/// The [`Version`] a plan gets when the input text omits a version section. /// /// Substrait's `plan.proto` documents `Plan.version` as "optional up to 0.17.0, /// required for later versions". @@ -962,6 +962,10 @@ impl<'a> Parser<'a> { // An explicitly set version (either null or with a // version number / attributes) will override this. // `None` here means 'explicitly unset'. + // We set the version to our substrait-explain default. + // An explicitly set version (either null or with a + // version number / attributes) will override this. + // `None` here means 'explicitly unset'. version: Some(default_plan_version()), extension_parser: ExtensionParser::default(), extension_registry: ExtensionRegistry::new(), diff --git a/src/textify/foundation.rs b/src/textify/foundation.rs index b565f1f3..43d43da0 100644 --- a/src/textify/foundation.rs +++ b/src/textify/foundation.rs @@ -32,8 +32,7 @@ pub struct OutputOptions { /// If `Never`, the section is left out entirely. /// /// If `Required` - the default - the plan's version is shown, unless it is - /// empty or the [`default_plan_version`](crate::default_plan_version)(add by the parser). - /// + /// empty or the [`default_plan_version`](crate::default_plan_version) (added by the parser). /// If `Always`, the section is shown even when the plan has no version at /// all, as `=== Version null`. pub show_version: Visibility, From 26b2b6423da228d244e577b5a4b72c048ca0d41e Mon Sep 17 00:00:00 2001 From: "gordon.hamilton" Date: Mon, 24 Aug 2026 11:30:13 -0400 Subject: [PATCH 4/4] fix: formatting --- src/parser/structural.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser/structural.rs b/src/parser/structural.rs index 5fb7da1a..a3210494 100644 --- a/src/parser/structural.rs +++ b/src/parser/structural.rs @@ -963,7 +963,7 @@ impl<'a> Parser<'a> { // version number / attributes) will override this. // `None` here means 'explicitly unset'. // We set the version to our substrait-explain default. - // An explicitly set version (either null or with a + // An explicitly set version (either null or with a // version number / attributes) will override this. // `None` here means 'explicitly unset'. version: Some(default_plan_version()),