Skip to content
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

Generates internal impl IR classes; updates partiql-plan #1109

Merged
merged 8 commits into from
Jun 14, 2023
Merged

Conversation

rchowell
Copy link
Contributor

@rchowell rchowell commented May 30, 2023

Tasks

Relevant Issues / Description

Related to #1054 we are splitting out Plan and AST intermediate representations into their own packages. This changes the generator to produce internal implementations and public interfaces which adhere to Explicit API.

You cannot directly instantiate an IR node, but you get four ways to do so

  • Type Safe Builders
  • Type Safe Builder via a DSL (with optional custom factory)
  • Default Factory methods
  • Custom Factory methods

The default implementation of a domain's abstract factory is a top-level object with the domain name (same as the PIG builder). In short,

// Before, direct constructor
val myNode = Rex.Id("a") 

// After, default factory method
val myNode = Plan.rexId("a") 

Additionally, each IR node has an _id: String field. The _id field is a simple unique identifier which can be used as a key, but nodes can be directly used in other data structures via hashCode and equals.

Other Information

  • Updated Unreleased Section in CHANGELOG: [YES/NO]
    YES

  • Any backward-incompatible changes? [YES/NO]
    YES. This is a breaking change. Customers would need to replace the direct constructors with the default factory method. Nothing has changed beyond the "constructor" location.

  • Any new external dependencies? [YES/NO]
    NO

License Information

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

@github-actions
Copy link

github-actions bot commented May 30, 2023

Conformance comparison report

Base (adb32f7) b9c3b1c +/-
% Passing 92.47% 92.47% 0.00%
✅ Passing 5380 5380 0
❌ Failing 438 438 0
🔶 Ignored 0 0 0
Total Tests 5818 5818 0

Number passing in both: 5380

Number failing in both: 438

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

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

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2023

Codecov Report

Patch coverage: 57.47% and no project coverage change.

Comparison is base (adb32f7) 73.67% compared to head (f0bd70c) 73.67%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1109   +/-   ##
=========================================
  Coverage     73.67%   73.67%           
- Complexity     2404     2407    +3     
=========================================
  Files           248      248           
  Lines         17729    17741   +12     
  Branches       3230     3235    +5     
=========================================
+ Hits          13062    13071    +9     
  Misses         3676     3676           
- Partials        991      994    +3     
Flag Coverage Δ
CLI 14.28% <ø> (ø)
EXAMPLES 80.28% <ø> (ø)
LANG 79.46% <57.47%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rtiql/lang/planner/transforms/plan/RexConverter.kt 53.52% <53.84%> (ø)
.../partiql/lang/planner/transforms/plan/PlanTyper.kt 48.00% <61.53%> (ø)
...rtiql/lang/planner/transforms/plan/RelConverter.kt 66.98% <61.90%> (ø)
...n/org/partiql/lang/planner/transforms/AstToPlan.kt 88.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@johnedquinn johnedquinn left a comment

Choose a reason for hiding this comment

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

Left small comments, but everything looks pretty good!

I did have one question about testing. Have you considered setting up a build task for generating a test IR? It could be useful to generate the test IR and then attempt to access some of the generated properties/methods as part of the unit tests -- just to show that it compiles and to make it easier for review.

@rchowell rchowell merged commit a899307 into main Jun 14, 2023
@rchowell rchowell deleted the ir-impl branch June 14, 2023 21:05
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.

3 participants