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

[DRAFT] Experimental planner and physical plan evaluator #582

Closed
wants to merge 81 commits into from

Conversation

dlurton
Copy link
Member

@dlurton dlurton commented Apr 25, 2022

Split Into Smaller PRs:

Original text follows

80 commits seems like a lot, however please keep in mind that many of them of them are merge commits from when I pulled in upstream changes over the last month (PR #559 and later.) I would also add that, with the new code, may be 1/3 of it is actually a duplicated, then modified EvaluatingCompiler (currently in the file org/partiql/lang/eval/physical/PhysicalExprToThunkConverterImpl.kt). And about half of the remaining diffs are excluding the planner & plan evaluator from the unit tests for the unsupported features listed below. So, while this PR is still pretty large, the portion that needs review is smaller than it would first appear.

This PR introduces a nascent physical plan evaluator and extremely minimal query planner as a new API, leaving the old AST evaluator intact. This allows customers to switch between them at runtime, providing a migration path.

This does not yet support the PartiQL features listed below. These will need to be supported before this work can be considered a complete replacement for the legacy AST evaluator:

If a customer depends on any of these features today, they will not be able to migrate to the new planner and evaluator until it does.

At some point we'll also have to think about doing static type checking / inference on the resolved logical plan (partiql_logical_resolved PIG domain) instead of on the AST (partiql_ast). Although this is not strictly mandatory for the short term because the existing static type checker can work on the AST.

Key Components to be Reviewed

  • lang/resources/org/partiql/type-domains/partiql.ion, which was modified to define the logical and physical plans as permutations of partiql_ast.
  • org.partiql.lang.planner package, which contains a very minimal query planner that currently only parses a PartiQL query, converts to a logical plan, resolves variables, and converts to a physical plan)
  • org.partiql.eval.physical package, which contains the physical plan evaluator.
  • org.partiql.lang.eval.evaluatortestframework, which has changes to allow all evaluation unit tests to be executed under planner & plan evaluator in addition to the old AST evaluator.

Tasks before this can be merged (now completed):

  • Add top-most "plan" node in query plans with a version number. i.e:
(plan
    (expression <expr>)
    (version <plan-version>) 
)     
  • Changes to static variable resolution (declare local variables under top-most plan node.)
(plan
    (locals (local_variable a 0) (local_variable 1))
    (expression <expr>)
)     
  • Make dynamic_id a function instead of a node.
  • Support struct-union with struct constructor, to support SELECT-list projections.

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

dlurton and others added 30 commits March 28, 2022 11:23
Mostly, this involved deleting code related to nodes present in PartiqlAst but not PartiqlPhysical.
3100+ failing unit tests though, to be expected for the time being.
Previously, build times were impact 10-30 seconds during IDE
builds, hampering developer productivitiy.  The following
changes in this commit should significantly improve the IDE
experience:

- Use PIG snapshot (which splits output into multiple files),
- Move pig output to build folder,
- Set up task dependencies correctly in gradle to avoid
needlessly regenerating & recompiling the PIG-generated files.
(needed dynamicId support because most CROSS JOIN tests uses dynamic resolution of variables)
These are to be implemented after this CR.
@dlurton dlurton marked this pull request as draft April 25, 2022 18:37
@dlurton dlurton changed the title [DRAFT] Experimental physical plan evaluator [DRAFT] Experimental planner and physical plan evaluator Apr 25, 2022
@almann
Copy link
Contributor

almann commented Apr 29, 2022

80 commits seems like a lot, however please keep in mind that many of them of them are merge commits from when I pulled in upstream changes over the last month (PR #559 and later.) I would also add that, with the new code, may be 1/3 of it is actually a duplicated, then modified EvaluatingCompiler (currently in the file org/partiql/lang/eval/physical/PhysicalExprToThunkConverterImpl.kt). And about half of the remaining diffs are excluding the planner & plan evaluator from the unit tests for the unsupported features listed below. So, while this PR is still pretty large, the portion that needs review is smaller than it would first appear.

@dlurton - I know this is a draft, but I wanted to check in and see how you'd like this to be reviewed.

You indicate above that a good portion of the PR is noise and has merge commits. Should we have this cleanly re-based so that the merge commits are not noise? Are you already planning to do that as a prerequisite? Are there features this relies on that has not been merged?

You also point out that a third of it is a duplicated an modified evaluating compiler--should that just be a separate PR on its own (probably dependent on this one)? Even your changes to the Unit tests might be better factored into another PR.

Just trying to get some context here for myself or other potential reviewers--I think it might be helpful in the interest of making it easier for you to get these changes merged.

cc: @jpschorr or @am357 as FYI, wouldn't mind any thoughts you might have on this PR.

@dlurton dlurton closed this Apr 30, 2022
@dlurton dlurton deleted the physical-plan-evaluator branch May 26, 2022 16:28
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