Add PlannerPipeline #590

dlurton · 2022-04-30T02:37:28Z

This PR was originally part of #582, and is based on #589, which must be merged first.

This PR adds PlannerPipeline and EvaluatorOptions, which are analagous to the AST evaluator's CompilerPipeline and CompileOptions, respectively. The difference is that the passes introduced in #588 are executed, after parsing the AST is converted into a logcial plan, variables are resolved, then its converted to a physical plan.

There is no ability to do so yet, however, PlannerPipeline will eventually house the APIs needed to make the planner user-extensible.

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

- AST->logical - logical->resolved - lesolved->physical. Not yet integrated with anything--that will come in a future commit.

…er-pipeline

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

dlurton

Noted a few places where I need to rename "global bindings" to "unique id resolver".

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

…plan-staging-planner-pipeline

am357 · 2022-05-23T22:09:17Z

lang/src/org/partiql/lang/SqlException.kt

@@ -34,12 +34,10 @@ import org.partiql.lang.util.propertyValueMapOf
 *
 * @param message the message for this exception
 * @param errorCode the error code for this exception
- * @param errorContextArg context for this error, contains details like line & column number among other attributes.
+ * @param errorContext context for this error, includes details like line & character offsets, among others.
+ * @param internal True to indicate that this exception a bug in ParitQL itself.  False to indicate it was caused by


Nit.: is a bug.

am357 · 2022-05-23T22:10:25Z

lang/src/org/partiql/lang/planner/EvaluatorOptions.kt

+
+/*
+
+Differences between CompilerOptions and PlannerOptions:


EvaluatorOptions?

Yes, thanks. Fixing...

am357 · 2022-05-23T22:14:00Z

lang/src/org/partiql/lang/planner/EvaluatorOptions.kt

+Differences between CompilerOptions and PlannerOptions:
+
+- There is no EvaluatorOptions equivalent for CompileOptions.visitorTransformMode since the planner always runs some basic
+ normalization and variable resolution passes *before* the customer can inject their own transforms.
+- There is no EvaluatorOptions equivalent for CompileOptions.thunkReturnTypeAssertions since PlannerPipeline does not
+support the static type inferencer (yet).
+- PlannerOptions.allowUndefinedVariables is new.
+- PlannerOptions has no equivalent for CompileOptions.undefinedVariableBehavior -- this was added for backward


EvaluatorOptions?

am357 · 2022-05-23T22:14:00Z

lang/src/org/partiql/lang/planner/EvaluatorOptions.kt

+Differences between CompilerOptions and PlannerOptions:
+
+- There is no EvaluatorOptions equivalent for CompileOptions.visitorTransformMode since the planner always runs some basic
+ normalization and variable resolution passes *before* the customer can inject their own transforms.
+- There is no EvaluatorOptions equivalent for CompileOptions.thunkReturnTypeAssertions since PlannerPipeline does not
+support the static type inferencer (yet).
+- PlannerOptions.allowUndefinedVariables is new.
+- PlannerOptions has no equivalent for CompileOptions.undefinedVariableBehavior -- this was added for backward


EvaluatorOptions?

am357 · 2022-05-23T22:21:18Z

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

+     * - Parses the specified SQL string, producing an AST.
+     * - Converts the AST to a logical plan.
+     * - Resolves all global and local variables in the logical plan, assigning unique indexes to local variables
+     * and calling [MetadataResolver.resolveVariable] to obtain PartiQL-service specific unique identifiers of global


Can we have an in place definition of PartiQL service here?

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

am357 · 2022-05-23T22:33:31Z

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

+        /**
+         * Add a custom function which will be callable by the compiled queries.
+         *
+         * Functions added here will replace any built-in function with the same name.
+         */
+        fun addFunction(function: ExprFunction): Builder = this.apply {
+            customFunctions[function.signature.name] = function
+        }
+
+        /**
+         * Add custom types to CAST/IS operators to.
+         *
+         * Built-in types will take precedence over custom types in case of a name collision.
+         */
+        fun customDataTypes(customTypes: List<CustomType>) = this.apply {
+            customDataTypes = customTypes
+        }
+
+        /**
+         * Add a custom stored procedure which will be callable by the compiled queries.
+         *
+         * Stored procedures added here will replace any built-in procedure with the same name.
+         */
+        fun addProcedure(procedure: StoredProcedure): Builder = this.apply {
+            customProcedures[procedure.signature.name] = procedure
+        }


I think the decisions that we're making here for custom functions, types, and procedures make future debugging or understanding the expected behavior difficult. I'd be more inclined to not to support them at this stage to leave the room open. If going with supporting them, wouldn't it make sense to disallow any name collisions with built-ins at all?

I think the decisions that we're making here for custom functions, types, and procedures make future debugging or understanding the expected behavior difficult.

I don't understand this statement--there are no decisions being made--their behavior and the behavior of this API is 100% identical to CompilerPipeline and the AST evaluator.

We had an offline discussion about this and—considering the open type system work that is started—current agreement is to be more restrictive w/ not introducing the custom function and type interfaces to the API at this stage and revisit once there is a defined use-case. If at that stage we don't have the impl from open type system we'll decide on incorporating the use-case with the least throw away work in this API.

am357 · 2022-05-23T22:33:31Z

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

+        /**
+         * Add a custom function which will be callable by the compiled queries.
+         *
+         * Functions added here will replace any built-in function with the same name.
+         */
+        fun addFunction(function: ExprFunction): Builder = this.apply {
+            customFunctions[function.signature.name] = function
+        }
+
+        /**
+         * Add custom types to CAST/IS operators to.
+         *
+         * Built-in types will take precedence over custom types in case of a name collision.
+         */
+        fun customDataTypes(customTypes: List<CustomType>) = this.apply {
+            customDataTypes = customTypes
+        }
+
+        /**
+         * Add a custom stored procedure which will be callable by the compiled queries.
+         *
+         * Stored procedures added here will replace any built-in procedure with the same name.
+         */
+        fun addProcedure(procedure: StoredProcedure): Builder = this.apply {
+            customProcedures[procedure.signature.name] = procedure
+        }


I think the decisions that we're making here for custom functions, types, and procedures make future debugging or understanding the expected behavior difficult. I'd be more inclined to not to support them at this stage to leave the room open. If going with supporting them, wouldn't it make sense to disallow any name collisions with built-ins at all?

am357 · 2022-05-23T22:34:20Z

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

+            val compileOptionsToUse = evaluatorOptions ?: EvaluatorOptions.standard()
+
+            when (compileOptionsToUse.thunkOptions.thunkReturnTypeAssertions) {
+                ThunkReturnTypeAssertions.DISABLED -> { /* intentionally blank */ }


Would be great to add the why in the comment.

am357 · 2022-05-23T22:34:20Z

lang/src/org/partiql/lang/planner/PlannerPipeline.kt

+            val compileOptionsToUse = evaluatorOptions ?: EvaluatorOptions.standard()
+
+            when (compileOptionsToUse.thunkOptions.thunkReturnTypeAssertions) {
+                ThunkReturnTypeAssertions.DISABLED -> { /* intentionally blank */ }


Would be great to add the why in the comment.

@am357

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.

@am357

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.

@am357

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.

@am357

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.

dlurton changed the title ~~Physical plan staging planner pipeline~~ Add PlannerPipeline Apr 30, 2022

dlurton changed the base branch from main to physical-plan-staging-filter-distinct April 30, 2022 02:38

dlurton force-pushed the physical-plan-staging-planner-pipeline branch from 8ba7305 to 53f6bda Compare April 30, 2022 02:39

This was referenced Apr 30, 2022

Query planner passes #588

Merged

[DRAFT] Experimental planner and physical plan evaluator #582

Closed

alancai98 mentioned this pull request May 2, 2022

Additional pipeline abstraction for test framework #583

Merged

dlurton mentioned this pull request May 4, 2022

Add Plan Evaluator #592

Merged

dlurton added this to the Basic Query Planner and Plan Evaluator milestone May 5, 2022

dlurton mentioned this pull request May 6, 2022

PIG domains for planner and plan evaluator #584

Merged

dlurton added 3 commits May 10, 2022 16:02

Add 3 query planner passes.

48a003e

- AST->logical - logical->resolved - lesolved->physical. Not yet integrated with anything--that will come in a future commit.

Address build failures

a5bd78b

Add FILTER_DISTINCT ExprFunction

befac52

dlurton force-pushed the physical-plan-staging-filter-distinct branch from 405ae74 to befac52 Compare May 11, 2022 00:14

dlurton force-pushed the physical-plan-staging-planner-pipeline branch from cf7f832 to c20b466 Compare May 11, 2022 00:44

Add PlannerPipeline

10ebc0e

dlurton force-pushed the physical-plan-staging-planner-pipeline branch from c20b466 to 10ebc0e Compare May 11, 2022 00:47

dlurton requested a review from alancai98 May 11, 2022 16:19

Base automatically changed from physical-plan-staging-filter-distinct to physical-plan-staging May 19, 2022 21:35

dlurton added 3 commits May 19, 2022 14:40

Merge branch 'physical-plan-staging' into physical-plan-staging-plann…

ebd36d3

…er-pipeline

Fix post-merge build failures

f18b6bb

Make ktlint happy

77a4ec5

dlurton requested a review from jpschorr May 19, 2022 22:09

dlurton commented May 23, 2022

View reviewed changes

lang/src/org/partiql/lang/planner/PlannerPipeline.kt Outdated Show resolved Hide resolved

dlurton commented May 23, 2022

View reviewed changes

Renames UniqueIdResolver to MetadataResolver

8383b55

dlurton mentioned this pull request May 23, 2022

Rename UniqueIdResolver to MetadataResolver #609

Merged

dlurton added 4 commits May 23, 2022 10:44

Merge branch 'physical-plan-staging-metadata-resolver' into physical-…

4a780a2

…plan-staging-planner-pipeline

emptySchemaResolver -> emptyMetadataResolver

7d84673

Merge branch 'physical-plan-staging-metadata-resolver' into physical-…

6f5d1ec

…plan-staging-planner-pipeline

Fix dependent code after merge

2dd95c7

dlurton changed the base branch from physical-plan-staging to physical-plan-staging-metadata-resolver May 23, 2022 18:47

dlurton requested a review from am357 May 23, 2022 18:49

dlurton self-assigned this May 23, 2022

Base automatically changed from physical-plan-staging-metadata-resolver to physical-plan-staging May 23, 2022 18:55

Account for name change

a2ced51

am357 reviewed May 24, 2022

View reviewed changes

dlurton added 2 commits May 24, 2022 08:54

Update comments

9f25f3f

Remove custom functions, procedures and types from Planner API

2cb4314

am357 approved these changes May 24, 2022

View reviewed changes

dlurton merged commit 694b660 into physical-plan-staging May 24, 2022

dlurton deleted the physical-plan-staging-planner-pipeline branch May 26, 2022 16:28

dlurton mentioned this pull request May 26, 2022

Merge physical plan evaluator and basic planner to main #612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PlannerPipeline #590

Add PlannerPipeline #590

dlurton commented Apr 30, 2022 •

edited

Loading

dlurton left a comment

am357 May 23, 2022

am357 May 23, 2022

dlurton May 24, 2022

am357 May 23, 2022

am357 May 23, 2022

am357 May 23, 2022

am357 May 23, 2022

dlurton May 24, 2022

am357 May 24, 2022

am357 May 23, 2022

am357 May 23, 2022

am357 May 23, 2022

Add PlannerPipeline #590

Add PlannerPipeline #590

Conversation

dlurton commented Apr 30, 2022 • edited Loading

dlurton left a 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

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

dlurton commented Apr 30, 2022 •

edited

Loading