Query planner passes #588

dlurton · 2022-04-29T22:11:38Z

Originally part of #582, this commit is based on #587, which must be merged first. However, it may be helpful to review the PR introducing the PlannerPipeline first (#590), if more context for these changes is desired.

This PR contains 3 query planner passes:

Convert AST to logical plan
Logical plan -> Resolved logical plan. This pass assigns indexes to all local variables, and resolves references to global variables. (undefined variables are detected and reported here).
Resolved logical plan -> physical plan

These are not yet integrated with anything--that's in #590.

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

lang/src/org/partiql/lang/planner/transforms/AstToLogicalVisitorTransform.kt

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

lziq

Just finished review for AstToLogicalVisitorTransform.kt and related test files. Generally, it looks good to me. Will continue to review rest of the changes.

lang/src/org/partiql/lang/planner/transforms/AstToLogicalVisitorTransform.kt

lang/test/org/partiql/lang/planner/transforms/AstToLogicalVisitorTransformTests.kt

am357

Had a review and left some comments w/ requesting code changes if they make sense.

am357 · 2022-05-13T21:11:43Z

lang/src/org/partiql/lang/domains/util.kt

+// Workaround for a bug in PIG that is fixed in its next release:
+// https://github.com/partiql/partiql-ir-generator/issues/41
+fun List<IonElement>.asAnyElement() =
+    this.map { it.asAnyElement() }


While at it, shouldn't we just release the PIG and remove this?

Yes, but partiql/partiql-ir-generator#41 hasn't been fixed yet.

am357 · 2022-05-13T21:16:01Z

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

+import org.partiql.lang.eval.BindingCase
+import org.partiql.lang.eval.BindingName
+
+/** Indicates the result of an attempt to resolve a global binding. */


I understand we have kdoc for GlobalBindings.resolve() but can we also add a definition for Global Binding here?

BTW.
I think it's helpful to have the link to the upcoming documentation in applicable constructs under planner.

I have rephrased this to be more clear.

am357 · 2022-05-13T21:24:42Z

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

+    /**
+     * A success case, indicates the [index] of the only possible match to the [BindingName] in a local lexical scope.
+     * This is `internal` because [index] is an implementation detail that shouldn't be accessible outside of this
+     * library.
+     */
+    internal data class LocalVariable(val index: Int) : ResolutionResult()


Considering that GlobalBindings.resolve() is meant to return GlobalVariable or undefined, should this be here?

It's internal, so it's not exposed to customers. Customer code is never intended to resolve local variables, so this fits the situation. Having this here does simplify a few things in relation in the LogicalToLogicalResolvedVisitorTransform class. Namely, I found this to be preferable to having two variations of this sealed class: one that is public and only has GlobalVariable and Undefined, and another that is private and has GlobalVariable, Undefined as well as LocalVariable. There would also need to be logic to convert from the public variation to the private.

Making one member of a public sealed class internal is perhaps a little unusual, but unusual doesn't equate to undesirable, necessarily. Was there a specific concern you had with this approach?

I wasn't worried about exposing it, I just thought this data class doesn't belong here. I think your explanation makes sense, so let's leave it as is.

am357 · 2022-05-13T21:25:33Z

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

+    data class Success<TResult>(val result: TResult, val warnings: List<Problem>) : PassResult<TResult>()
+
+    /**
+     * Indicates query planning was not successful and includes a list of errors and warnings that were encountered


errors and warnings -> errors?

This is designed to allow us to continue at least the current pass in the event of a semantic error instead of throwing an exception. That means its entirely possible to encounter both warnings and errors. I'll add a note.

lang/src/org/partiql/lang/planner/transforms/LogicalResolvedToPhysicalVisitorTransform.kt

am357 · 2022-05-13T21:38:35Z

lang/src/org/partiql/lang/planner/transforms/LogicalToLogicalResolvedVisitorTransform.kt

+ * we keep going to provide the end user any additional error messaging, unless [ProblemHandler.handleProblem] throws
+ * an exception when an error is logged.  **If any undefined variables are detected, in order to allow traversal to
+ * continue, a fake index value is used in place of a real one and the resolved logical plan returned by this function
+ * is guaranteed to be invalid.** **Therefore, it is the responsibility therefore of callers to check if any problems


A redundant _therefore`?

Out of order, actually. Fixing... EDIT: nope, redundant. Removing.

also added a couple dozen lines about $__dynamic_lookup__

am357 · 2022-05-13T21:48:30Z

lang/src/org/partiql/lang/planner/transforms/Util.kt

+/**
+ * This is the version number of the logical and physical plans supported by this version of PartiQL.
+ *
+ * It would be nice to embed this in the PIG domain somehow, but this isn't supported, so we have to include it


Didn't we introduce the version number in the new domains. I'm sure you feel the same way, but this is not great. Even if going with this hard-coding, the version doesn't tell anything about backward compatibility; shouldn't we adopt a format like SemVer?

I really think this aught to be something solved by PIG actually so I created partiql/partiql-ir-generator#121

I would like to leave this as-is for now if you don't mind.

I'll add a link to the ticket in the comment.

Do you also mind adding some comments to

(record plan (stmt statement) (version int) )

in the partiql.ion file, to state that the version number here will be removed and assigned to PIG domains once partiql/partiql-ir-generator#121 is resolved?

I would like to leave this as-is for now if you don't mind.

I think this is something that we need to fix before we expose the API; once we have the version as is and the API is exposed, then it's too late. Happy to discuss further offline.

Offline discussion indicates this doesn't need to be addressed in this PR, but should be addressed before this hits main. Creating an issue in the related milestone to track this: #605

Just an update on this; as a comprise, requested having the version as SemVer hard-coded for now. @dlurton had a point on using patch—which is for bug fixes—might not be applicable to our use-case. We ultimately agreed on having major and minor notions in the version for now.

am357 · 2022-05-13T21:49:56Z

lang/src/org/partiql/lang/planner/transforms/VariableIdAllocator.kt

+    return planWithAllocatedVariables to allLocals.toList()
+}
+
+private const val VARIABLE_ID_META_TAG = "\$variable_id"


Nit.: I'm thinking if we need to get all the constants in one place under planner.

In general, I'd rather not have one file with a bunch of unrelated constants. Note that this is private, putting it somewhere else means it can't be private anymore and will pollute the package namespace. Putting the constant here also has the advantage that the definition is close to its uses, which improves readability IMHO.

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

lang/test/org/partiql/lang/planner/transforms/LogicalToLogicalResolvedVisitorTransformTests.kt

lziq

Just finished the review. Overall it looks good to me.

lziq · 2022-05-16T23:53:16Z

lang/src/org/partiql/lang/planner/transforms/Util.kt

+/**
+ * This is the version number of the logical and physical plans supported by this version of PartiQL.
+ *
+ * It would be nice to embed this in the PIG domain somehow, but this isn't supported, so we have to include it


Do you also mind adding some comments to

(record plan (stmt statement) (version int) )

in the partiql.ion file, to state that the version number here will be removed and assigned to PIG domains once partiql/partiql-ir-generator#121 is resolved?

lang/src/org/partiql/lang/planner/transforms/LogicalResolvedToPhysicalVisitorTransform.kt

lziq · 2022-05-19T18:39:46Z

The new commit makes sense to me and there is no more file change requested from my side. If @am357 also agrees, I think we can merge this PR.

@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 force-pushed the physical-plan-staging-planner-passes branch from 9310e4a to 41e6384 Compare April 29, 2022 22:33

This was referenced Apr 30, 2022

[DRAFT] Experimental planner and physical plan evaluator #582

Closed

Add PlannerPipeline #590

Merged

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 FILTER_DISTINCT function. #589

Merged

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

dlurton commented May 9, 2022

View reviewed changes

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

Base automatically changed from physical-plan-staging-thunk to physical-plan-staging May 10, 2022 22:26

Add 3 query planner passes.

48a003e

- 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-passes branch from ac6155a to 48a003e Compare May 10, 2022 23:03

dlurton added 2 commits May 10, 2022 16:44

Address build failures

a5bd78b

Remove uneeded (lit bool)

1a36cb2

dlurton requested review from alancai98, lziq and am357 and removed request for alancai98 May 11, 2022 16:18

lziq reviewed May 13, 2022

View reviewed changes

lang/src/org/partiql/lang/planner/transforms/AstToLogicalVisitorTransform.kt Show resolved Hide resolved

lang/test/org/partiql/lang/planner/transforms/AstToLogicalVisitorTransformTests.kt Show resolved Hide resolved

am357 requested changes May 13, 2022

View reviewed changes

lziq reviewed May 13, 2022

View reviewed changes

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

dlurton commented May 14, 2022

View reviewed changes

lang/test/org/partiql/lang/planner/transforms/LogicalToLogicalResolvedVisitorTransformTests.kt Show resolved Hide resolved

dlurton added 2 commits May 16, 2022 12:55

Apply feedback from Arash and Ziqi and myself

0bcb4e6

Make ktlint happy

cb8ebcb

dlurton requested review from am357 and lziq May 16, 2022 20:58

lziq reviewed May 16, 2022

View reviewed changes

dlurton mentioned this pull request May 17, 2022

In physical-plan-staging utilize partiql-ir-generator#121 or implement other work-around #605

Open

Rename to LogicalResolvedToPhysicalVisitorTransform

f5557b7

Change version to string, make semantic

69d1835

am357 approved these changes May 19, 2022

View reviewed changes

dlurton merged commit 77b522b into physical-plan-staging May 19, 2022

dlurton deleted the physical-plan-staging-planner-passes branch May 19, 2022 20:51

This was referenced May 23, 2022

Rename UniqueIdResolver to MetadataResolver #609

Merged

Merge physical plan evaluator and basic planner to main #612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query planner passes #588

Query planner passes #588

dlurton commented Apr 29, 2022 •

edited

Loading

lziq left a comment •

edited

Loading

am357 left a comment

am357 May 13, 2022

dlurton May 13, 2022

am357 May 13, 2022

dlurton May 16, 2022

am357 May 13, 2022

dlurton May 13, 2022 •

edited

Loading

am357 May 17, 2022

am357 May 13, 2022

dlurton May 13, 2022 •

edited

Loading

am357 May 13, 2022

dlurton May 13, 2022 •

edited

Loading

dlurton May 13, 2022

am357 May 13, 2022

dlurton May 13, 2022 •

edited

Loading

dlurton May 16, 2022 •

edited

Loading

lziq May 16, 2022

am357 May 17, 2022

dlurton May 17, 2022 •

edited

Loading

am357 May 19, 2022

am357 May 13, 2022

dlurton May 13, 2022

lziq left a comment

lziq May 16, 2022

lziq commented May 19, 2022 •

edited

Loading

Query planner passes #588

Query planner passes #588

Conversation

dlurton commented Apr 29, 2022 • edited Loading

lziq left a comment • edited Loading

Choose a reason for hiding this comment

am357 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

dlurton May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlurton May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlurton May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlurton May 13, 2022 • edited Loading

Choose a reason for hiding this comment

dlurton May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlurton May 17, 2022 • edited Loading

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

lziq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lziq commented May 19, 2022 • edited Loading

dlurton commented Apr 29, 2022 •

edited

Loading

lziq left a comment •

edited

Loading

dlurton May 13, 2022 •

edited

Loading

dlurton May 13, 2022 •

edited

Loading

dlurton May 13, 2022 •

edited

Loading

dlurton May 13, 2022 •

edited

Loading

dlurton May 16, 2022 •

edited

Loading

dlurton May 17, 2022 •

edited

Loading

lziq commented May 19, 2022 •

edited

Loading