-
Notifications
You must be signed in to change notification settings - Fork 61
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.
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
Merge physical plan evaluator and basic planner to main
#612
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
517c9a4
Additional pipeline abstraction for test framework (#583)
dlurton 35f659e
Exclude planner & plan evaluator from tests for features not yet impl…
dlurton c57fdff
PIG domains for planner and plan evaluator (#584)
dlurton ec7d390
Make ThunkFactory generic (#587)
dlurton 77b522b
Query planner passes (#588)
dlurton 6fd6cc5
Add FILTER_DISTINCT function. (#589)
dlurton d2b6ede
Rename `UniqueIdResolver` to `MetadataResolver` (#609)
dlurton 694b660
Add PlannerPipeline (#590)
dlurton e0a3519
Add Plan Evaluator (#592)
dlurton 4a71241
Merge branch 'main' into physical-plan-staging
dlurton ea40e4a
Add TODO
dlurton 34025b3
Make new aggregate tests target CompilerPipeline
dlurton 49e777d
Update lang/src/org/partiql/lang/SqlException.kt
dlurton 192a84f
Apply PR feedback
dlurton 0b97399
Apply PR feedback from Alan
dlurton 209175e
Apply more feedback / fix typos
dlurton 924e01a
add todo
dlurton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
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 nullerrorContextArg
will default to an empty property value map.There was a problem hiding this comment.
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 iferrorContextArg
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.There was a problem hiding this comment.
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?