diff --git a/API.md b/API.md index c18c1311..2b2a8c4b 100644 --- a/API.md +++ b/API.md @@ -363,7 +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 +- `--detailed` - Show more detail on plans, including type annotations and plan version - `--verbose` - Show detailed progress information #### Validate Command @@ -394,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 8e275566..06a79d9c 100644 --- a/GRAMMAR.md +++ b/GRAMMAR.md @@ -116,8 +116,14 @@ 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 +``` ```rust # use substrait_explain::Parser; @@ -136,6 +142,19 @@ Root[result] # assert_eq!(version.producer, "my-optimizer"); ``` +```rust +# use substrait_explain::Parser; +# +# let plan_text = r#" +=== Version null +=== Plan +Root[result] + Read[orders => quantity:i32?] +# "#; +# +# Parser::parse(plan_text).unwrap(); +``` + #### Extension format ```text diff --git a/src/cli.rs b/src/cli.rs index c8257006..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,14 +75,14 @@ impl Cli { output, from, to, - show_literal_types, + 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); + 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( @@ -125,11 +123,11 @@ impl Cli { output, from, to, - show_literal_types, + detailed, verbose, .. } => { - let options = self.create_output_options(*show_literal_types); + 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( @@ -149,14 +147,12 @@ impl Cli { } } - fn create_output_options(&self, show_literal_types: 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() } - - options } fn resolve_input_format(&self, format: &Option, input_path: &str) -> Result { @@ -277,9 +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) + /// Show more detail on plans, including type annotations and plan version #[arg(long)] - show_literal_types: bool, + detailed: bool, /// Verbose output #[arg(short, long)] verbose: bool, @@ -515,7 +511,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -541,7 +537,7 @@ Root[result] output: "output.json".to_string(), from: Some(Format::Text), to: Some(Format::Json), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -568,7 +564,7 @@ Root[result] output: "output.json".to_string(), from: Some(Format::Text), to: Some(Format::Json), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -587,7 +583,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Json), to: Some(Format::Text), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -612,7 +608,7 @@ Root[result] output: "output.pb".to_string(), from: Some(Format::Text), to: Some(Format::Protobuf), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -685,7 +681,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: true, + detailed: true, verbose: false, }, }; @@ -693,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] @@ -738,7 +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, + detailed: false, verbose: false, }, }; @@ -764,7 +764,7 @@ Root[result] output: "output.json".to_string(), from: None, // Should fail auto-detection to: None, - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -790,7 +790,7 @@ Root[result] output: "output.unknown".to_string(), from: None, to: None, // Should fail auto-detection - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -816,7 +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, + detailed: false, verbose: false, }, }; @@ -841,7 +841,7 @@ Root[result] output: "output.pb".to_string(), from: Some(Format::Text), to: Some(Format::Protobuf), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -860,7 +860,7 @@ Root[result] output: "output.substrait".to_string(), from: Some(Format::Protobuf), to: Some(Format::Text), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -951,7 +951,7 @@ Root[val] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -974,7 +974,7 @@ Root[val] output: "output.json".to_string(), from: Some(Format::Text), to: Some(Format::Json), - show_literal_types: false, + detailed: false, verbose: false, }, }; @@ -1017,7 +1017,7 @@ Root[val] output: "output.substrait".to_string(), from: Some(Format::Text), to: Some(Format::Text), - show_literal_types: false, + detailed: false, verbose: false, }, }; diff --git a/src/lib.rs b/src/lib.rs index 1dc70596..1e626ca3 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; 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..a3210494 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 input text omits a version section. +/// +/// 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, @@ -946,7 +958,15 @@ impl<'a> Parser<'a> { line_no: 1, state: State::Initial, cursor: None, - version: 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'. + // 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(), relation_parser: RelationParser::default(), @@ -1097,7 +1117,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 +1131,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 +1173,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 +1209,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..43d43da0 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 `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) (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, /// 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::Required, 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..d2c17782 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, default_plan_version}; 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,34 @@ 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`](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 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() || v == &default_plan_version()) => + { + return Ok(()); + } + Some(v) => v, }; - if version == &proto::Version::default() { - return Ok(()); - } writeln!( w, 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 55da726d..d490961c 100644 --- a/tests/json_parsing.rs +++ b/tests/json_parsing.rs @@ -192,7 +192,7 @@ fn make_cli(from: Format) -> Cli { output: "-".to_string(), from: Some(from), to: Some(Format::Text), - show_literal_types: 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 f038b8be..4b19f806 100644 --- a/tests/plan_roundtrip.rs +++ b/tests/plan_roundtrip.rs @@ -7,8 +7,12 @@ mod common; -use common::{assert_roundtrip_canonical, roundtrip_plan}; -use substrait_explain::{ParseError, Parser}; +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, + format_with_options, +}; #[test] fn test_simple_plan_roundtrip() { @@ -1185,23 +1189,53 @@ Root[a] roundtrip_plan(plan); } -/// 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}" + ); +} + +#[test] +fn test_version_maintained() { + let plan = r#"=== Version 0.52.0 + producer: some-optimizer +=== Plan +Root[a] + Read[t => a:i64]"#; + + 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 @@ -1215,10 +1249,10 @@ Root[a] // The parser preserves the (empty) version it was given... let parsed = Parser::parse(plan).unwrap(); - assert!(parsed.version.is_some()); + assert_eq!(parsed.version, Some(Version::default())); - // ...but the formatter suppresses an all-default version. - let (text, errors) = substrait_explain::format(&parsed); + // ...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"), @@ -1226,6 +1260,68 @@ 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_plan_with_options( + plan, + &OutputOptions { + show_version: Visibility::Always, + ..OutputOptions::default() + }, + ); + + 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_plan_with_options( + plan, + &OutputOptions { + show_version: Visibility::Always, + ..OutputOptions::default() + }, + ); + + 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() {