Make ThunkFactory generic #587
Merged
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
…CompilerPipeline and future PlannerPipeline
dlurton
force-pushed
the
physical-plan-staging-thunk
branch
from
57641ab to
afce4bc
Compare
Merged
4 tasks
alancai98
reviewed
May 6, 2022
| @@ -28,8 +28,10 @@ import org.partiql.lang.errors.Property | |||
| * See https://en.wikipedia.org/wiki/Thunk | |||
| * | |||
| * This name was chosen because it is a thunk that accepts an instance of `Environment`. | |||
There was a problem hiding this comment.
Comment may need updated since Environment was made generic. Similarly on L41.
There was a problem hiding this comment.
I think I can probably just delete these lines--I don't think they added much.
| @@ -43,10 +44,11 @@ import org.partiql.lang.errors.UNKNOWN | |||
| open class SqlException( | |||
| override var message: String, | |||
| val errorCode: ErrorCode, | |||
| val errorContext: PropertyValueMap? = null, | |||
| errorContextArg: PropertyValueMap? = null, | |||
There was a problem hiding this comment.
kdoc for this parameter needs to be updated.
Base automatically changed from
physical-plan-staging-pig-domains
to
physical-plan-staging
May 10, 2022 17:21
alancai98
reviewed
May 10, 2022
There was a problem hiding this comment.
Changes since last commit look good other than the one comment in EvaluatingCompilerOffsetTests.kt.
| tc.copy(excludeLegacySerializerAssertions = true), | ||
| tc.copy( | ||
| excludeLegacySerializerAssertions = true, | ||
| targetPipeline = EvaluatorTestTarget.COMPILER_PIPELINE, // planner & phys. alg. have no support for OFFSET (yet) |
There was a problem hiding this comment.
From your comment in the PR that set the test exclusions, #586, you left a comment saying that this comment was wrong. Perhaps there was some missed commit in the merge?
(The original version was just tc.copy(excludeLegacySerializerAssertions = true) and the exclusions were set by the individual EvaluatorTestCases.)
There was a problem hiding this comment.
Oh, yeah, it must have returned after a merge. Will fix. Thanks.
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.
Originally part of #582, this PR is based on #584 (which must be merged first).
The legacy AST evaluator's
ThunkFactoryand related types were tightly coupled to theorg.partiql.lang.eval.Environmentclass. However, the new plan evaluator replaces this class with a different one. This PR makesThunkFactoryand related types generic so that a different environment class may be used with the physical plan evaluator without having to duplicateThunkFactoryet al.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.