Skip to content

Conversation

am357
Copy link
Contributor

@am357 am357 commented Aug 12, 2024

Description of changes:

This PR

  • adds NodeId to StaticType; this is to be able to use the id as a reference to add additional data to the types out of band.
  • makes AutoNodeIdGenerator thread-safe
  • adds PartiqlShapeBuilder and moves some PartiqlShape APIs to it; this is to be able to generate unique NodeIds for a PartiqlShape that includes static types that themselves can include other static types.
  • adds a static thread safe shape_builder function that provides a convenient way for using PartiqlShapeBuilder for creating new shapes.
  • prepends existing type macros with type such as type_int! to make macro names more friendly.
  • removes const PartiQL types under partiql-types in favor of PartiqlShapeBuilder.

The majority of the diffs are related to the macro renames or replacement with the previous const types. The main change is in partiql-types/src/lib.rs file.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 75.31172% with 99 lines in your changes missing coverage. Please review.

Project coverage is 80.98%. Comparing base (e5b8ce7) to head (74c3f67).
Report is 1 commits behind head on main.

Files Patch % Lines
partiql-types/src/lib.rs 64.53% 72 Missing ⚠️
partiql-logical-planner/src/typer.rs 82.41% 16 Missing ⚠️
extension/partiql-extension-ddl/src/ddl.rs 72.41% 5 Missing and 3 partials ⚠️
partiql-eval/src/eval/expr/operators.rs 93.54% 2 Missing ⚠️
partiql-eval/src/eval/expr/pattern_match.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #485      +/-   ##
==========================================
- Coverage   81.00%   80.98%   -0.03%     
==========================================
  Files          69       69              
  Lines       18704    18796      +92     
  Branches    18704    18796      +92     
==========================================
+ Hits        15152    15221      +69     
- Misses       3110     3131      +21     
- Partials      442      444       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Aug 12, 2024

Conformance comparison report

Base (e5b8ce7) 3657ec5 +/-
% Passing 90.35% 90.35% 0.00%
✅ Passing 5731 5731 0
❌ Failing 612 612 0
🔶 Ignored 0 0 0
Total Tests 6343 6343 0

Number passing in both: 5731

Number failing in both: 612

Number passing in Base (e5b8ce7) but now fail: 0

Number failing in Base (e5b8ce7) but now pass: 0

This PR
- adds `NodeId` to `StaticType`.
- makes `AutoNodeIdGenerator` thread-safe
- adds `PartiqlShapeBuilder` and moves some `PartiqlShape` APIs to it; this is to be able to generate unique `NodeId`s for a `PartiqlShape` that includes static types that themselves can include other static types.
- adds a static thread safe `shape_builder` function that provides a convenient way for using `PartiqlShapeBuilder` for creating new shapes.
- prepends existing type macros with `type` such as `type_int!` to make macro names more friendly.
- removes `const` PartiQL types under `partiql-types` in favor of `PartiqlShapeBuilder`.
@am357 am357 force-pushed the feat-type-nodeid branch 4 times, most recently from 0801a40 to b40fdc6 Compare August 13, 2024 15:25
@am357 am357 marked this pull request as ready for review August 13, 2024 15:33
@am357 am357 requested a review from jpschorr August 13, 2024 15:33
@am357 am357 force-pushed the feat-type-nodeid branch 2 times, most recently from 0955cfd to fd3badd Compare August 13, 2024 17:30
@am357 am357 force-pushed the feat-type-nodeid branch from fd3badd to 1b946b4 Compare August 13, 2024 17:36
@am357 am357 changed the title Adds PartiqlShapeBuilder with NodeId generation for to the StaticType Adds PartiqlShapeBuilder with NodeId generation for the StaticType Aug 13, 2024
Copy link
Contributor

@jpschorr jpschorr left a comment

Choose a reason for hiding this comment

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

PartiqlShapeBuilder.any_of:

    pub fn any_of<I>(&self, types: I) -> PartiqlShape
    where
        I: IntoIterator<Item = PartiqlShape>,
    {
        let any_of = AnyOf::from_iter(types);
        match any_of.types.len() {
            0 => type_dynamic!(),
            1 => {
                let AnyOf { types } = any_of;
                types.into_iter().next().unwrap()
            }
            // TODO figure out what does it mean for a Union to be nullable or not
            _ => PartiqlShape::AnyOf(any_of),
        }
    }

The AnyOf::from_iter(types) uses an IndexSet internally to deduplicate types, thus the match on any_of.types.len() could "flatten" AnyOfs that had duplicates. With the addition of IDs, this deduplication no longer happens...

@am357 am357 requested a review from jpschorr August 14, 2024 20:33
@am357 am357 merged commit 7203852 into main Aug 14, 2024
18 of 19 checks passed
@am357 am357 deleted the feat-type-nodeid branch August 14, 2024 22:17
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.

2 participants