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.

Sign up for GitHub

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

Jump to bottom

Merge physical plan evaluator and basic planner to main #612

Merged
merged 17 commits into from
May 27, 2022
Merged

Merge physical plan evaluator and basic planner to main #612

merged 17 commits into from
May 27, 2022

Conversation

dlurton
Copy link
Member

@dlurton dlurton commented May 26, 2022 •
edited
Loading

This will merge the following PRs to main:

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 added 10 commits May 9, 2022 10:35
@dlurton
Additional pipeline abstraction for test framework (#583)
517c9a4 
* Additional pipeline abstractions to share more code when testing the CompilerPipeline and future PlannerPipeline
@dlurton
Exclude planner & plan evaluator from tests for features not yet impl…
35f659e 
…emented (#586)
@dlurton
PIG domains for planner and plan evaluator (#584)
c57fdff
@dlurton
Make ThunkFactory generic (#587)
ec7d390
@dlurton
Query planner passes (#588)
77b522b 
* Add 3 query planner passes.

- AST->logical
- logical->resolved
- lesolved->physical.

Not yet integrated with anything--that will come in a future commit.
@dlurton
Add FILTER_DISTINCT function. (#589)
6fd6cc5
@dlurton
Rename UniqueIdResolver to MetadataResolver (#609)
d2b6ede
@dlurton
Add PlannerPipeline (#590)
694b660 
* Add 3 query planner passes.

- AST->logical
- logical->resolved
-resolved->physical.

Not yet integrated with anything--that will come in a future commit.
@dlurton
Add Plan Evaluator (#592)
e0a3519
@dlurton
Merge branch 'main' into physical-plan-staging
4a71241
@dlurton dlurton requested a review from am357 May 26, 2022 16:45
@am357 am357 requested review from alancai98 May 26, 2022 17:57
am357
am357 previously approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@am357 am357 left a comment

Choose a reason for hiding this comment

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

LGTM, all the incorporating PRs have already been reviewed by maintainers.

Please wait for the second reviewer approval before merge. We want to have the 2-person verification for this merge.

alancai98
alancai98 previously approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

I gave a cursory look at all the changes. The logic based on the documentation and prior discussions looks reasonable to me, and I think it can be merged to main. The comments I left were all minor and mostly dealt with documentation.

lang/src/org/partiql/lang/SqlException.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/SqlException.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/SqlException.kt
*/
open class SqlException(
override var message: String,
val errorCode: ErrorCode,
val errorContext: PropertyValueMap? = null,
errorContextArg: PropertyValueMap? = null,
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be nullable (i.e. what does a nullable errorContextArg define)? The errorContext definition on L49 could be redundant since a null errorContextArg will default to an empty property value map.

Copy link
Member Author

@dlurton dlurton May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's nullable because it always has been nullable. Changing this to an argument and defaulting the errorContext property to an empty map if errorContextArg is null was a way of simplifying error handling code that reads context properties without changing all of the call sites of this constructor (a few direct, but many indirect) to conform its new signature, which would have resulted in many more noisy diffs in this series of PRs. If you want, I can undo this change in another commit against this PR, but I would consider making this argument non-nullable to be out of scope of this work.

Copy link
Member

Choose a reason for hiding this comment

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

Ok got it. I don't think it needs to be addressed in this PR. Could you make an issue and link here regarding making errorContextArg non-nullable?

lang/src/org/partiql/lang/eval/builtins/DynamicLookupExprFunction.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/eval/physical/RelationThunk.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/planner/transforms/Util.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/planner/transforms/Util.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/planner/transforms/VariableIdAllocator.kt Outdated Show resolved Hide resolved
lang/test/org/partiql/lang/eval/EvaluatingCompilerOffsetTests.kt Outdated Show resolved Hide resolved
lang/test/org/partiql/lang/planner/transforms/AstToLogicalVisitorTransformTests.kt Outdated Show resolved Hide resolved
@dlurton dlurton requested a review from am357 May 27, 2022 20:46
@dlurton dlurton mentioned this pull request May 27, 2022
Open
alancai98
alancai98 previously approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback. Given @am357's previous approval and the fact that my feedback was minor (mostly related to documentation), I think we can merge this feature branch into main.

@johnedquinn johnedquinn mentioned this pull request May 27, 2022
Closed
@dlurton dlurton merged commit 8e32405 into main May 27, 2022
RCHowell pushed a commit that referenced this pull request Jun 1, 2022
@dlurton @RCHowell
Merge physical plan evaluator and basic planner to main (#612)
70a26cc 
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
@dlurton @RCHowell
Merge physical plan evaluator and basic planner to main (#612)
ad58199 
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
@dlurton @RCHowell
Merge physical plan evaluator and basic planner to main (#612)
29ece26 
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.
@alancai98 alancai98 deleted the physical-plan-staging branch June 3, 2022 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alancai98 alancai98 Awaiting requested review from alancai98

@am357 am357 Awaiting requested review from am357

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dlurton @alancai98 @am357