apply lists of rules recursively #570
Open
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
|
Changes Unknown when pulling 9ed2b5a on recursively_apply_rules_as_list into ** on master**. |
senderista
requested changes
Aug 16, 2017
| @@ -1731,6 +1732,8 @@ def insert_split_before_heavy(self, op): | |||
| encounter a heavyweight operator.""" | |||
| if isinstance(op, MyriaAlgebra.fragment_leaves): | |||
| return op | |||
| if isinstance(op, algebra.Split): | |||
| @@ -1524,7 +1524,8 @@ def add_hyper_shuffle(): | |||
| if not isinstance(expr, algebra.NaryJoin): | |||
| return expr | |||
| # check if HC shuffle has been placed before | |||
| shuffled_child = [isinstance(op, algebra.HyperCubeShuffle) | |||
| shuffled_child = [isinstance(op, algebra.HyperCubeShuffle) or | |||
| isinstance(op, algebra.OrderBy) | |||
There was a problem hiding this comment.
wouldn't this only happen if OrderByBeforeNaryJoin was somehow applied before HCShuffleBeforeNaryJoin?
| @@ -1638,7 +1638,7 @@ def __init__(self, array_rep, memory_scan_class=GrappaMemoryScan): | |||
|
|
|||
| def fire(self, expr): | |||
| if isinstance(expr, algebra.Scan) \ | |||
| and not isinstance(expr, GrappaFileScan): | |||
| and not isinstance(expr, cppcommon.CBaseFileScan): | |||
| @@ -2314,6 +2314,8 @@ class GrappaWhileCondition(rules.Rule): | |||
| result of the condition table""" | |||
|
|
|||
| def fire(self, expr): | |||
| if isinstance(expr, GrappaDoWhile): | |||
| expr = recursiverule(expr) | ||
|
|
||
| for rules in rules_lists: | ||
| for i in range(20): |
There was a problem hiding this comment.
If this lands in an infinite recursion, it'll blow the python stack soon enough, right? Do we really need an explicit cutoff?
| @@ -847,9 +833,6 @@ def __str__(self): | |||
| # For now, run twice and finish with PushApply. | |||
There was a problem hiding this comment.
remove obsolete comment
| accessed = sorted(set(itertools.chain(*(accessed_columns(e) | ||
| for e in emits)))) | ||
| new_cols = [child.output_columns[p] for p in accessed] |
There was a problem hiding this comment.
could you explain why you removed all this code?
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.
An algebra was defined as a list of lists of rules but then got flattened. This limits recursive optimizations that consist of multiple rules, for example, PushApply + RemoveUnusedColumn were repeated twice, which still produces redundant columns if the query contains more than two joins. This PR removes the flattening and applied a list of rules recursively until no change. It fixes the PushApply + RemoveUnusedColumn problem and also several bugs disclosed by this change.