Skip to content

feat: rich proto types in ExtensionArgs for Explainable trait (#98)#102

Draft
wackywendell wants to merge 2 commits intomainfrom
wendell/ext-pieces
Draft

feat: rich proto types in ExtensionArgs for Explainable trait (#98)#102
wackywendell wants to merge 2 commits intomainfrom
wendell/ext-pieces

Conversation

@wackywendell
Copy link
Copy Markdown
Collaborator

Description

Redesigns the extension argument system so Explainable implementations work directly with parsed Substrait proto types instead of raw strings.

Key changes:

  • ExtensionColumn::Named carries proto::Type instead of String — the parser already parsed these, now it keeps the result
  • ExtensionValue::Type(proto::Type) — new variant for type arguments in extension relations
  • RawExpression replaced with Expr(Box<proto::Expression>) — expressions are now fully parsed
  • ExplainContext passed to from_args/to_args, with schema()/columns() helpers for NamedStruct conversion
  • Grammar adds extension_type_arg for types in extension arguments (simple + compound types)
  • fixtures::parse_type() helper for constructing types from strings in tests

Breaking changes to the Explainable trait:

  • from_args(args: &ExtensionArgs)from_args(args: &ExtensionArgs, ctx: &ExplainContext)
  • to_args(&self)to_args(&self, ctx: &ExplainContext)
  • ExtensionColumn::Named { type_spec: String }{ ty: proto::Type }
  • RawExpression removed, Expr newtype added
  • Registry parse_*/decode_* methods take &SimpleExtensions

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Example update
  • Other (please describe)

Testing

  • Added tests for new functionality
  • All existing tests pass
  • Ran examples to verify behavior

7 new tests cover Type arguments (roundtrip), TryFrom impls, and ExplainContext schema/columns helpers including error cases.

Checklist

  • Code follows Rust conventions
  • Documentation updated if needed
  • No breaking changes (or breaking changes documented)
  • Pre-commit hooks pass

Related Issues

Closes #98

@wackywendell
Copy link
Copy Markdown
Collaborator Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 940c92f64b

ℹ️ 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".

Comment thread src/parser/expression_grammar.pest Outdated
Comment on lines +306 to +307
extension_type_arg = { simple_type | compound_type }
extension_argument = { enum_value | reference | extension_type_arg | literal | expression }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Accept user-defined types in extension arguments

ExtensionValue::Type now carries arbitrary proto::Type, and textification will emit user-defined types, but the new grammar only permits simple_type | compound_type for extension_type_arg. As a result, a valid top-level user-defined type argument (for example, from an extension’s to_args) cannot be parsed back, so format→parse roundtrips break for extensions that use user-defined type args. This is especially problematic for named args where the ambiguity concern does not apply after =.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose public API for type string conversion and NamedStruct ↔ ExtensionColumn mapping

1 participant