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

[15721] Query Rewrite #1340

Closed
wants to merge 43 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zhaoguoquan94
Contributor

zhaoguoquan94 commented May 5, 2018

This PR adds support for query rewriting optimizations that are oblivious to the cost model in Peloton.

  • Transitive predicates (optimized in OptimizeLoop)
  • Constant folding (optimized in PostgresParser).

We implement query rewrite rules using the current optimizer framework:

  • Transitive predicates: we implemented this rule by adding a new step to the beginning OptimizerLoop using a TopDownRewriter. We added rules to take predicates from nodes, generating all possible predicates by taking a transitive closure and add the new ones;

  • Trivial predicate simplification: we used the same framework as in the transitive predicates rewrite. This method removes trivial predicates like a = a, some of which are generated by the transitive predicates rule;

Constant folding can transform queries like select * from table where t.column1 = 3*(2-1) to select * from table where t.column1 = 3. These transformation are done in parser level instead of optimizer level because of implementation simplicity. And we don't really lose any information.
It support 3 type of constant folding

  1. Operators like +-*/
  2. Comparison operators like >=, NOT, is null,
  3. Conjunctions : AND OR.

@zhaoguoquan94 zhaoguoquan94 changed the title from [15721] Expression Rewrite to [15721] Query Rewrite May 5, 2018

gangsterveggies and others added some commits May 4, 2018

This commit should failed. Commit it because we need to see whether t…
…he addressSanitizer's output are marked with correct line number or not.
Revert "This commit should failed. Commit it because we need to see w…
…hether the addressSanitizer's output are marked with correct line number or not."

try to make address sanitizer report something.

This reverts commit 00020ce.
return false;
}
auto vcl = children_[0].get();

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

it isn't clear exactly what vcl stands for. would recommend spelling it out or at least adding a comment. same applies to the other acronyms below

throw Exception("Invalid comparison expression type.");
}
} catch (std::exception &e) {
return false;

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

you should log out the exception here unless if you follow my proposal earlier

return exp_type_ == oexp_type_ && ocl->ExactlyEquals(*vcl) &&
ocr->ExactlyEquals(*vcr);
default:
throw Exception("Invalid comparison expression type.");

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

if you are simply catching the exception you throw here, you should probably just return false in default and log information that there was an invalid comparison expression type. If you want the exception to cause a failure, then you can throw but would recommend not catching here. If you are expecting another exception from this try block, then I'd recommend picking a finer grained exception in the catch statement.

void Transform(std::shared_ptr<OperatorExpression> input,
std::vector<std::shared_ptr<OperatorExpression>> &transformed,
OptimizeContext *context) const override;

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

i think OptimizeContext is usually passed around as a shared_ptr in other parts of the optimizer. you may want to do that for consistency and to gain the benefits of it

public:
TransitivePredicatesLogicalGet();
bool Check(std::shared_ptr<OperatorExpression> plan,

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

as you are declaring the same methods here and below for all of these classes, i'd recommend creating an abstract class that inherits from Rule so that you don't need to duplicate the declaration of the same Check and Transform methods

match_pattern = std::make_shared<Pattern>(OpType::Get);
}
bool TransitivePredicatesLogicalGet::Check(std::shared_ptr<OperatorExpression> input,

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

add a TODO for what you actually want to do here. same for the other Check methods that do this

@@ -23,6 +26,145 @@ namespace peloton {
namespace optimizer {
namespace util {
void FillTransitiveTable(

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

i think this method should have some inline documentation about what is happening here

for (auto predicate : predicates) {
std::string exp_type = ExpressionTypeToString(predicate.expr->GetExpressionType());
if (exp_type.substr(0, 7) == "COMPARE") {

This comment has been minimized.

@nappelson

nappelson May 11, 2018

Contributor

i'd try and avoid using magic number (7 in this case)

This comment has been minimized.

@GustavoAngulo

GustavoAngulo May 13, 2018

Member

Agreed, you could probably do it like how it is done here

@GustavoAngulo

Overall looks great, just some comments and clarifications here and there.

// Apply query rewrite rules
task_stack->Push(new TopDownRewrite(root_group_id, root_context,
RewriteRuleSetName::SIMPLIFY_PREDICATES));

This comment has been minimized.

@GustavoAngulo

GustavoAngulo May 13, 2018

Member

Why is simplifying predicates executed after transitive predicates? Will this not apply the transitive predicate rule on trivial predicates that will just get thrown out by the simplify predicate rule? Seems like we can save some work by flipping the order of these

TransitivePredicatesLogicalFilter::TransitivePredicatesLogicalFilter() {
type_ = RuleType::TRANSITIVE_PREDICATES_LOGICAL_FILTER;
std::shared_ptr<Pattern> child(std::make_shared<Pattern>(OpType::InnerJoin));

This comment has been minimized.

@GustavoAngulo

GustavoAngulo May 13, 2018

Member

Why is the child for this a join and not a leaf? Is there a reason we should only apply this to filters with joins as children?

SimplifyPredicatesLogicalFilter::SimplifyPredicatesLogicalFilter() {
type_ = RuleType::SIMPLIFY_PREDICATES_LOGICAL_FILTER;
std::shared_ptr<Pattern> child(std::make_shared<Pattern>(OpType::InnerJoin));

This comment has been minimized.

@GustavoAngulo

GustavoAngulo May 13, 2018

Member

Same comment as in earlier LogicalFilter rule, is there a reason for this to be an InnerJoin and not a leaf?

for (auto predicate : predicates) {
std::string exp_type = ExpressionTypeToString(predicate.expr->GetExpressionType());
if (exp_type.substr(0, 7) == "COMPARE") {

This comment has been minimized.

@GustavoAngulo

GustavoAngulo May 13, 2018

Member

Agreed, you could probably do it like how it is done here

}
}
return new_predicates;

This comment has been minimized.

@GustavoAngulo

GustavoAngulo May 13, 2018

Member

Do we ever add the original predicate to the new_predicates list?

@apavlo

This comment has been minimized.

Member

apavlo commented Jun 21, 2018

Closing. We will have @Zeninma build this in the fall with a proper rewrite layer.

@apavlo apavlo closed this Jun 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment