fix: making version non-default

[gord02]

Description

The version in the plan proto is currently always printed. This information ins't always necessary to view when printing a plan so we should print this primarily in verbose mode.

Type of Change

• Bug fix

Testing

• Added tests for new functionality
• All existing tests pass

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 770adadd16

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve explicit versions in default output

Use Visibility::Required as the default rather than Never: when format() or the CLI converts a JSON/protobuf plan carrying an explicit version, this setting omits the entire section, and parsing the emitted text replaces the original version with default_plan_version(); a plan whose version is None likewise becomes versioned. Thus an ordinary conversion or round trip silently changes compatibility and producer metadata unless users know to opt into --show-plan-version.

AGENTS.md reference: AGENTS.md:L90-L92

Useful? React with 👍 / 👎.

[wackywendell]

Suggested change

	/// A document with no version(`=== Version null`) section gets
	/// [`default_plan_version`](crate::default_plan_version);
	///

This comment doesn't fit here, it's the wrong level of detail here.

Also, this seems wrong anyway:

• === Version null in the text should translate to unset Version in the protobuf
• A missing Version section should translate to the 'default' plan version

[wackywendell]

Suggested change

	version: Some(default_plan_version()),
	// 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()),

Given that its unusual to set something to Some by default and then override it to None, this could use explanation.

[wackywendell]

Why are we adding this specifically to Validate? That seems odd...

[wackywendell]

I think instead of adding these as separate options, we should add a --detailed flag which uses OutputOptions::verbose(). I'm not sure we need specific flags for every option; we don't have them now.

[wackywendell]

Why is this an option for Validate? What would it mean to enable / disable it?

Without good reason for adding it, I don't think we should.

[wackywendell]

Firstly, I think we should just drop show_plan_version as an option to Validate.

Secondly, even if there was a good reason to have this, we should add options here, not add each option one by one.

[wackywendell]

This is oddly specific. May I suggest:

pub fn roundtrip_plan(input: &str) {
    roundtrip_plan_with_options(input, &OutputOptions::default());
}

pub fn roundtrip_with_options(input: &str, options: &OutputOptions) {
    …
}

Then in your tests:

let options = OutputOptions {
    show_version: Visibility::Required,
    ..OutputOptions::default()
};

roundtrip_plan_with_options(plan, &options);

That's a more generally useful function, and both arguments (plan and options are at the same abstraction level.

[wackywendell]

Alternatively, this interface is getting complex enough now it's probably worth framing our data structures to fit:

struct Roundtrip<'a> {
    canonical: &'a str,
    equivalent_inputs: Vec<&'a str>,
    options: OutputOptions,
}

Then you can use it like so:

// Simple roundtrip
Roundtrip::new(plan).assert();

// with options
Roundtrip::new(plan)
    .with_options(OutputOptions {
        show_version: Visibility::Always,
        ..OutputOptions::default()
    })
    .assert();

// with a canonical and equivalent plan
Roundtrip::new(canonical)
    .also_accepts(equivalent)
    .assert();

This is a non-blocking suggestion - feel free to just do the roundtrip_with_options suggested above.

[wackywendell]

Suggested change

	- `--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

Let's make this one option rather than two.

[wackywendell]

Suggested change

- `--show-plan-version` - Show the plan's Substrait version

Let's drop this, it doesn't make much sense as part of validate

[wackywendell]

Suggested change

	#
	# 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
	# );

You don't need a full test here asserting lots of things - the tests in GRAMMAR.md should merely assert that the example plan is accepted as valid input.