Add Plan Evaluator #592
Merged
Add Plan Evaluator #592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dlurton
changed the base branch from
main
to
physical-plan-staging-planner-pipeline
4 tasks
- AST->logical - logical->resolved - lesolved->physical. Not yet integrated with anything--that will come in a future commit.
dlurton
force-pushed
the
physical-plan-staging-planner-pipeline
branch
from
cf7f832
to
c20b466
Compare
dlurton
force-pushed
the
physical-plan-staging-planner-pipeline
branch
from
c20b466
to
10ebc0e
Compare
dlurton
force-pushed
the
physical-plan-staging-plan-evaluator
branch
from
e2b4a98
to
bb6ebb4
Compare
dlurton
force-pushed
the
physical-plan-staging-plan-evaluator
branch
from
bb6ebb4
to
3b898db
Compare
Merged
…plan-staging-planner-pipeline
…plan-staging-planner-pipeline
…lan-staging-plan-evaluator
dlurton
commented
May 24, 2022
| * | ||
| * [1]: https://www.complang.tuwien.ac.at/anton/lvas/sem06w/fest.pdf | ||
| */ | ||
| internal class PhysicalExprToThunkConverterImpl( |
There was a problem hiding this comment.
This is the copied & revised EvaluatingCompiler. You'll note that the compileSelect() function is gone.
It is not worth reviewing the entire file in detail since most of it's old code with only PartiqlAst replaced with PartiqlPhysical. I'll note the places below that are new, however.
Comment on lines
+276
to
+290
| private fun compileBindingsToValues(expr: PartiqlPhysical.Expr.BindingsToValues): PhysicalPlanThunk { | ||
| val mapThunk = compileAstExpr(expr.exp) | ||
| val bexprThunk: RelationThunkEnv = PhysicalBexprToThunkConverter(this, thunkFactory.valueFactory) | ||
| .convert(expr.query) | ||
|
|
||
| return thunkFactory.thunkEnv(expr.metas) { env -> | ||
| val elements = sequence { | ||
| val relItr = bexprThunk(env) | ||
| while (relItr.nextRow()) { | ||
| yield(mapThunk(env)) | ||
| } | ||
| } | ||
| valueFactory.newBag(elements) | ||
| } | ||
| } |
There was a problem hiding this comment.
This compileBindingsToValues is a new function.
Comment on lines
+906
to
+912
| @Suppress("UNUSED_PARAMETER") | ||
| private fun compileLocalId(expr: PartiqlPhysical.Expr.LocalId, metas: MetaContainer): PhysicalPlanThunk { | ||
| val localIndex = expr.index.value.toIntExact() | ||
| return thunkFactory.thunkEnv(metas) { env -> | ||
| env.registers[localIndex] | ||
| } | ||
| } |
There was a problem hiding this comment.
compileLocalId is new.
TODO: is there a reason I can't delete the unused metas parameter?
| } | ||
| } | ||
|
|
||
| private fun compileStruct(expr: PartiqlPhysical.Expr.Struct): PhysicalPlanThunk { |
There was a problem hiding this comment.
This function was modified to support the enhanced struct constructors.
| } | ||
|
|
||
| /** | ||
| * Given an AST node that represents a `LIKE` predicate, return an ExprThunk that evaluates a `LIKE` predicate. |
| * Represents an element in a select list that is to be projected into the final result. | ||
| * i.e. an expression, or a (project_all) node. | ||
| */ | ||
| private sealed class CompiledStructPart { |
There was a problem hiding this comment.
This class is new and is part of support for the enhanced struct constructor.
| @@ -98,10 +97,17 @@ class EvaluatorTests { | |||
| "selectDistinctWithAggregate", // TODO: Support aggregates in physical plans | |||
| "selectDistinctAggregationWithGroupBy", // TODO: Support GROUP BY in physical plans | |||
There was a problem hiding this comment.
Not sure why, but these were missed in #586
dlurton
commented
May 24, 2022
|
|
||
| /** | ||
| * Provides rules for basic AST sanity checks that should be performed before any attempt at further phsycial | ||
| * plan processing. This is provided as a distinct [PartiqlPhysical.Visitor] so that [PlanCompiler] may assume that |
There was a problem hiding this comment.
PlanCompiler is the old name.
am357
approved these changes
May 24, 2022
| /** | ||
| * Simple API that defines a method to convert a [PartiqlPhysical.Expr] to a [PhysicalPlanThunk]. | ||
| * | ||
| * Intended to abstract prevent [PhysicalBexprToThunkConverter] from having to take a direct dependency on |
There was a problem hiding this comment.
Nit.: abstract prevent -> prevent?
This reverts commit 2cb4314.
dlurton
changed the base branch from
physical-plan-staging-planner-pipeline
to
physical-plan-staging
am357
approved these changes
May 26, 2022
Comment on lines
+284
to
+285
| // customFunctions must be on the right side of + here to ensure that they overwrite any | ||
| // built-in functions with the same name. |
There was a problem hiding this comment.
I know this for later but I'm noting—overwrite any built-int—as a design decision that we need to make on the open type system.
| import kotlin.test.assertNotEquals | ||
| import kotlin.test.assertNull | ||
|
|
||
| internal class PlannerPipelineFactory : PipelineFactory { |
This was referenced May 26, 2022
dlurton
added a commit
that referenced
this pull request
May 27, 2022
Merges the following PRs to `main`: - #583 - #584 - #587 - #588 - #589 - #609 - #590 - #592 Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
RCHowell
pushed a commit
that referenced
this pull request
Jun 1, 2022
Merges the following PRs to `main`: - #583 - #584 - #587 - #588 - #589 - #609 - #590 - #592 Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
RCHowell
pushed a commit
that referenced
this pull request
Jun 1, 2022
Merges the following PRs to `main`: - #583 - #584 - #587 - #588 - #589 - #609 - #590 - #592 Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
RCHowell
pushed a commit
that referenced
this pull request
Jun 1, 2022
Merges the following PRs to `main`: - #583 - #584 - #587 - #588 - #589 - #609 - #590 - #592 Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Yes, there's a failed unit test here. I will need to fix that before this can be merged--that does not however, mean can'tEDIT: No longer, this is now fixed.get started reviewing this before then.
This was originally part of #582 and is based on #590, which must be merged first.
Compilation of a query plan is now handled by two classes:
PhysicalBexprToThunkConverter, which compiles the physical algebra itself.PhysicalExprToThunkConverterImpl, which is a copy and paste ofEvaluatingCompilerwith:(select ...)node (including the features out of scope for now:ORDER BY,GROUP BY,HAVING, aggregate functions (SUM,MAX,MIN, etc),PIVOTandUNPIVOT).(struct ...)constructor (that supports struct unions)idnode removed. (This is handled now by eitherlocal_idorglobal_id, where variables can be resolved at compile time and by the$__dynamic_lookup__(...)ExprFunctionif they cannot (and this is enabled).bindings_to_valuesexpression added (see PIG domains for planner and plan evaluator #584)The two classes are mutually recursive. Each invokes the other to compile value or relational expressions as needed.
Why Is the Duplication Needed
The duplication is unfortunate but necessary and can hopefully be removed in the short term. The goal is, once we're confident in the new plan compiler and it has reached feature parity, to delete
EvaluatingCompiler. All of the physical plan's node types (value and relational expressions) exist in thePartiqlPhysicaldomain while the AST's node types exist in thePartiqlAstdomain. If this duplication is to persist long term, we might need to look at how to eliminate it (possibly thru a new PIG feature), since maintaining bothPhysicalExprToThunkConverterImplandEvaluatingCompilerlong term could get onerous.Other Notables That Need Reviewing
EvaluatorStateand its uses. This is the equivalent ofEnvironmentfor the plan compiler.sequence { },RelationIteratoretc and their uses. Kotlin coroutines are used during relational operator evaluation.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.