feat!: add `parse` module #89

mbrobbel · 2023-04-05T20:36:54Z

Some requirements of Substrait data can not be expressed via Protobuf definition or schema files. This module provides new types for the generated types, that when constructed are known to be checked. This enables producers and consumers to skip redundant checking of invariants described by the specification.

This is based on the idea described in the Parse don't validate blog post.

Proposal

The approach is to build the parsing around two traits: one for parsing and one for the parser context.

/// A parse trait.
pub trait Parse<C: Context>: fmt::Debug + Sized {
    /// The parsed type.
    ///
    /// After parsing this type must be able to convert back. Note that it is
    /// not required for the conversion to be lossless, as long as the semantics
    /// don't change.
    ///
    /// This bound also helps with tracking breaking Protobuf definition changes
    /// via compilation errors.
    type Parsed: Into<Self>;

    /// The error type for this parser.
    type Error: error::Error;

    /// Parse and return a parsed type or error.
    fn parse(self, ctx: &mut C) -> Result<Self::Parsed, Self::Error>;
}

/// A parse context.
///
/// Parsing Substrait data is context-sensitive. This trait provides methods
/// that can be used by parser implementations to parse Substrait data.
pub trait Context {
    /// Parse an item using this context.
    ///
    /// See [Parse::parse].
    #[tracing::instrument(skip_all, err, fields(item = %std::any::type_name::<T>()))]
    fn parse<T: Parse<Self>>(&mut self, item: T) -> Result<T::Parsed, T::Error>
    where
        Self: Sized,
    {
        item.parse(self)
    }

    /// Parse an item using this context and record the item value on the parse span.
    ///
    /// See [Parse::parse].
    #[tracing::instrument(name = "parse", skip_all, err, fields(item = %std::any::type_name::<T>(), value = ?item))]
    fn parse_record_value<T: Parse<Self>>(&mut self, item: T) -> Result<T::Parsed, T::Error>
    where
        Self: Sized,
    {
        item.parse(self)
    }

    /// Parse an item using this context and record the item value and its index on the parse span.
    ///
    /// See [Parse::parse].
    #[tracing::instrument(name = "parse", skip_all, err, fields(item = %std::any::type_name::<T>(), index = index, value = ?item))]
    fn parse_record_value_with_index<T: Parse<Self>>(
        &mut self,
        item: T,
        index: usize,
    ) -> Result<T::Parsed, T::Error>
    where
        Self: Sized,
    {
        item.parse(self)
    }

    /// Parse an item using this context and record the item index on the parse span.
    ///
    /// See [Parse::parse].
    #[tracing::instrument(name = "parse", skip_all, err, fields(item = %std::any::type_name::<T>(), index = index))]
    fn parse_record_index<T: Parse<Self>>(
        &mut self,
        item: T,
        index: usize,
    ) -> Result<T::Parsed, T::Error>
    where
        Self: Sized,
    {
        item.parse(self)
    }

    /// Add a [SimpleExtensionURI] to this context. Should return an error for duplicate
    /// anchors or when the URI is not supported.
    ///
    /// This function must eagerly resolve and parse the simple extension, returning an
    /// error if either fails.
    fn add_simple_extension_uri(
        &mut self,
        simple_extension_uri: &SimpleExtensionURI,
    ) -> Result<&SimpleExtensions, ContextError>;

    /// Returns the simple extensions for the given simple extension anchor.
    fn simple_extensions(&self, anchor: Anchor<SimpleExtensionURI>) -> &SimpleExtensions;

    /// Add an extension type declaration to this context.
    fn add_extension_type(
        &mut self,
        extension_type: &ExtensionType,
    ) -> Result<&SimpleExtensionsTypesItem, ContextError>;

    /// Returns the extension type for the given extension type anchor.
    fn extension_type(
        &self,
        extension_type_anchor: Anchor<ExtensionType>,
    ) -> &SimpleExtensionsTypesItem;

    /// Add an extension type variation declaration to this context.
    fn add_extension_type_variation(
        &mut self,
        extension_type_variation: &ExtensionTypeVariation,
    ) -> Result<&SimpleExtensionsTypeVariationsItem, ContextError>;

    /// Returns the extension type variation for the given extension type variation anchor.
    fn extension_type_variation(
        &self,
        extension_type_variation_anchor: Anchor<ExtensionTypeVariation>,
    ) -> &SimpleExtensionsTypeVariationsItem;

    /// Add an extension function declaration to this context.
    fn add_extension_function(
        &mut self,
        extension_function: &ExtensionFunction,
    ) -> Result<&SimpleExtensionFunction, ContextError>;

    /// Returns the extension function for the given extension type variation anchor.
    fn extension_function(
        &self,
        extension_function_anchor: Anchor<ExtensionFunction>,
    ) -> &SimpleExtensionFunction;
}

Progress

universalmind303 · 2023-04-17T14:23:19Z

@mbrobbel Just wondering if you've had a chance to make any more progress on this? I cloned this branch & started expanding the functionality. I'd be happy to continue this work if our are unable.

mbrobbel · 2023-04-17T14:36:47Z

@mbrobbel Just wondering if you've had a chance to make any more progress on this? I cloned this branch & started expanding the functionality. I'd be happy to continue this work if our are unable.

Yes I made some progress. I'll push it and fix conflicts. Happy to collaborate.

EpsilonPrime · 2024-03-06T22:00:21Z

src/lib.rs

@@ -140,6 +145,10 @@
 //! # Ok(()) }
 //! ```
 //!
+//! # Parsing
+//!
+//! Write about [parse] module.


Documentation goes here?

EpsilonPrime · 2024-03-06T22:03:40Z

src/parse/simple_extensions/mod.rs

+    /// Returns the compound names of this simple extensions function.
+    pub fn compound_names(&self) -> Vec<String> {
+        // todo(mbrobbel): parse functions
+        // notes :


Leftover comments to self?

EpsilonPrime · 2024-03-06T22:11:35Z

src/parse/version.rs

+    Substrait(semver::Version, semver::VersionReq),
+}
+
+impl<C: Context> Parse<C> for proto::Version {


I'm surprised to see all of this validation here. Were you aiming for putting the validation into a separate step from the parsing?

EpsilonPrime · 2024-03-06T22:49:08Z

src/lib.rs

@@ -12,6 +12,11 @@
 //! Protobuf serialization and deserialization are provided via [prost] in the
 //! [proto] module.
 //!
+//! > Please note that [protoc](https://grpc.io/docs/protoc-installation/) is


Is the canonical protobuf installation guide for Rust is on the grpc.io site? I would have expected a link to the protobuf site.

EpsilonPrime · 2024-03-06T22:53:45Z

src/parse/context.rs

+    #[error("undefined reference to extension function with anchor `{0}`")]
+    UndefinedExtensionFunction(Anchor<ExtensionFunction>),
+
+    /// Undefined reference to extension function.


function definition

EpsilonPrime · 2024-03-06T22:54:50Z

src/parse/extended_expression.rs

+    version: Version,
+    simple_extension_uris: Vec<SimpleExtensionURI>,
+    simple_extension_declarations: Vec<SimpleExtensionDeclaration>,
+    // ...


Is this a TODO?

EpsilonPrime · 2024-03-06T23:00:59Z

src/parse/parser.rs

+#[cfg(feature = "parse-reqwest")]
+#[derive(Default)]
+/// A resolver that uses [reqwest] to resolve simple extensions.
+pub struct ReqwestResolver {


Oh, reqwest is a package name. It may be useful to disable external checking of URIs -- that seems more like a validation step that might not be useful on systems just trying to use plans.

EpsilonPrime · 2024-03-06T23:48:56Z

src/parse/plan.rs

+use crate::proto;
+use thiserror::Error;
+
+/// A parsed [proto::Plan].


From a naming standpoint I'd consider the plan to be parsed once you use the protobuf parse methods to deserialize it. As a second step (perhaps called study) it makes more sense to me to have the validation that is present in this PR. I could see even more validation as an optional third step, like checking for issues with output mappings, function name checking, etc.

EpsilonPrime · 2024-03-06T23:56:29Z

src/parse/relation.rs

+pub enum Relation {
+    /// A read relation.
+    Read(ReadRelation),
+}


TODO for the other relation types?

mbrobbel · 2024-03-07T08:50:07Z

Closing (I'm going to spllit this PR up). See #157.

Include core extensions from `Subtrait`. The majority of the code originates from the un-merged pr #89. --------- Co-authored-by: Matthijs Brobbel <[email protected]>

mbrobbel added 5 commits April 5, 2023 22:31

Add parse module

f8be071

Remove .vscode settings

6679405

Fix typo and add missing license header

744394e

Merge branch 'main' into parse-dont-validate

c5ccc8b

Fix clippy warnings

a744283

mbrobbel added 23 commits April 17, 2023 16:57

Implement parse for simple extension declarations

af97462

Merge branch 'main' into parse-dont-validate

325197a

Fix warnings, tests and docs workflow

b01728e

Merge branch 'main' into parse-dont-validate

f17b6f1

Move some items between modules and add wrapper abstraction

55f0414

Merge branch 'main' into parse-dont-validate

1fbd407

Bump MSRV to 1.65.0 for let else

2e73082

Bump MSRV to 1.66.0 for map_first_last

0737ced

Add support for different simple extensions resolvers in the parser

6ca6572

Add missing headers

3ae5dd1

Merge branch 'main' into parse-dont-validate

fa6b67d

Fix clippy warning

f34ac75

Add cache for Rust jobs

f5e413c

Merge branch 'main' into parse-dont-validate

4b25b5a

Update cache keys

4261655

Update restore keys

023c374

Remove build step from package job

3d76304

Trigger build script before package

c70f2d0

Package needs build script output

8861eb5

Fix package problems

d148244

Fix serde feature build

af15603

Only show the first error

ecedd7f

Merge branch 'main' into parse-dont-validate

63de23a

mbrobbel mentioned this pull request Mar 5, 2024

Tracking issue for parse module #157

Open

32 tasks

anyhow is a dependency of the parse feature

aebd6cc

EpsilonPrime approved these changes Mar 6, 2024

View reviewed changes

mbrobbel closed this Mar 7, 2024

shanretoo mentioned this pull request May 18, 2024

feat(extensions): include Substrait core extensions #187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add `parse` module #89

feat!: add `parse` module #89

mbrobbel commented Apr 5, 2023 •

edited

Loading

universalmind303 commented Apr 17, 2023

mbrobbel commented Apr 17, 2023

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

EpsilonPrime Mar 6, 2024

mbrobbel commented Mar 7, 2024

feat!: add parse module #89

feat!: add parse module #89

Conversation

mbrobbel commented Apr 5, 2023 • edited Loading

Proposal

Progress

universalmind303 commented Apr 17, 2023

mbrobbel commented Apr 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrobbel commented Mar 7, 2024

feat!: add `parse` module #89

feat!: add `parse` module #89

mbrobbel commented Apr 5, 2023 •

edited

Loading