-
Notifications
You must be signed in to change notification settings - Fork 27
feat: add initial extension support #395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
076998e
57e5dee
68f0e7d
dfef7ee
facb09b
5a5ac23
7869676
bc2a86b
1b49a6c
6dc4302
9c593a1
7a8584e
ac447d8
9b47219
8a44db4
47fbf18
5b1e5df
6444f5c
162e8ab
421d957
46b6468
83a4249
f415dc8
0cbfc34
93b41ef
470aa00
202747a
ac479d9
c0a60b4
ca1e133
95270ab
25d6392
3e58a42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,9 @@ | |
|
|
||
| use thiserror::Error; | ||
|
|
||
| use crate::parse::{ | ||
| Anchor, Parse, proto::extensions::SimpleExtensionUrn, text::simple_extensions::SimpleExtensions, | ||
| }; | ||
| use crate::parse::proto::extensions::SimpleExtensionUrn; | ||
| use crate::parse::text::simple_extensions::ExtensionFile; | ||
| use crate::parse::{Anchor, Parse}; | ||
|
|
||
| /// A parse context. | ||
| /// | ||
|
|
@@ -22,22 +22,24 @@ pub trait Context { | |
| { | ||
| item.parse(self) | ||
| } | ||
| } | ||
|
|
||
| pub trait ProtoContext: Context { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned, I don't have a ton of Rust experience, so it may be that this is totally standard. But I wonder if there's a simpler way to handle parsing that isn't so trait-heavy. AFAICT, there is only one implementor of the Could we drop the |
||
| /// Add a [SimpleExtensionUrn] to this context. Must return an error for duplicate | ||
| /// anchors or when the urn is not supported. | ||
| /// anchors or when the URI is not supported. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just consistently use |
||
| /// | ||
| /// This function must eagerly resolve and parse the simple extension, returning an | ||
| /// error if either fails. | ||
| fn add_simple_extension_urn( | ||
| &mut self, | ||
| simple_extension_urn: &SimpleExtensionUrn, | ||
| ) -> Result<&SimpleExtensions, ContextError>; | ||
| ) -> Result<&ExtensionFile, ContextError>; | ||
|
|
||
| /// Returns the simple extensions for the given simple extension anchor. | ||
| fn simple_extensions( | ||
| &self, | ||
| anchor: &Anchor<SimpleExtensionUrn>, | ||
| ) -> Result<&SimpleExtensions, ContextError>; | ||
| ) -> Result<&ExtensionFile, ContextError>; | ||
| } | ||
|
|
||
| /// Parse context errors. | ||
|
|
@@ -57,57 +59,49 @@ pub enum ContextError { | |
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(crate) mod tests { | ||
| pub(crate) mod fixtures { | ||
| use std::collections::{HashMap, hash_map::Entry}; | ||
|
|
||
| use crate::parse::{ | ||
| Anchor, context::ContextError, proto::extensions::SimpleExtensionUrn, | ||
| text::simple_extensions::SimpleExtensions, | ||
| text::simple_extensions::ExtensionFile, | ||
| }; | ||
|
|
||
| /// A test context. | ||
| /// | ||
| /// This currently mocks support for simple extensions (does not resolve or | ||
| /// parse). | ||
| #[derive(Default)] | ||
| pub struct Context { | ||
| empty_simple_extensions: SimpleExtensions, | ||
| simple_extensions: HashMap<Anchor<SimpleExtensionUrn>, SimpleExtensionUrn>, | ||
| simple_extensions: HashMap<Anchor<SimpleExtensionUrn>, ExtensionFile>, | ||
| } | ||
|
|
||
| impl Default for Context { | ||
| fn default() -> Self { | ||
| Self { | ||
| empty_simple_extensions: SimpleExtensions {}, | ||
| simple_extensions: Default::default(), | ||
| } | ||
| } | ||
| } | ||
| impl super::Context for Context {} | ||
|
|
||
| impl super::Context for Context { | ||
| impl super::ProtoContext for Context { | ||
| fn add_simple_extension_urn( | ||
| &mut self, | ||
| simple_extension_urn: &crate::parse::proto::extensions::SimpleExtensionUrn, | ||
| ) -> Result<&SimpleExtensions, ContextError> { | ||
| ) -> Result<&ExtensionFile, ContextError> { | ||
| match self.simple_extensions.entry(simple_extension_urn.anchor()) { | ||
| Entry::Occupied(_) => Err(ContextError::DuplicateSimpleExtension( | ||
| simple_extension_urn.anchor(), | ||
| )), | ||
| Entry::Vacant(entry) => { | ||
| // TODO: fetch | ||
| entry.insert(simple_extension_urn.clone()); | ||
| // For now just return an empty extension | ||
| Ok(&self.empty_simple_extensions) | ||
| let f = ExtensionFile::empty(simple_extension_urn.urn().clone()); | ||
| let ext_ref = entry.insert(f); | ||
|
|
||
| Ok(ext_ref) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn simple_extensions( | ||
| &self, | ||
| anchor: &Anchor<SimpleExtensionUrn>, | ||
| ) -> Result<&SimpleExtensions, ContextError> { | ||
| ) -> Result<&ExtensionFile, ContextError> { | ||
| self.simple_extensions | ||
| .contains_key(anchor) | ||
| .then_some(&self.empty_simple_extensions) | ||
| .get(anchor) | ||
| .ok_or(ContextError::UndefinedSimpleExtension(*anchor)) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,11 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //! Parsing of [text](crate::text) types. | ||
| //! Utilities for working with Substrait *text* objects. | ||
| //! | ||
| //! The generated [`crate::text`] module exposes the raw YAML-derived structs | ||
| //! (e.g. [`crate::text::simple_extensions::SimpleExtensions`]). This module | ||
| //! provides parsing helpers that validate those raw values and offer | ||
| //! higher-level wrappers for validation, lookups, and combining into protobuf | ||
| //! objects. | ||
|
|
||
| pub mod simple_extensions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, it looks like regress was already present before. Is this comment just clarifying why it was there in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just clarifying its use!