[V1] Adds basic strategies to compiler (see #1625) #1644
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
RCHowell
commented
Nov 5, 2024
Comment on lines
+29
to
+49
| /** | ||
| * Get the i-th operator (pre-order) matched by the pattern. | ||
| * | ||
| * @param i 0-indexed | ||
| * @return Operator | ||
| */ | ||
| @NotNull | ||
| public Operator operator(int i) { | ||
| return operators[i]; | ||
| } | ||
|
|
||
| /** | ||
| * Get the i-th input to this pattern. | ||
| * | ||
| * @param i 0-indexed | ||
| * @return Expr | ||
| */ | ||
| @NotNull | ||
| public List<Expr> children(int i) { | ||
| return children.get(i); | ||
| } |
Comment on lines
+13
to
+14
| @NotNull | ||
| private final Pattern pattern; |
There was a problem hiding this comment.
self-review: since this is an abstract class, it might be worth making the field public and removing the getter.
Comment on lines
+174
to
+179
| if (strategy.getPattern().matches(operator)) { | ||
| // compile children | ||
| val children = operator.getChildren().map { compileWithStrategies(it, ctx) } | ||
| val match = Match(operator, children) | ||
| return strategy.apply(match) | ||
| } |
There was a problem hiding this comment.
self-review: this shows how we can recurse to compile the children, then include those in the match object.
Comment on lines
+10
to
+17
| /** | ||
| * Get i-th child (input) operator. | ||
| * | ||
| * @param index | ||
| */ | ||
| public fun getChild(index: Int) { | ||
| throw UnsupportedOperationException("getChild") | ||
| } |
There was a problem hiding this comment.
self-review: get i-th child is crucial to building the match structures but is unused now.
Comment on lines
+714
to
+722
| /** | ||
| * @return a PartiQL row type | ||
| * @deprecated this API is experimental and is subject to modification/deletion without prior notice. | ||
| */ | ||
| @NotNull | ||
| static PType row(@NotNull Field... fields) { | ||
| return new PTypeRow(Arrays.asList(fields)); | ||
| } | ||
|
|
There was a problem hiding this comment.
self-review: auto-formatting messed with some things. All I've done is added the row helper.
CROSS-ENGINE-REPORT ❌
Testing Details
Result Details
Now FAILING Tests ❌The following 3 test(s) were previously PASSING in BASE but are now FAILING in TARGET: Click here to see
Now IGNORED Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. Now Passing Tests180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-0A305C4). Before merging, confirm they are intended to pass. The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. CROSS-COMMIT-REPORT ✅
Testing DetailsResult Details
|
RCHowell
commented
Nov 5, 2024
| import java.util.List; | ||
|
|
||
| /** | ||
| * Match represents a subtree match to be sent to the |
There was a problem hiding this comment.
Suggested change
| * Match represents a subtree match to be sent to the | |
| * Match represents a subtree match to be sent to a strategy. |
johnedquinn
approved these changes
Nov 5, 2024
| val input = "select * from [1,2] limit 100" | ||
| compile(input, strategy) | ||
| // assert the strategy was triggered | ||
| assertTrue(trigged, "the compiler did not apply the custom strategy") |
There was a problem hiding this comment.
Non-blocking nit.
Suggested change
| assertTrue(trigged, "the compiler did not apply the custom strategy") | |
| assertTrue(trigged, "the compiler applied the custom strategy") |
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.
Relevant Issues
#1625
Description
This PR defines a single node pattern along with the strategy API. This allows you to define a "strategy" or hook to insert custom implementations of certain nodes.
Other Information
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.