feat: rich proto types in ExtensionArgs for Explainable trait (#98)#102
feat: rich proto types in ExtensionArgs for Explainable trait (#98)#102wackywendell wants to merge 2 commits intomainfrom
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 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".
| extension_type_arg = { simple_type | compound_type } | ||
| extension_argument = { enum_value | reference | extension_type_arg | literal | expression } |
There was a problem hiding this comment.
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 👍 / 👎.
940c92f to
0b8ff72
Compare
Description
Redesigns the extension argument system so
Explainableimplementations work directly with parsed Substrait proto types instead of raw strings.Key changes:
ExtensionColumn::Namedcarriesproto::Typeinstead ofString— the parser already parsed these, now it keeps the resultExtensionValue::Type(proto::Type)— new variant for type arguments in extension relationsRawExpressionreplaced withExpr(Box<proto::Expression>)— expressions are now fully parsedExplainContextpassed tofrom_args/to_args, withschema()/columns()helpers for NamedStruct conversionextension_type_argfor types in extension arguments (simple + compound types)fixtures::parse_type()helper for constructing types from strings in testsBreaking changes to the
Explainabletrait: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 }RawExpressionremoved,Exprnewtype addedparse_*/decode_*methods take&SimpleExtensionsType of Change
Testing
7 new tests cover Type arguments (roundtrip), TryFrom impls, and ExplainContext schema/columns helpers including error cases.
Checklist
Related Issues
Closes #98