From 8e194429c13cd56c55d0b6fa3aa7bef72078a491 Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Tue, 6 Dec 2022 14:50:07 +0100 Subject: [PATCH 01/13] adding operator pool class. But first going to look at what happens to the pointers --- .../duckdb/optimizer/operator_pool.hpp | 25 +++++++++++++++++++ src/optimizer/operator_pool.cpp | 15 +++++++++++ src/optimizer/optimizer.cpp | 4 ++- 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 src/include/duckdb/optimizer/operator_pool.hpp create mode 100644 src/optimizer/operator_pool.cpp diff --git a/src/include/duckdb/optimizer/operator_pool.hpp b/src/include/duckdb/optimizer/operator_pool.hpp new file mode 100644 index 000000000000..437d5aba2894 --- /dev/null +++ b/src/include/duckdb/optimizer/operator_pool.hpp @@ -0,0 +1,25 @@ +//===----------------------------------------------------------------------===// +// DuckDB +// +// duckdb/optimizer/operator_pool.hpp +// +// +//===----------------------------------------------------------------------===// + +namespace duckdb { + +struct LogicalOperatorIdentifier { + LogicalOperatorType type; + string name; + +}; + +class OperatorPool { +public: + void AddOperator(unique_ptr op); + bool InPool(unique_ptr op); + +private: + unordered_set seen_operators; +}; +} \ No newline at end of file diff --git a/src/optimizer/operator_pool.cpp b/src/optimizer/operator_pool.cpp new file mode 100644 index 000000000000..ac0062e0f249 --- /dev/null +++ b/src/optimizer/operator_pool.cpp @@ -0,0 +1,15 @@ +// +// Created by Tom Ebergen on 06/12/2022. +// + +namespace duckdb { + +void OperatorPool::AddOperator(unique_ptr op) { + +} + +bool OperatorPool::InPool(unique_ptr op) { + return false; +} + +} \ No newline at end of file diff --git a/src/optimizer/optimizer.cpp b/src/optimizer/optimizer.cpp index 0641a5ab0eeb..4a87013d7d92 100644 --- a/src/optimizer/optimizer.cpp +++ b/src/optimizer/optimizer.cpp @@ -74,7 +74,9 @@ unique_ptr Optimizer::Optimize(unique_ptr plan this->plan = move(plan_p); // first we perform expression rewrites using the ExpressionRewriter // this does not change the logical plan structure, but only simplifies the expression trees - RunOptimizer(OptimizerType::EXPRESSION_REWRITER, [&]() { rewriter.VisitOperator(*plan); }); + RunOptimizer(OptimizerType::EXPRESSION_REWRITER, [&]() { + rewriter.VisitOperator(*plan); + }); // perform filter pullup RunOptimizer(OptimizerType::FILTER_PULLUP, [&]() { From ae000bf24cd5f5e734836dbeb99fbbbf4261d67b Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Tue, 6 Dec 2022 15:24:46 +0100 Subject: [PATCH 02/13] added a class to keep a pool of logical operator pointers. They are unique though, so dont understand how they should be copied --- .../duckdb/optimizer/filter_pushdown.hpp | 2 ++ src/include/duckdb/optimizer/operator_pool.hpp | 17 ++++++++++------- src/optimizer/CMakeLists.txt | 1 + src/optimizer/operator_pool.cpp | 11 ++++++++--- 4 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/include/duckdb/optimizer/filter_pushdown.hpp b/src/include/duckdb/optimizer/filter_pushdown.hpp index ee4d40e7d365..aa6a32b41f2b 100644 --- a/src/include/duckdb/optimizer/filter_pushdown.hpp +++ b/src/include/duckdb/optimizer/filter_pushdown.hpp @@ -11,6 +11,7 @@ #include "duckdb/common/unordered_set.hpp" #include "duckdb/optimizer/filter_combiner.hpp" #include "duckdb/optimizer/rule.hpp" +#include "duckdb/optimizer/operator_pool.hpp" namespace duckdb { @@ -37,6 +38,7 @@ class FilterPushdown { private: vector> filters; Optimizer &optimizer; + OperatorPool seen_operators; //! Push down a LogicalAggregate op unique_ptr PushdownAggregate(unique_ptr op); diff --git a/src/include/duckdb/optimizer/operator_pool.hpp b/src/include/duckdb/optimizer/operator_pool.hpp index 437d5aba2894..5874928851bb 100644 --- a/src/include/duckdb/optimizer/operator_pool.hpp +++ b/src/include/duckdb/optimizer/operator_pool.hpp @@ -6,20 +6,23 @@ // //===----------------------------------------------------------------------===// -namespace duckdb { +#pragma once -struct LogicalOperatorIdentifier { - LogicalOperatorType type; - string name; +#include "duckdb/planner/logical_operator.hpp" +#include "duckdb/common/unordered_set.hpp" -}; +namespace duckdb { class OperatorPool { public: - void AddOperator(unique_ptr op); + OperatorPool() { + seen_operators = unordered_set>(); + } + + unique_ptr AddOperator(unique_ptr op); bool InPool(unique_ptr op); private: - unordered_set seen_operators; + unordered_set> seen_operators; }; } \ No newline at end of file diff --git a/src/optimizer/CMakeLists.txt b/src/optimizer/CMakeLists.txt index f67158abfa0e..7f7ce3f68dd1 100644 --- a/src/optimizer/CMakeLists.txt +++ b/src/optimizer/CMakeLists.txt @@ -18,6 +18,7 @@ add_library_unity( filter_pullup.cpp in_clause_rewriter.cpp optimizer.cpp + operator_pool.cpp expression_rewriter.cpp regex_range_filter.cpp remove_unused_columns.cpp diff --git a/src/optimizer/operator_pool.cpp b/src/optimizer/operator_pool.cpp index ac0062e0f249..e6229bd9f876 100644 --- a/src/optimizer/operator_pool.cpp +++ b/src/optimizer/operator_pool.cpp @@ -2,14 +2,19 @@ // Created by Tom Ebergen on 06/12/2022. // -namespace duckdb { +#include "duckdb/optimizer/operator_pool.hpp" -void OperatorPool::AddOperator(unique_ptr op) { +namespace duckdb { +unique_ptr OperatorPool::AddOperator(unique_ptr op) { + D_ASSERT(!InPool(op)); + seen_operators.insert(op); + return move(op); } bool OperatorPool::InPool(unique_ptr op) { - return false; + auto it = seen_operators.find(op); + return it != seen_operators.end(); } } \ No newline at end of file From fbe7ed84f582eb0d7683e00dd9e9dbd44387f858 Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Tue, 6 Dec 2022 16:12:35 +0100 Subject: [PATCH 03/13] this should be the fix for filter pushdown --- benchmark/micro/join/many_left_joins.benchmark | 2 -- src/include/duckdb/optimizer/operator_pool.hpp | 8 ++++---- src/optimizer/filter_pushdown.cpp | 9 ++++++++- src/optimizer/operator_pool.cpp | 9 ++++----- src/optimizer/pushdown/pushdown_left_join.cpp | 3 +++ 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/benchmark/micro/join/many_left_joins.benchmark b/benchmark/micro/join/many_left_joins.benchmark index d1da889c0dbf..d89926f1c92f 100644 --- a/benchmark/micro/join/many_left_joins.benchmark +++ b/benchmark/micro/join/many_left_joins.benchmark @@ -111,5 +111,3 @@ left join t5 ttt7 on (ttt6.c6=ttt7.c7) inner join t3 on (t2.c2=t3.c1) left join t4 tt4 on (t47.c1=tt4.c3) left join t4 tt5 on (tt4.c4=tt5.c5); - -result I \ No newline at end of file diff --git a/src/include/duckdb/optimizer/operator_pool.hpp b/src/include/duckdb/optimizer/operator_pool.hpp index 5874928851bb..f8a3359f29b5 100644 --- a/src/include/duckdb/optimizer/operator_pool.hpp +++ b/src/include/duckdb/optimizer/operator_pool.hpp @@ -16,13 +16,13 @@ namespace duckdb { class OperatorPool { public: OperatorPool() { - seen_operators = unordered_set>(); + seen_operators = unordered_set(); } - unique_ptr AddOperator(unique_ptr op); - bool InPool(unique_ptr op); + void AddOperator(LogicalOperator *op); + bool InPool(LogicalOperator *op); private: - unordered_set> seen_operators; + unordered_set seen_operators; }; } \ No newline at end of file diff --git a/src/optimizer/filter_pushdown.cpp b/src/optimizer/filter_pushdown.cpp index b8b51830ab79..5f0ce5a00f34 100644 --- a/src/optimizer/filter_pushdown.cpp +++ b/src/optimizer/filter_pushdown.cpp @@ -5,14 +5,21 @@ #include "duckdb/planner/operator/logical_join.hpp" #include "duckdb/optimizer/optimizer.hpp" +#include "iostream" + namespace duckdb { using Filter = FilterPushdown::Filter; -FilterPushdown::FilterPushdown(Optimizer &optimizer) : optimizer(optimizer), combiner(optimizer.context) { +FilterPushdown::FilterPushdown(Optimizer &optimizer) : optimizer(optimizer), seen_operators(), combiner(optimizer.context) { } unique_ptr FilterPushdown::Rewrite(unique_ptr op) { + if (seen_operators.InPool(op.get())) { + // We've already optimized this operator, so just return. + return move(op); + } + seen_operators.AddOperator(op.get()); D_ASSERT(!combiner.HasFilters()); switch (op->type) { case LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY: diff --git a/src/optimizer/operator_pool.cpp b/src/optimizer/operator_pool.cpp index e6229bd9f876..b227c9c339e3 100644 --- a/src/optimizer/operator_pool.cpp +++ b/src/optimizer/operator_pool.cpp @@ -6,14 +6,13 @@ namespace duckdb { -unique_ptr OperatorPool::AddOperator(unique_ptr op) { +void OperatorPool::AddOperator(LogicalOperator *op) { D_ASSERT(!InPool(op)); - seen_operators.insert(op); - return move(op); + seen_operators.insert((idx_t)op); } -bool OperatorPool::InPool(unique_ptr op) { - auto it = seen_operators.find(op); +bool OperatorPool::InPool(LogicalOperator *op) { + auto it = seen_operators.find((idx_t)op); return it != seen_operators.end(); } diff --git a/src/optimizer/pushdown/pushdown_left_join.cpp b/src/optimizer/pushdown/pushdown_left_join.cpp index d5b88db33671..be23f18a2b17 100644 --- a/src/optimizer/pushdown/pushdown_left_join.cpp +++ b/src/optimizer/pushdown/pushdown_left_join.cpp @@ -8,6 +8,8 @@ #include "duckdb/planner/operator/logical_comparison_join.hpp" #include "duckdb/planner/operator/logical_filter.hpp" +#include "iostream" + namespace duckdb { using Filter = FilterPushdown::Filter; @@ -120,6 +122,7 @@ unique_ptr FilterPushdown::PushdownLeftJoin(unique_ptrchildren[0] = left_pushdown.Rewrite(move(op->children[0])); op->children[1] = right_pushdown.Rewrite(move(op->children[1])); if (filters.empty()) { From 9515c5f100111ba34153bd2a4edd67559cfdc18c Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Wed, 7 Dec 2022 12:54:48 +0100 Subject: [PATCH 04/13] make format fix and reorganization --- src/include/duckdb/optimizer/operator_pool.hpp | 2 +- src/optimizer/operator_pool.cpp | 6 +++++- src/optimizer/optimizer.cpp | 7 +++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/include/duckdb/optimizer/operator_pool.hpp b/src/include/duckdb/optimizer/operator_pool.hpp index f8a3359f29b5..b47f653b1f89 100644 --- a/src/include/duckdb/optimizer/operator_pool.hpp +++ b/src/include/duckdb/optimizer/operator_pool.hpp @@ -25,4 +25,4 @@ class OperatorPool { private: unordered_set seen_operators; }; -} \ No newline at end of file +} // namespace duckdb diff --git a/src/optimizer/operator_pool.cpp b/src/optimizer/operator_pool.cpp index b227c9c339e3..7088cf3343db 100644 --- a/src/optimizer/operator_pool.cpp +++ b/src/optimizer/operator_pool.cpp @@ -16,4 +16,8 @@ bool OperatorPool::InPool(LogicalOperator *op) { return it != seen_operators.end(); } -} \ No newline at end of file +void OperatorPool::EmptyOperatorPool() { + seen_operators.clear(); +} + +} // namespace duckdb diff --git a/src/optimizer/optimizer.cpp b/src/optimizer/optimizer.cpp index 4a87013d7d92..f565c49efc8b 100644 --- a/src/optimizer/optimizer.cpp +++ b/src/optimizer/optimizer.cpp @@ -26,7 +26,8 @@ namespace duckdb { -Optimizer::Optimizer(Binder &binder, ClientContext &context) : context(context), binder(binder), rewriter(context) { +Optimizer::Optimizer(Binder &binder, ClientContext &context) + : context(context), binder(binder), rewriter(context), seen_operators() { rewriter.rules.push_back(make_unique(rewriter)); rewriter.rules.push_back(make_unique(rewriter)); rewriter.rules.push_back(make_unique(rewriter)); @@ -74,9 +75,7 @@ unique_ptr Optimizer::Optimize(unique_ptr plan this->plan = move(plan_p); // first we perform expression rewrites using the ExpressionRewriter // this does not change the logical plan structure, but only simplifies the expression trees - RunOptimizer(OptimizerType::EXPRESSION_REWRITER, [&]() { - rewriter.VisitOperator(*plan); - }); + RunOptimizer(OptimizerType::EXPRESSION_REWRITER, [&]() { rewriter.VisitOperator(*plan); }); // perform filter pullup RunOptimizer(OptimizerType::FILTER_PULLUP, [&]() { From a04a4b03a32010a6d0660eeb108800da40ef282e Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Wed, 7 Dec 2022 13:15:26 +0100 Subject: [PATCH 05/13] get rid of include iostream --- src/optimizer/filter_pushdown.cpp | 2 -- src/optimizer/pushdown/pushdown_left_join.cpp | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/optimizer/filter_pushdown.cpp b/src/optimizer/filter_pushdown.cpp index 5f0ce5a00f34..038a26f527a2 100644 --- a/src/optimizer/filter_pushdown.cpp +++ b/src/optimizer/filter_pushdown.cpp @@ -5,8 +5,6 @@ #include "duckdb/planner/operator/logical_join.hpp" #include "duckdb/optimizer/optimizer.hpp" -#include "iostream" - namespace duckdb { using Filter = FilterPushdown::Filter; diff --git a/src/optimizer/pushdown/pushdown_left_join.cpp b/src/optimizer/pushdown/pushdown_left_join.cpp index be23f18a2b17..ae1606234576 100644 --- a/src/optimizer/pushdown/pushdown_left_join.cpp +++ b/src/optimizer/pushdown/pushdown_left_join.cpp @@ -8,8 +8,6 @@ #include "duckdb/planner/operator/logical_comparison_join.hpp" #include "duckdb/planner/operator/logical_filter.hpp" -#include "iostream" - namespace duckdb { using Filter = FilterPushdown::Filter; From a366d50f37934aed26a138762084bf0f0d12b72f Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Thu, 8 Dec 2022 10:59:36 +0100 Subject: [PATCH 06/13] add operator pool to throw error in debug when reoptimizing a operator --- .../duckdb/optimizer/filter_pushdown.hpp | 3 +++ .../duckdb/optimizer/operator_pool.hpp | 2 +- src/optimizer/filter_pushdown.cpp | 19 +++++++++++++++++++ src/optimizer/operator_pool.cpp | 4 +++- src/optimizer/pushdown/pushdown_aggregate.cpp | 2 +- src/optimizer/pushdown/pushdown_mark_join.cpp | 3 ++- .../pushdown/pushdown_single_join.cpp | 2 +- 7 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/include/duckdb/optimizer/filter_pushdown.hpp b/src/include/duckdb/optimizer/filter_pushdown.hpp index aa6a32b41f2b..7c402927f9af 100644 --- a/src/include/duckdb/optimizer/filter_pushdown.hpp +++ b/src/include/duckdb/optimizer/filter_pushdown.hpp @@ -72,6 +72,9 @@ class FilterPushdown { // Finish pushing down at this operator, creating a LogicalFilter to store any of the stored filters and recursively // pushing down into its children (if any) unique_ptr FinishPushdown(unique_ptr op); + // This function is used when child operations are optimized with an optimizer that has + // should only + unique_ptr FinishPushdownNoChildOptimization(unique_ptr op); //! Adds a filter to the set of filters. Returns FilterResult::UNSATISFIABLE if the subtree should be stripped, or //! FilterResult::SUCCESS otherwise FilterResult AddFilter(unique_ptr expr); diff --git a/src/include/duckdb/optimizer/operator_pool.hpp b/src/include/duckdb/optimizer/operator_pool.hpp index b47f653b1f89..d8a38f00dada 100644 --- a/src/include/duckdb/optimizer/operator_pool.hpp +++ b/src/include/duckdb/optimizer/operator_pool.hpp @@ -19,7 +19,7 @@ class OperatorPool { seen_operators = unordered_set(); } - void AddOperator(LogicalOperator *op); + void CheckNotOptimized(LogicalOperator *op); bool InPool(LogicalOperator *op); private: diff --git a/src/optimizer/filter_pushdown.cpp b/src/optimizer/filter_pushdown.cpp index 038a26f527a2..3970ca2f947c 100644 --- a/src/optimizer/filter_pushdown.cpp +++ b/src/optimizer/filter_pushdown.cpp @@ -13,11 +13,16 @@ FilterPushdown::FilterPushdown(Optimizer &optimizer) : optimizer(optimizer), see } unique_ptr FilterPushdown::Rewrite(unique_ptr op) { +<<<<<<< HEAD if (seen_operators.InPool(op.get())) { // We've already optimized this operator, so just return. return move(op); } seen_operators.AddOperator(op.get()); +======= + // throws error in debug if operator has already been optimized with the current optimizer. + optimizer.seen_operators.CheckNotOptimized(op.get()); +>>>>>>> ea15a377ae (add operator pool to throw error in debug when reoptimizing a operator) D_ASSERT(!combiner.HasFilters()); switch (op->type) { case LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY: @@ -129,6 +134,20 @@ unique_ptr FilterPushdown::FinishPushdown(unique_ptr FilterPushdown::FinishPushdownNoChildOptimization(unique_ptr op) { + // now push any existing filters + if (filters.empty()) { + // no filters to push + return op; + } + auto filter = make_unique(); + for (auto &f : filters) { + filter->expressions.push_back(move(f->filter)); + } + filter->children.push_back(move(op)); + return move(filter); +} + void FilterPushdown::Filter::ExtractBindings() { bindings.clear(); LogicalJoin::GetExpressionBindings(*filter, bindings); diff --git a/src/optimizer/operator_pool.cpp b/src/optimizer/operator_pool.cpp index 7088cf3343db..3661a3a114f6 100644 --- a/src/optimizer/operator_pool.cpp +++ b/src/optimizer/operator_pool.cpp @@ -6,9 +6,11 @@ namespace duckdb { -void OperatorPool::AddOperator(LogicalOperator *op) { +void OperatorPool::CheckNotOptimized(LogicalOperator *op) { D_ASSERT(!InPool(op)); +#ifdef DEBUG seen_operators.insert((idx_t)op); +#endif } bool OperatorPool::InPool(LogicalOperator *op) { diff --git a/src/optimizer/pushdown/pushdown_aggregate.cpp b/src/optimizer/pushdown/pushdown_aggregate.cpp index a79953365407..63604d9991f0 100644 --- a/src/optimizer/pushdown/pushdown_aggregate.cpp +++ b/src/optimizer/pushdown/pushdown_aggregate.cpp @@ -65,7 +65,7 @@ unique_ptr FilterPushdown::PushdownAggregate(unique_ptrchildren[0] = child_pushdown.Rewrite(move(op->children[0])); - return FinishPushdown(move(op)); + return FinishPushdownNoChildOptimization(move(op)); } } // namespace duckdb diff --git a/src/optimizer/pushdown/pushdown_mark_join.cpp b/src/optimizer/pushdown/pushdown_mark_join.cpp index 58530d4039bd..795ed0fcd0ad 100644 --- a/src/optimizer/pushdown/pushdown_mark_join.cpp +++ b/src/optimizer/pushdown/pushdown_mark_join.cpp @@ -77,7 +77,8 @@ unique_ptr FilterPushdown::PushdownMarkJoin(unique_ptrchildren[0] = left_pushdown.Rewrite(move(op->children[0])); op->children[1] = right_pushdown.Rewrite(move(op->children[1])); - return FinishPushdown(move(op)); + return FinishPushdownNoChildOptimization(move(op)); +// return FinishPushdown(move(op)); } } // namespace duckdb diff --git a/src/optimizer/pushdown/pushdown_single_join.cpp b/src/optimizer/pushdown/pushdown_single_join.cpp index 9e1c6e38a451..34746d1e2d7e 100644 --- a/src/optimizer/pushdown/pushdown_single_join.cpp +++ b/src/optimizer/pushdown/pushdown_single_join.cpp @@ -23,7 +23,7 @@ unique_ptr FilterPushdown::PushdownSingleJoin(unique_ptrchildren[0] = left_pushdown.Rewrite(move(op->children[0])); op->children[1] = right_pushdown.Rewrite(move(op->children[1])); - return FinishPushdown(move(op)); + return FinishPushdownNoChildOptimization(move(op)); } } // namespace duckdb From c3e90e7fd43fb7a0d50bf2a08bda177bc7136275 Mon Sep 17 00:00:00 2001 From: Tmonster Date: Thu, 8 Dec 2022 13:26:17 +0100 Subject: [PATCH 07/13] adding the check to some more optimizers --- src/optimizer/deliminator.cpp | 1 + src/optimizer/filter_pullup.cpp | 1 + src/optimizer/regex_range_filter.cpp | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/optimizer/deliminator.cpp b/src/optimizer/deliminator.cpp index 2352b74e0ada..49763e270b1c 100644 --- a/src/optimizer/deliminator.cpp +++ b/src/optimizer/deliminator.cpp @@ -107,6 +107,7 @@ void DeliminatorPlanUpdater::VisitExpression(unique_ptr *expression) } unique_ptr Deliminator::Optimize(unique_ptr op) { + optimizer.seen_operators.CheckNotOptimized(op.get()); vector *> candidates; FindCandidates(&op, candidates); diff --git a/src/optimizer/filter_pullup.cpp b/src/optimizer/filter_pullup.cpp index 1cb3cb08c462..2fe9b09e852f 100644 --- a/src/optimizer/filter_pullup.cpp +++ b/src/optimizer/filter_pullup.cpp @@ -4,6 +4,7 @@ namespace duckdb { unique_ptr FilterPullup::Rewrite(unique_ptr op) { + optimizer.seen_operators.CheckNotOptimized(op.get()); switch (op->type) { case LogicalOperatorType::LOGICAL_FILTER: return PullupFilter(move(op)); diff --git a/src/optimizer/regex_range_filter.cpp b/src/optimizer/regex_range_filter.cpp index 7898502b7868..6cb695fd7f9f 100644 --- a/src/optimizer/regex_range_filter.cpp +++ b/src/optimizer/regex_range_filter.cpp @@ -16,7 +16,7 @@ namespace duckdb { unique_ptr RegexRangeFilter::Rewrite(unique_ptr op) { - + optimizer.seen_operators.CheckNotOptimized(op.get()); for (idx_t child_idx = 0; child_idx < op->children.size(); child_idx++) { op->children[child_idx] = Rewrite(move(op->children[child_idx])); } From 03ab3f89ad75feb4ea421fdab0690e937751b6dc Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Thu, 8 Dec 2022 13:43:40 +0100 Subject: [PATCH 08/13] removing code that shouldn't be in there --- src/optimizer/deliminator.cpp | 1 - src/optimizer/filter_pullup.cpp | 1 - src/optimizer/regex_range_filter.cpp | 1 - 3 files changed, 3 deletions(-) diff --git a/src/optimizer/deliminator.cpp b/src/optimizer/deliminator.cpp index 49763e270b1c..2352b74e0ada 100644 --- a/src/optimizer/deliminator.cpp +++ b/src/optimizer/deliminator.cpp @@ -107,7 +107,6 @@ void DeliminatorPlanUpdater::VisitExpression(unique_ptr *expression) } unique_ptr Deliminator::Optimize(unique_ptr op) { - optimizer.seen_operators.CheckNotOptimized(op.get()); vector *> candidates; FindCandidates(&op, candidates); diff --git a/src/optimizer/filter_pullup.cpp b/src/optimizer/filter_pullup.cpp index 2fe9b09e852f..1cb3cb08c462 100644 --- a/src/optimizer/filter_pullup.cpp +++ b/src/optimizer/filter_pullup.cpp @@ -4,7 +4,6 @@ namespace duckdb { unique_ptr FilterPullup::Rewrite(unique_ptr op) { - optimizer.seen_operators.CheckNotOptimized(op.get()); switch (op->type) { case LogicalOperatorType::LOGICAL_FILTER: return PullupFilter(move(op)); diff --git a/src/optimizer/regex_range_filter.cpp b/src/optimizer/regex_range_filter.cpp index 6cb695fd7f9f..c1467b313ce2 100644 --- a/src/optimizer/regex_range_filter.cpp +++ b/src/optimizer/regex_range_filter.cpp @@ -16,7 +16,6 @@ namespace duckdb { unique_ptr RegexRangeFilter::Rewrite(unique_ptr op) { - optimizer.seen_operators.CheckNotOptimized(op.get()); for (idx_t child_idx = 0; child_idx < op->children.size(); child_idx++) { op->children[child_idx] = Rewrite(move(op->children[child_idx])); } From 07dc6021a4590c7231692d92e57f288f2502fa71 Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Thu, 8 Dec 2022 13:57:49 +0100 Subject: [PATCH 09/13] ok filter pushdown shouldn't have crazy recursive calls --- src/optimizer/filter_pushdown.cpp | 8 -------- src/optimizer/operator_pool.cpp | 4 ---- src/optimizer/pushdown/pushdown_mark_join.cpp | 1 - 3 files changed, 13 deletions(-) diff --git a/src/optimizer/filter_pushdown.cpp b/src/optimizer/filter_pushdown.cpp index 3970ca2f947c..fec969873018 100644 --- a/src/optimizer/filter_pushdown.cpp +++ b/src/optimizer/filter_pushdown.cpp @@ -13,16 +13,8 @@ FilterPushdown::FilterPushdown(Optimizer &optimizer) : optimizer(optimizer), see } unique_ptr FilterPushdown::Rewrite(unique_ptr op) { -<<<<<<< HEAD - if (seen_operators.InPool(op.get())) { - // We've already optimized this operator, so just return. - return move(op); - } - seen_operators.AddOperator(op.get()); -======= // throws error in debug if operator has already been optimized with the current optimizer. optimizer.seen_operators.CheckNotOptimized(op.get()); ->>>>>>> ea15a377ae (add operator pool to throw error in debug when reoptimizing a operator) D_ASSERT(!combiner.HasFilters()); switch (op->type) { case LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY: diff --git a/src/optimizer/operator_pool.cpp b/src/optimizer/operator_pool.cpp index 3661a3a114f6..a5e57a9b7e38 100644 --- a/src/optimizer/operator_pool.cpp +++ b/src/optimizer/operator_pool.cpp @@ -1,7 +1,3 @@ -// -// Created by Tom Ebergen on 06/12/2022. -// - #include "duckdb/optimizer/operator_pool.hpp" namespace duckdb { diff --git a/src/optimizer/pushdown/pushdown_mark_join.cpp b/src/optimizer/pushdown/pushdown_mark_join.cpp index 795ed0fcd0ad..edd9b76757c9 100644 --- a/src/optimizer/pushdown/pushdown_mark_join.cpp +++ b/src/optimizer/pushdown/pushdown_mark_join.cpp @@ -78,7 +78,6 @@ unique_ptr FilterPushdown::PushdownMarkJoin(unique_ptrchildren[0] = left_pushdown.Rewrite(move(op->children[0])); op->children[1] = right_pushdown.Rewrite(move(op->children[1])); return FinishPushdownNoChildOptimization(move(op)); -// return FinishPushdown(move(op)); } } // namespace duckdb From 155c33d9c44c9fb6578ec9199ea6c98a5c7da154 Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Fri, 9 Dec 2022 10:06:22 +0100 Subject: [PATCH 10/13] format fix --- src/optimizer/filter_pushdown.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/optimizer/filter_pushdown.cpp b/src/optimizer/filter_pushdown.cpp index fec969873018..56a00d3991ad 100644 --- a/src/optimizer/filter_pushdown.cpp +++ b/src/optimizer/filter_pushdown.cpp @@ -9,7 +9,8 @@ namespace duckdb { using Filter = FilterPushdown::Filter; -FilterPushdown::FilterPushdown(Optimizer &optimizer) : optimizer(optimizer), seen_operators(), combiner(optimizer.context) { +FilterPushdown::FilterPushdown(Optimizer &optimizer) + : optimizer(optimizer), seen_operators(), combiner(optimizer.context) { } unique_ptr FilterPushdown::Rewrite(unique_ptr op) { From f4beb7c7819b89736f1f7914ed185c2274fa940c Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Fri, 9 Dec 2022 11:26:14 +0100 Subject: [PATCH 11/13] fixes so operator pool branch compiles --- src/include/duckdb/optimizer/filter_pushdown.hpp | 1 - src/include/duckdb/optimizer/operator_pool.hpp | 3 ++- src/include/duckdb/optimizer/optimizer.hpp | 3 +++ src/optimizer/filter_pushdown.cpp | 4 ++-- src/optimizer/operator_pool.cpp | 2 +- src/optimizer/optimizer.cpp | 6 ++++++ src/optimizer/pushdown/pushdown_mark_join.cpp | 2 +- src/storage/table/row_group_collection.cpp | 2 +- 8 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/include/duckdb/optimizer/filter_pushdown.hpp b/src/include/duckdb/optimizer/filter_pushdown.hpp index 7c402927f9af..cf682bed946d 100644 --- a/src/include/duckdb/optimizer/filter_pushdown.hpp +++ b/src/include/duckdb/optimizer/filter_pushdown.hpp @@ -38,7 +38,6 @@ class FilterPushdown { private: vector> filters; Optimizer &optimizer; - OperatorPool seen_operators; //! Push down a LogicalAggregate op unique_ptr PushdownAggregate(unique_ptr op); diff --git a/src/include/duckdb/optimizer/operator_pool.hpp b/src/include/duckdb/optimizer/operator_pool.hpp index d8a38f00dada..abe4c77c2882 100644 --- a/src/include/duckdb/optimizer/operator_pool.hpp +++ b/src/include/duckdb/optimizer/operator_pool.hpp @@ -19,8 +19,9 @@ class OperatorPool { seen_operators = unordered_set(); } - void CheckNotOptimized(LogicalOperator *op); + void AssertNotInPool(LogicalOperator *op); bool InPool(LogicalOperator *op); + void EmptyOperatorPool(); private: unordered_set seen_operators; diff --git a/src/include/duckdb/optimizer/optimizer.hpp b/src/include/duckdb/optimizer/optimizer.hpp index 96bde0ad7012..7efee712a499 100644 --- a/src/include/duckdb/optimizer/optimizer.hpp +++ b/src/include/duckdb/optimizer/optimizer.hpp @@ -12,6 +12,7 @@ #include "duckdb/planner/logical_operator.hpp" #include "duckdb/planner/logical_operator_visitor.hpp" #include "duckdb/common/enums/optimizer_type.hpp" +#include "duckdb/optimizer/operator_pool.hpp" #include @@ -23,6 +24,7 @@ class Optimizer { Optimizer(Binder &binder, ClientContext &context); unique_ptr Optimize(unique_ptr plan); + void AssertNotOptimized(LogicalOperator *op); ClientContext &context; Binder &binder; @@ -33,6 +35,7 @@ class Optimizer { void Verify(LogicalOperator &op); private: + OperatorPool seen_operators; unique_ptr plan; }; diff --git a/src/optimizer/filter_pushdown.cpp b/src/optimizer/filter_pushdown.cpp index 56a00d3991ad..59c80752357e 100644 --- a/src/optimizer/filter_pushdown.cpp +++ b/src/optimizer/filter_pushdown.cpp @@ -10,12 +10,12 @@ namespace duckdb { using Filter = FilterPushdown::Filter; FilterPushdown::FilterPushdown(Optimizer &optimizer) - : optimizer(optimizer), seen_operators(), combiner(optimizer.context) { + : optimizer(optimizer), combiner(optimizer.context) { } unique_ptr FilterPushdown::Rewrite(unique_ptr op) { // throws error in debug if operator has already been optimized with the current optimizer. - optimizer.seen_operators.CheckNotOptimized(op.get()); + optimizer.AssertNotOptimized(op.get()); D_ASSERT(!combiner.HasFilters()); switch (op->type) { case LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY: diff --git a/src/optimizer/operator_pool.cpp b/src/optimizer/operator_pool.cpp index a5e57a9b7e38..49b512633b44 100644 --- a/src/optimizer/operator_pool.cpp +++ b/src/optimizer/operator_pool.cpp @@ -2,7 +2,7 @@ namespace duckdb { -void OperatorPool::CheckNotOptimized(LogicalOperator *op) { +void OperatorPool::AssertNotInPool(LogicalOperator *op) { D_ASSERT(!InPool(op)); #ifdef DEBUG seen_operators.insert((idx_t)op); diff --git a/src/optimizer/optimizer.cpp b/src/optimizer/optimizer.cpp index f565c49efc8b..cfa63d2d7f71 100644 --- a/src/optimizer/optimizer.cpp +++ b/src/optimizer/optimizer.cpp @@ -61,6 +61,8 @@ void Optimizer::RunOptimizer(OptimizerType type, const std::function &ca profiler.StartPhase(OptimizerTypeToString(type)); callback(); profiler.EndPhase(); + seen_operators.EmptyOperatorPool(); + if (plan) { Verify(*plan); } @@ -70,6 +72,10 @@ void Optimizer::Verify(LogicalOperator &op) { ColumnBindingResolver::Verify(op); } +void Optimizer::AssertNotOptimized(LogicalOperator *op) { + seen_operators.AssertNotInPool(op); +} + unique_ptr Optimizer::Optimize(unique_ptr plan_p) { Verify(*plan_p); this->plan = move(plan_p); diff --git a/src/optimizer/pushdown/pushdown_mark_join.cpp b/src/optimizer/pushdown/pushdown_mark_join.cpp index edd9b76757c9..58530d4039bd 100644 --- a/src/optimizer/pushdown/pushdown_mark_join.cpp +++ b/src/optimizer/pushdown/pushdown_mark_join.cpp @@ -77,7 +77,7 @@ unique_ptr FilterPushdown::PushdownMarkJoin(unique_ptrchildren[0] = left_pushdown.Rewrite(move(op->children[0])); op->children[1] = right_pushdown.Rewrite(move(op->children[1])); - return FinishPushdownNoChildOptimization(move(op)); + return FinishPushdown(move(op)); } } // namespace duckdb diff --git a/src/storage/table/row_group_collection.cpp b/src/storage/table/row_group_collection.cpp index 19e537228b87..b319407ef553 100644 --- a/src/storage/table/row_group_collection.cpp +++ b/src/storage/table/row_group_collection.cpp @@ -1,4 +1,4 @@ -#include "duckdb/storage/table/row_group_collection.hpp" + #include "duckdb/storage/table/row_group_collection.hpp" #include "duckdb/storage/table/persistent_table_data.hpp" #include "duckdb/execution/expression_executor.hpp" #include "duckdb/main/client_context.hpp" From 1da0ccf969a3c9e61f2414c327e8118f565c50ea Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Tue, 13 Dec 2022 11:06:17 +0100 Subject: [PATCH 12/13] make format fix --- src/optimizer/filter_pushdown.cpp | 3 +-- src/storage/table/row_group_collection.cpp | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/optimizer/filter_pushdown.cpp b/src/optimizer/filter_pushdown.cpp index 59c80752357e..5689a9933b15 100644 --- a/src/optimizer/filter_pushdown.cpp +++ b/src/optimizer/filter_pushdown.cpp @@ -9,8 +9,7 @@ namespace duckdb { using Filter = FilterPushdown::Filter; -FilterPushdown::FilterPushdown(Optimizer &optimizer) - : optimizer(optimizer), combiner(optimizer.context) { +FilterPushdown::FilterPushdown(Optimizer &optimizer) : optimizer(optimizer), combiner(optimizer.context) { } unique_ptr FilterPushdown::Rewrite(unique_ptr op) { diff --git a/src/storage/table/row_group_collection.cpp b/src/storage/table/row_group_collection.cpp index b319407ef553..19e537228b87 100644 --- a/src/storage/table/row_group_collection.cpp +++ b/src/storage/table/row_group_collection.cpp @@ -1,4 +1,4 @@ - #include "duckdb/storage/table/row_group_collection.hpp" +#include "duckdb/storage/table/row_group_collection.hpp" #include "duckdb/storage/table/persistent_table_data.hpp" #include "duckdb/execution/expression_executor.hpp" #include "duckdb/main/client_context.hpp" From c08d7c730497e8fdb02692ca53c83f58e204a3bd Mon Sep 17 00:00:00 2001 From: Tom Ebergen Date: Tue, 13 Dec 2022 11:08:42 +0100 Subject: [PATCH 13/13] fix pushdown mark join to not optimize children --- src/optimizer/pushdown/pushdown_mark_join.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/optimizer/pushdown/pushdown_mark_join.cpp b/src/optimizer/pushdown/pushdown_mark_join.cpp index 58530d4039bd..edd9b76757c9 100644 --- a/src/optimizer/pushdown/pushdown_mark_join.cpp +++ b/src/optimizer/pushdown/pushdown_mark_join.cpp @@ -77,7 +77,7 @@ unique_ptr FilterPushdown::PushdownMarkJoin(unique_ptrchildren[0] = left_pushdown.Rewrite(move(op->children[0])); op->children[1] = right_pushdown.Rewrite(move(op->children[1])); - return FinishPushdown(move(op)); + return FinishPushdownNoChildOptimization(move(op)); } } // namespace duckdb