From 62147a7927263a3c76c3bc6c3d4c37e825affd42 Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Sun, 5 Jul 2026 20:38:55 +0000 Subject: [PATCH 01/14] feat(pushdown): dialect-aware filter rendering + exactness signal Add a query::Dialect (Postgres|ClickHouse) to QueryWriter/FilterPushdown configs so the shared renderer can target dialects that diverge in ways the quote/escape/blob knobs can't express: - VARCHAR constants: quote+escape via WriteQuotedAndEscaped (ClickHouse treats backslash as an escape inside '...', so BACKSLASH style escapes both ' and \; Postgres keeps ToSQLString-equivalent doubling). - (U)HugeInt / Decimal: rendered via toInt128/toUInt128/toDecimal128 casts for ClickHouse, which otherwise parses a bare wide literal as Float64 and loses precision. - COLLATE "C" is emitted only for Postgres; ClickHouse String comparison is already byte-wise and rejects the clause. - struct/tuple field access: tupleElement(col, 'name') for ClickHouse vs (col)."name" for Postgres. Add an optional `exact` out-param to TransformFilter: when provided, a dropped AND conjunct sets *exact=false (the caller must re-apply the superset locally) and a dropped OR branch renders the whole disjunction empty (never a wrong subset). nullptr preserves the legacy drop-and-widen behaviour, so existing callers are unaffected. Fix: IN-list constants were rendered with the identifier config (identifier quoting, no blob wrapper), producing e.g. `col IN ("a")` for Postgres and backtick-quoted values for ClickHouse. Render them with the constant config, matching the comparison path. --- .../dbconnector/query/query_writer.hpp | 9 ++- .../table_scan/filter_pushdown.hpp | 18 ++++-- src/query/query_writer.cpp | 25 +++++++- src/table_scan/filter_pushdown.cpp | 60 +++++++++++++------ 4 files changed, 88 insertions(+), 24 deletions(-) diff --git a/src/include/dbconnector/query/query_writer.hpp b/src/include/dbconnector/query/query_writer.hpp index 42ccb66..1b443cf 100644 --- a/src/include/dbconnector/query/query_writer.hpp +++ b/src/include/dbconnector/query/query_writer.hpp @@ -11,6 +11,11 @@ namespace query { enum class QuoteEscapeStyle { BACKSLASH, DOUBLE_QUOTE }; +// The remote SQL dialect a pushdown targets. Selects the dialect-specific bits +// that are not expressible through the quote/escape/blob knobs alone: string +// constant escaping, VARCHAR collation, and struct/tuple field access. +enum class Dialect { Postgres, ClickHouse }; + class QueryWriter { public: struct Config { @@ -18,11 +23,13 @@ class QueryWriter { QuoteEscapeStyle escape_style = QuoteEscapeStyle::DOUBLE_QUOTE; std::string blob_literal_prefix; std::string blob_literal_suffix; + Dialect dialect = Dialect::Postgres; }; static Config CreateConfig(char quote, QuoteEscapeStyle escape_style, const std::string &blob_literal_prefix = std::string(), - const std::string &blob_literal_suffix = std::string()); + const std::string &blob_literal_suffix = std::string(), + Dialect dialect = Dialect::Postgres); static std::string WriteQuotedAndEscaped(const QueryWriter::Config &config, const std::string &text); static std::string WriteConstant(const QueryWriter::Config &config, const duckdb::Value &val); diff --git a/src/include/dbconnector/table_scan/filter_pushdown.hpp b/src/include/dbconnector/table_scan/filter_pushdown.hpp index 5e7efaf..c5d6c5c 100644 --- a/src/include/dbconnector/table_scan/filter_pushdown.hpp +++ b/src/include/dbconnector/table_scan/filter_pushdown.hpp @@ -17,21 +17,29 @@ class FilterPushdown { query::QuoteEscapeStyle escape_style = query::QuoteEscapeStyle::DOUBLE_QUOTE; std::string blob_literal_prefix; std::string blob_literal_suffix; + query::Dialect dialect = query::Dialect::Postgres; }; public: static Config CreateConfig(char identifier_quote, char constant_quote, query::QuoteEscapeStyle escape_style, const std::string &blob_literal_prefix = std::string(), - const std::string &blob_literal_suffix = std::string()); - + const std::string &blob_literal_suffix = std::string(), + query::Dialect dialect = query::Dialect::Postgres); + + // `exact` (optional): when provided, set to false if the rendered SQL is WIDER + // than the filter (a dropped AND conjunct -> superset the caller must re-apply + // locally); a partial OR renders empty (the whole disjunction stays local) + // rather than a wrong subset. Passing nullptr keeps the legacy drop-and-widen + // behaviour, so existing callers are unaffected. static std::string TransformFilter(const Config &config, const std::string &column_name, - const duckdb::TableFilter &filter, duckdb::column_t column_id); + const duckdb::TableFilter &filter, duckdb::column_t column_id, + bool *exact = nullptr); private: static std::string TransformExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, const duckdb::Expression &expr, - duckdb::column_t column_id); + duckdb::column_t column_id, bool *exact); static std::string TransformExpressionSubject(const query::QueryWriter::Config &identifier_config, const std::string &column_name, const duckdb::Expression &expr); static std::string TransformConstantFilter(const query::QueryWriter::Config &constant_config, @@ -42,7 +50,7 @@ class FilterPushdown { const query::QueryWriter::Config &constant_config, const std::string &column_name, const duckdb::vector> &filters, - const std::string &op, duckdb::column_t column_id); + const std::string &op, duckdb::column_t column_id, bool *exact); }; } // namespace table_scan diff --git a/src/query/query_writer.cpp b/src/query/query_writer.cpp index b2d9d1d..c3735a6 100644 --- a/src/query/query_writer.cpp +++ b/src/query/query_writer.cpp @@ -7,12 +7,13 @@ namespace query { QueryWriter::Config QueryWriter::CreateConfig(char quote, QuoteEscapeStyle escape_style, const std::string &blob_literal_prefix, - const std::string &blob_literal_suffix) { + const std::string &blob_literal_suffix, Dialect dialect) { Config res; res.quote = quote; res.escape_style = escape_style; res.blob_literal_prefix = blob_literal_prefix; res.blob_literal_suffix = blob_literal_suffix; + res.dialect = dialect; return res; } @@ -60,6 +61,21 @@ std::string QueryWriter::EncodeBlob(const QueryWriter::Config &config, const std } std::string QueryWriter::WriteConstant(const QueryWriter::Config &config, const duckdb::Value &val) { + // ClickHouse parses a bare literal wider than (U)Int64 as Float64, losing + // precision; render (U)HugeInt and Decimal via exact typed casts instead. + if (config.dialect == Dialect::ClickHouse) { + switch (val.type().id()) { + case duckdb::LogicalTypeId::HUGEINT: + return "toInt128('" + val.ToString() + "')"; + case duckdb::LogicalTypeId::UHUGEINT: + return "toUInt128('" + val.ToString() + "')"; + case duckdb::LogicalTypeId::DECIMAL: + return "toDecimal128('" + val.ToString() + "', " + + std::to_string(duckdb::DecimalType::GetScale(val.type())) + ")"; + default: + break; + } + } if (val.type().IsNumeric() || val.type().id() == duckdb::LogicalTypeId::BOOLEAN) { return val.ToSQLString(); } @@ -71,6 +87,13 @@ std::string QueryWriter::WriteConstant(const QueryWriter::Config &config, const .DefaultCastAs(duckdb::LogicalType::VARCHAR) .ToSQLString(); } + // A plain string constant: quote + escape per the config's escape_style so the + // literal is valid in the target dialect (ClickHouse treats backslash as an + // escape inside '...', so its BACKSLASH style escapes both ' and \; postgres' + // DOUBLE_QUOTE style doubles ' and leaves \ literal -- matching ToSQLString). + if (val.type().id() == duckdb::LogicalTypeId::VARCHAR) { + return WriteQuotedAndEscaped(config, duckdb::StringValue::Get(val)); + } return val.DefaultCastAs(duckdb::LogicalType::VARCHAR).ToSQLString(); } diff --git a/src/table_scan/filter_pushdown.cpp b/src/table_scan/filter_pushdown.cpp index 563b3cc..900d328 100644 --- a/src/table_scan/filter_pushdown.cpp +++ b/src/table_scan/filter_pushdown.cpp @@ -23,13 +23,14 @@ using namespace duckdb; FilterPushdown::Config FilterPushdown::CreateConfig(char identifier_quote, char constant_quote, query::QuoteEscapeStyle escape_style, const std::string &blob_literal_prefix, - const std::string &blob_literal_suffix) { + const std::string &blob_literal_suffix, query::Dialect dialect) { Config res; res.identifier_quote = identifier_quote; res.constant_quote = constant_quote; res.escape_style = escape_style; res.blob_literal_prefix = blob_literal_prefix; res.blob_literal_suffix = blob_literal_suffix; + res.dialect = dialect; return res; } @@ -37,11 +38,22 @@ std::string FilterPushdown::CreateExpression(const query::QueryWriter::Config &i const query::QueryWriter::Config &constant_config, const std::string &column_name, const vector> &filters, const std::string &op, - column_t column_id) { + column_t column_id, bool *exact) { + const bool is_or = op == "OR"; vector filter_entries; for (auto &filter : filters) { - auto new_filter = TransformExpression(identifier_config, constant_config, column_name, *filter, column_id); + auto new_filter = + TransformExpression(identifier_config, constant_config, column_name, *filter, column_id, exact); if (new_filter.empty()) { + if (exact) { + // Dropping an OR branch would push a wrong subset -> the whole + // disjunction stays local. Dropping an AND conjunct pushes a + // superset the caller must re-apply locally. + if (is_or) { + return std::string(); + } + *exact = false; + } continue; } filter_entries.push_back(std::move(new_filter)); @@ -92,7 +104,10 @@ string FilterPushdown::TransformConstantFilter(const query::QueryWriter::Config } auto operator_string = TransformComparison(comparison_type); string comparison = StringUtil::Format("%s %s %s", column_name, operator_string, constant_string); - if (constant.type().id() == LogicalTypeId::VARCHAR) { + // Postgres forces byte-wise comparison to match DuckDB; ClickHouse's String + // comparison is already byte-wise and rejects the COLLATE clause. + if (constant.type().id() == LogicalTypeId::VARCHAR && + constant_config.dialect == query::Dialect::Postgres) { comparison += " COLLATE \"C\""; } return comparison; @@ -118,8 +133,16 @@ string FilterPushdown::TransformExpressionSubject(const query::QueryWriter::Conf if (struct_type.id() != LogicalTypeId::STRUCT || StructType::IsUnnamed(struct_type)) { return string(); } - auto child_name = query::QueryWriter::WriteQuotedAndEscaped( - identifier_config, StructType::GetChildName(struct_type, child_idx).GetIdentifierName()); + auto field = StructType::GetChildName(struct_type, child_idx).GetIdentifierName(); + if (identifier_config.dialect == query::Dialect::ClickHouse) { + // ClickHouse addresses a Tuple field as tupleElement(col, 'name'). + auto constant_config = query::QueryWriter::CreateConfig('\'', identifier_config.escape_style, + std::string(), std::string(), + identifier_config.dialect); + return "tupleElement(" + parent_name + ", " + + query::QueryWriter::WriteQuotedAndEscaped(constant_config, field) + ")"; + } + auto child_name = query::QueryWriter::WriteQuotedAndEscaped(identifier_config, field); return "(" + parent_name + ")." + child_name; } default: @@ -130,7 +153,7 @@ string FilterPushdown::TransformExpressionSubject(const query::QueryWriter::Conf std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, const Expression &expr, - column_t column_id) { + column_t column_id, bool *exact) { if (BoundComparisonExpression::IsComparison(expr)) { auto &comparison = expr.Cast(); auto comparison_type = comparison.GetExpressionType(); @@ -159,10 +182,10 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config switch (conjunction.GetExpressionType()) { case ExpressionType::CONJUNCTION_AND: return CreateExpression(identifier_config, constant_config, column_name, conjunction.GetChildren(), "AND", - column_id); + column_id, exact); case ExpressionType::CONJUNCTION_OR: return CreateExpression(identifier_config, constant_config, column_name, conjunction.GetChildren(), "OR", - column_id); + column_id, exact); default: return std::string(); } @@ -199,7 +222,7 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config in_list += "FALSE"; } else { in_list += query::QueryWriter::WriteConstant( - identifier_config, op.GetChildren()[i]->Cast().GetValue()); + constant_config, op.GetChildren()[i]->Cast().GetValue()); } } return IsVirtualColumn(column_id) ? "FALSE" : subject + " IN (" + in_list + ")"; @@ -213,13 +236,13 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config if (func.Function().GetName() == OptionalFilterScalarFun::NAME && func.BindInfo()) { auto &data = func.BindInfo()->Cast(); return data.child_filter_expr ? TransformExpression(identifier_config, constant_config, column_name, - *data.child_filter_expr, column_id) + *data.child_filter_expr, column_id, exact) : std::string(); } if (func.Function().GetName() == SelectivityOptionalFilterScalarFun::NAME && func.BindInfo()) { auto &data = func.BindInfo()->Cast(); return data.child_filter_expr ? TransformExpression(identifier_config, constant_config, column_name, - *data.child_filter_expr, column_id) + *data.child_filter_expr, column_id, exact) : std::string(); } if (func.Function().GetName() == DynamicFilterScalarFun::NAME) { @@ -233,13 +256,16 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config } std::string FilterPushdown::TransformFilter(const FilterPushdown::Config &config, const std::string &column_name, - const TableFilter &filter, column_t column_id) { - auto identifier_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style); - auto constant_config = query::QueryWriter::CreateConfig(config.constant_quote, config.escape_style, - config.blob_literal_prefix, config.blob_literal_suffix); + const TableFilter &filter, column_t column_id, bool *exact) { + auto identifier_config = + query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style, std::string(), std::string(), + config.dialect); + auto constant_config = query::QueryWriter::CreateConfig( + config.constant_quote, config.escape_style, config.blob_literal_prefix, config.blob_literal_suffix, + config.dialect); std::string column_name_quoted = query::QueryWriter::WriteQuotedAndEscaped(identifier_config, column_name); auto &expr = FilterUtil::GetExpression(filter, "FilterPushdown::TransformFilter"); - return TransformExpression(identifier_config, constant_config, column_name_quoted, expr, column_id); + return TransformExpression(identifier_config, constant_config, column_name_quoted, expr, column_id, exact); } } // namespace table_scan From e4594fa59d7c7da6ffd1565c15e74fe0d1ea1e83 Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Wed, 8 Jul 2026 19:02:34 +0000 Subject: [PATCH 02/14] feat(optimizer): safety hooks + engine-agnostic NULLS placement Add two optional vetoes to OrderByAndLimitOptimizer::Config so connectors whose remote semantics diverge from DuckDB's can adopt the rule (the fold REMOVES the local sort node, so it must only fire when the remote result is exactly DuckDB's): - order_key_unsafe(get, column_id): refuse folding an order key (e.g. the remote engine's ordering for the column's type differs -- ClickHouse float NaN placement, UUID/Enum/IP, toString()-surfaced columns). - limit_unsafe(get): refuse LIMIT-carrying folds (TOP_N, LIMIT) when cutting rows remotely is unsafe -- e.g. the scan re-applies part of its table filters locally, and a remote LIMIT would truncate BEFORE that re-check. A pure ORDER BY fold is unaffected (local filtering preserves row order). Null callbacks preserve the existing behaviour. Fix: TryBuildOrderByClause emitted a bare "col ASC" for NULLS FIRST (and bare "col DESC" for NULLS LAST), relying on the remote engine's default null placement. That default is engine-specific -- MySQL sorts NULLs first on ASC, Postgres and ClickHouse sort them last -- so ORDER BY x NULLS FIRST LIMIT k over a nullable key returned the WRONG ROWS on the latter two, with no local sort left to correct it. Emit the explicit `IS [NOT] NULL` prefix key for all four direction/placement combinations instead of relying on any default. --- .../order_by_and_limit_optimizer.hpp | 15 ++++++++++++++ .../order_by_and_limit_optimizer.cpp | 20 ++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp index fafeff9..bed7aa7 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp @@ -1,9 +1,11 @@ #pragma once +#include #include #include "duckdb/main/client_context.hpp" #include "duckdb/optimizer/optimizer_extension.hpp" +#include "duckdb/planner/operator/logical_get.hpp" #include "dbconnector/query/query_writer.hpp" @@ -17,6 +19,19 @@ class OrderByAndLimitOptimizer { char identifier_quote = '"'; query::QuoteEscapeStyle escape_style = query::QuoteEscapeStyle::DOUBLE_QUOTE; std::string table_scan_name; + //! Per-order-key veto: called with the scan and the resolved TABLE column id + //! once an order key traces to it; return true to refuse folding that key's + //! sort remotely (e.g. the remote engine's ordering for the column's type + //! diverges from DuckDB's, so a remote sort would ship a different row set). + //! Null = every traceable key is safe (legacy behaviour). + std::function order_key_unsafe; + //! Scan-level veto for LIMIT-carrying folds (TOP_N and LIMIT): return true to + //! refuse when cutting rows remotely is unsafe -- e.g. the scan re-applies part + //! of its table filters locally, so a remote LIMIT would truncate the stream + //! BEFORE the local re-check and drop rows that belong in the result. A pure + //! ORDER BY fold is unaffected (local filtering preserves the row order). + //! Null = always safe (legacy behaviour). + std::function limit_unsafe; }; static Config CreateConfig(duckdb::ClientContext &ctx, const std::string &enabled_option, char identifier_quote, diff --git a/src/optimizer/order_by_and_limit_optimizer.cpp b/src/optimizer/order_by_and_limit_optimizer.cpp index ee0d925..b26609a 100644 --- a/src/optimizer/order_by_and_limit_optimizer.cpp +++ b/src/optimizer/order_by_and_limit_optimizer.cpp @@ -84,6 +84,11 @@ static string TraceColumnToGet(const OrderByAndLimitOptimizer::Config &config, E if (actual_col_idx >= get.names.size()) { return std::string(); } + if (config.order_key_unsafe && config.order_key_unsafe(get, actual_col_idx)) { + // The connector vetoed this key: the remote engine's ordering for its type + // diverges from DuckDB's, so a remote sort would return the wrong rows. + return std::string(); + } auto query_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style); return query::QueryWriter::WriteQuotedAndEscaped(query_config, get.names[actual_col_idx].GetIdentifierName()); } @@ -108,9 +113,14 @@ static string TryBuildOrderByClause(const OrderByAndLimitOptimizer::Config &conf (direction == OrderType::ASCENDING) ? OrderByNullType::NULLS_LAST : OrderByNullType::NULLS_FIRST; } + // Emit an explicit IS [NOT] NULL prefix key for BOTH null placements instead + // of relying on the remote engine's default, which differs per engine (MySQL + // sorts NULLs first on ASC; Postgres and ClickHouse sort them last). A bare + // "col ASC" for NULLS FIRST returns the wrong rows on the latter two -- and + // since the fold removes the local sort node, nothing downstream corrects it. if (direction == OrderType::ASCENDING) { if (null_order == OrderByNullType::NULLS_FIRST) { - fragments.push_back(col_name + " ASC"); + fragments.push_back(col_name + " IS NOT NULL, " + col_name + " ASC"); } else { fragments.push_back(col_name + " IS NULL, " + col_name + " ASC"); } @@ -118,7 +128,7 @@ static string TryBuildOrderByClause(const OrderByAndLimitOptimizer::Config &conf if (null_order == OrderByNullType::NULLS_FIRST) { fragments.push_back(col_name + " IS NOT NULL, " + col_name + " DESC"); } else { - fragments.push_back(col_name + " DESC"); + fragments.push_back(col_name + " IS NULL, " + col_name + " DESC"); } } } @@ -256,7 +266,8 @@ void OrderByAndLimitOptimizer::Optimize(const OrderByAndLimitOptimizer::Config & auto &topn = op->Cast(); LogicalGet *get = nullptr; dbconnector::BindData *bind_data = nullptr; - if (OptimizerUtil::FindExtensionGet(config.table_scan_name, *op->children[0], get, bind_data)) { + if (OptimizerUtil::FindExtensionGet(config.table_scan_name, *op->children[0], get, bind_data) && + !(config.limit_unsafe && config.limit_unsafe(*get))) { string order_clause = TryBuildOrderByClause(config, topn.orders, *op->children[0], *get); if (!order_clause.empty()) { auto &order_by_and_limit_bind_data = bind_data->GetOrderByAndLimitBindData(); @@ -315,6 +326,9 @@ void OrderByAndLimitOptimizer::Optimize(const OrderByAndLimitOptimizer::Config & if (get.function.name != config.table_scan_name) { return; } + if (config.limit_unsafe && config.limit_unsafe(get)) { + return; + } switch (limit.limit_val.Type()) { case LimitNodeType::CONSTANT_VALUE: case LimitNodeType::UNSET: From b7f5a8d4c2921531949e7fe6bbb85b6c2bc3792f Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 00:31:20 +0000 Subject: [PATCH 03/14] refactor(optimizer): replace the veto callbacks with plain data The two std::function hooks on OrderByAndLimitOptimizer::Config were over-general for what they carried: - order_key_unsafe only ever answered a per-column, bind-time question ("does the remote order this column's type like DuckDB?"). It is now a vector on OrderByAndLimitBindData -- the struct the optimizer already reaches through the shared BindData -- filled by the connector from its type metadata (lazily in its GetOrderByAndLimitBindData override works well). Empty = every column safe, the legacy behaviour. - limit_unsafe only ever inspected get.table_filters, which the shared optimizer can see itself. It is now Config::fold_limit_with_table_ filters (default true = legacy); when false the optimizer refuses LIMIT/TOP_N folds over a scan carrying non-optional filters (their remote rendering may be inexact and re-applied locally; a remote LIMIT would truncate before that re-check). No behaviour change for either consumer; connectors now supply data, not code. --- .../order_by_and_limit_bind_data.hpp | 8 ++++++ .../order_by_and_limit_optimizer.hpp | 21 +++++---------- .../order_by_and_limit_optimizer.cpp | 27 +++++++++++++++---- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp index 5fd3191..b93745a 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include namespace dbconnector { namespace optimizer { @@ -8,6 +9,13 @@ namespace optimizer { struct OrderByAndLimitBindData { std::string limit_clause; std::string order_by_clause; + //! Per TABLE column (parallel to the scan's names): true = the remote engine's + //! ordering for this column's type diverges from DuckDB's, so an ORDER BY key + //! tracing to it must not be folded remotely (the sorted row set would differ). + //! Empty = every column is safe (the legacy behaviour). Filled by the connector + //! from its bind-time type metadata, typically lazily in its + //! GetOrderByAndLimitBindData() override. + std::vector order_key_unsafe; }; } // namespace optimizer diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp index bed7aa7..e90ef30 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp @@ -1,6 +1,5 @@ #pragma once -#include #include #include "duckdb/main/client_context.hpp" @@ -19,19 +18,13 @@ class OrderByAndLimitOptimizer { char identifier_quote = '"'; query::QuoteEscapeStyle escape_style = query::QuoteEscapeStyle::DOUBLE_QUOTE; std::string table_scan_name; - //! Per-order-key veto: called with the scan and the resolved TABLE column id - //! once an order key traces to it; return true to refuse folding that key's - //! sort remotely (e.g. the remote engine's ordering for the column's type - //! diverges from DuckDB's, so a remote sort would ship a different row set). - //! Null = every traceable key is safe (legacy behaviour). - std::function order_key_unsafe; - //! Scan-level veto for LIMIT-carrying folds (TOP_N and LIMIT): return true to - //! refuse when cutting rows remotely is unsafe -- e.g. the scan re-applies part - //! of its table filters locally, so a remote LIMIT would truncate the stream - //! BEFORE the local re-check and drop rows that belong in the result. A pure - //! ORDER BY fold is unaffected (local filtering preserves the row order). - //! Null = always safe (legacy behaviour). - std::function limit_unsafe; + //! False when the connector may re-apply the scan's table filters locally + //! (inexact remote pushdown): folding LIMIT/TOP_N would then truncate the + //! stream BEFORE the local re-check and drop rows that belong in the result. + //! A filterless scan still folds either way, and a pure ORDER BY fold is + //! unaffected (local filtering preserves the row order). True = always fold + //! (exact-pushdown engines, the legacy behaviour). + bool fold_limit_with_table_filters = true; }; static Config CreateConfig(duckdb::ClientContext &ctx, const std::string &enabled_option, char identifier_quote, diff --git a/src/optimizer/order_by_and_limit_optimizer.cpp b/src/optimizer/order_by_and_limit_optimizer.cpp index b26609a..d2a3303 100644 --- a/src/optimizer/order_by_and_limit_optimizer.cpp +++ b/src/optimizer/order_by_and_limit_optimizer.cpp @@ -8,6 +8,7 @@ #include "duckdb/planner/operator/logical_projection.hpp" #include "duckdb/planner/expression/bound_columnref_expression.hpp" #include "duckdb/planner/expression_iterator.hpp" +#include "duckdb/planner/filter/expression_filter.hpp" #include "dbconnector/bind_data.hpp" #include "dbconnector/optimizer/optimizer_util.hpp" @@ -84,15 +85,31 @@ static string TraceColumnToGet(const OrderByAndLimitOptimizer::Config &config, E if (actual_col_idx >= get.names.size()) { return std::string(); } - if (config.order_key_unsafe && config.order_key_unsafe(get, actual_col_idx)) { - // The connector vetoed this key: the remote engine's ordering for its type - // diverges from DuckDB's, so a remote sort would return the wrong rows. + auto &traced_bind_data = get.bind_data->Cast(); + const auto &unsafe_keys = traced_bind_data.GetOrderByAndLimitBindData().order_key_unsafe; + if (actual_col_idx < unsafe_keys.size() && unsafe_keys[actual_col_idx]) { + // The connector marked this column: the remote engine's ordering for its + // type diverges from DuckDB's, so a remote sort would return the wrong rows. return std::string(); } auto query_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style); return query::QueryWriter::WriteQuotedAndEscaped(query_config, get.names[actual_col_idx].GetIdentifierName()); } +static bool LimitFoldUnsafe(const OrderByAndLimitOptimizer::Config &config, const LogicalGet &get) { + if (config.fold_limit_with_table_filters) { + return false; + } + // The connector re-applies non-optional table filters locally: a remote LIMIT + // would truncate the stream before that re-check and drop qualifying rows. + for (auto &entry : get.table_filters) { + if (!ExpressionFilter::IsOptionalFilter(entry.Filter())) { + return true; + } + } + return false; +} + static string TryBuildOrderByClause(const OrderByAndLimitOptimizer::Config &config, vector &orders, LogicalOperator &child, LogicalGet &get) { vector fragments; @@ -267,7 +284,7 @@ void OrderByAndLimitOptimizer::Optimize(const OrderByAndLimitOptimizer::Config & LogicalGet *get = nullptr; dbconnector::BindData *bind_data = nullptr; if (OptimizerUtil::FindExtensionGet(config.table_scan_name, *op->children[0], get, bind_data) && - !(config.limit_unsafe && config.limit_unsafe(*get))) { + !LimitFoldUnsafe(config, *get)) { string order_clause = TryBuildOrderByClause(config, topn.orders, *op->children[0], *get); if (!order_clause.empty()) { auto &order_by_and_limit_bind_data = bind_data->GetOrderByAndLimitBindData(); @@ -326,7 +343,7 @@ void OrderByAndLimitOptimizer::Optimize(const OrderByAndLimitOptimizer::Config & if (get.function.name != config.table_scan_name) { return; } - if (config.limit_unsafe && config.limit_unsafe(get)) { + if (LimitFoldUnsafe(config, get)) { return; } switch (limit.limit_val.Type()) { From ba9ee2e87ea1be922c984b7b9dbe4db53a82f04a Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 01:51:36 +0000 Subject: [PATCH 04/14] refactor: TransformFilter returns RenderedFilter{sql, exact} instead of bool* out-param The optional bool* kept legacy callers source-compatible but made the inexact-render signal easy to ignore silently. Now every caller sees it: - postgres/clickhouse read .sql (clickhouse also consumes .exact); - AggregateOptimizer now refuses inexact filters under a remote aggregate (previously passed nullptr and would have aggregated over a widened WHERE). --- .../table_scan/filter_pushdown.hpp | 23 ++++++++------- src/optimizer/aggregate_optimizer.cpp | 6 ++-- src/table_scan/filter_pushdown.cpp | 28 ++++++++++--------- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/include/dbconnector/table_scan/filter_pushdown.hpp b/src/include/dbconnector/table_scan/filter_pushdown.hpp index c5d6c5c..76dab22 100644 --- a/src/include/dbconnector/table_scan/filter_pushdown.hpp +++ b/src/include/dbconnector/table_scan/filter_pushdown.hpp @@ -26,20 +26,23 @@ class FilterPushdown { const std::string &blob_literal_suffix = std::string(), query::Dialect dialect = query::Dialect::Postgres); - // `exact` (optional): when provided, set to false if the rendered SQL is WIDER - // than the filter (a dropped AND conjunct -> superset the caller must re-apply - // locally); a partial OR renders empty (the whole disjunction stays local) - // rather than a wrong subset. Passing nullptr keeps the legacy drop-and-widen - // behaviour, so existing callers are unaffected. - static std::string TransformFilter(const Config &config, const std::string &column_name, - const duckdb::TableFilter &filter, duckdb::column_t column_id, - bool *exact = nullptr); + struct RenderedFilter { + std::string sql; + //! False when `sql` is WIDER than the filter: an AND conjunct could not be + //! rendered and was dropped, so the remote returns a SUPERSET and the caller + //! must re-apply the full filter locally. An OR with an unrenderable branch + //! never widens -- the whole disjunction renders empty instead (a partial OR + //! would be a wrong subset). + bool exact = true; + }; + static RenderedFilter TransformFilter(const Config &config, const std::string &column_name, + const duckdb::TableFilter &filter, duckdb::column_t column_id); private: static std::string TransformExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, const duckdb::Expression &expr, - duckdb::column_t column_id, bool *exact); + duckdb::column_t column_id, bool &exact); static std::string TransformExpressionSubject(const query::QueryWriter::Config &identifier_config, const std::string &column_name, const duckdb::Expression &expr); static std::string TransformConstantFilter(const query::QueryWriter::Config &constant_config, @@ -50,7 +53,7 @@ class FilterPushdown { const query::QueryWriter::Config &constant_config, const std::string &column_name, const duckdb::vector> &filters, - const std::string &op, duckdb::column_t column_id, bool *exact); + const std::string &op, duckdb::column_t column_id, bool &exact); }; } // namespace table_scan diff --git a/src/optimizer/aggregate_optimizer.cpp b/src/optimizer/aggregate_optimizer.cpp index 394f8e4..941c0f6 100644 --- a/src/optimizer/aggregate_optimizer.cpp +++ b/src/optimizer/aggregate_optimizer.cpp @@ -195,13 +195,15 @@ static PushedAggregate TryPushAggregateToMySQL(const AggregateOptimizer::Config auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style); auto new_filter = table_scan::FilterPushdown::TransformFilter(scan_config, column_name.GetIdentifierName(), entry.Filter(), table_col_idx); - if (new_filter.empty()) { + if (new_filter.sql.empty() || !new_filter.exact) { + // An inexact WHERE under a remote aggregate returns wrong numbers; + // there is no local re-check once the rows are aggregated away. return res; } if (!where_clause.empty()) { where_clause += " AND "; } - where_clause += new_filter; + where_clause += new_filter.sql; } if (!where_clause.empty()) { res.where_clause = where_clause; diff --git a/src/table_scan/filter_pushdown.cpp b/src/table_scan/filter_pushdown.cpp index 900d328..523778f 100644 --- a/src/table_scan/filter_pushdown.cpp +++ b/src/table_scan/filter_pushdown.cpp @@ -38,22 +38,20 @@ std::string FilterPushdown::CreateExpression(const query::QueryWriter::Config &i const query::QueryWriter::Config &constant_config, const std::string &column_name, const vector> &filters, const std::string &op, - column_t column_id, bool *exact) { + column_t column_id, bool &exact) { const bool is_or = op == "OR"; vector filter_entries; for (auto &filter : filters) { auto new_filter = TransformExpression(identifier_config, constant_config, column_name, *filter, column_id, exact); if (new_filter.empty()) { - if (exact) { - // Dropping an OR branch would push a wrong subset -> the whole - // disjunction stays local. Dropping an AND conjunct pushes a - // superset the caller must re-apply locally. - if (is_or) { - return std::string(); - } - *exact = false; + // Dropping an OR branch would push a wrong subset -> the whole + // disjunction stays local. Dropping an AND conjunct pushes a + // superset the caller must re-apply locally. + if (is_or) { + return std::string(); } + exact = false; continue; } filter_entries.push_back(std::move(new_filter)); @@ -153,7 +151,7 @@ string FilterPushdown::TransformExpressionSubject(const query::QueryWriter::Conf std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, const Expression &expr, - column_t column_id, bool *exact) { + column_t column_id, bool &exact) { if (BoundComparisonExpression::IsComparison(expr)) { auto &comparison = expr.Cast(); auto comparison_type = comparison.GetExpressionType(); @@ -255,8 +253,9 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config } } -std::string FilterPushdown::TransformFilter(const FilterPushdown::Config &config, const std::string &column_name, - const TableFilter &filter, column_t column_id, bool *exact) { +FilterPushdown::RenderedFilter FilterPushdown::TransformFilter(const FilterPushdown::Config &config, + const std::string &column_name, + const TableFilter &filter, column_t column_id) { auto identifier_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style, std::string(), std::string(), config.dialect); @@ -265,7 +264,10 @@ std::string FilterPushdown::TransformFilter(const FilterPushdown::Config &config config.dialect); std::string column_name_quoted = query::QueryWriter::WriteQuotedAndEscaped(identifier_config, column_name); auto &expr = FilterUtil::GetExpression(filter, "FilterPushdown::TransformFilter"); - return TransformExpression(identifier_config, constant_config, column_name_quoted, expr, column_id, exact); + RenderedFilter result; + result.sql = + TransformExpression(identifier_config, constant_config, column_name_quoted, expr, column_id, result.exact); + return result; } } // namespace table_scan From 7da3f2bc103001d3fb17de9a0c2f538c6bf5bda7 Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 03:05:45 +0000 Subject: [PATCH 05/14] refactor: all-or-nothing filter rendering, drop the exact flag TransformFilter returns a plain string again (upstream signature): a filter either renders SQL equivalent to itself or renders empty and stays local. Only optional (advisory) pieces may be dropped from a conjunction; an unrenderable required piece vetoes the whole filter instead of widening (AND) or narrowing (OR) the remote WHERE. Also closes the aggregate optimizer's latent hole: a partially rendered WHERE under a remote aggregate now renders empty and stops the fold, instead of aggregating over a superset. --- .../table_scan/filter_pushdown.hpp | 21 ++++---- src/optimizer/aggregate_optimizer.cpp | 6 +-- src/table_scan/filter_pushdown.cpp | 48 ++++++++++--------- 3 files changed, 36 insertions(+), 39 deletions(-) diff --git a/src/include/dbconnector/table_scan/filter_pushdown.hpp b/src/include/dbconnector/table_scan/filter_pushdown.hpp index 76dab22..bfa46f0 100644 --- a/src/include/dbconnector/table_scan/filter_pushdown.hpp +++ b/src/include/dbconnector/table_scan/filter_pushdown.hpp @@ -26,23 +26,18 @@ class FilterPushdown { const std::string &blob_literal_suffix = std::string(), query::Dialect dialect = query::Dialect::Postgres); - struct RenderedFilter { - std::string sql; - //! False when `sql` is WIDER than the filter: an AND conjunct could not be - //! rendered and was dropped, so the remote returns a SUPERSET and the caller - //! must re-apply the full filter locally. An OR with an unrenderable branch - //! never widens -- the whole disjunction renders empty instead (a partial OR - //! would be a wrong subset). - bool exact = true; - }; - static RenderedFilter TransformFilter(const Config &config, const std::string &column_name, - const duckdb::TableFilter &filter, duckdb::column_t column_id); + //! All-or-nothing: returns SQL equivalent to the filter, or an empty string + //! when any required piece cannot be rendered (the filter must then be + //! applied locally). Only optional (advisory) pieces may be dropped from + //! the rendered SQL -- correctness never depends on them. + static std::string TransformFilter(const Config &config, const std::string &column_name, + const duckdb::TableFilter &filter, duckdb::column_t column_id); private: static std::string TransformExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, const duckdb::Expression &expr, - duckdb::column_t column_id, bool &exact); + duckdb::column_t column_id); static std::string TransformExpressionSubject(const query::QueryWriter::Config &identifier_config, const std::string &column_name, const duckdb::Expression &expr); static std::string TransformConstantFilter(const query::QueryWriter::Config &constant_config, @@ -53,7 +48,7 @@ class FilterPushdown { const query::QueryWriter::Config &constant_config, const std::string &column_name, const duckdb::vector> &filters, - const std::string &op, duckdb::column_t column_id, bool &exact); + const std::string &op, duckdb::column_t column_id); }; } // namespace table_scan diff --git a/src/optimizer/aggregate_optimizer.cpp b/src/optimizer/aggregate_optimizer.cpp index 941c0f6..2fb1b91 100644 --- a/src/optimizer/aggregate_optimizer.cpp +++ b/src/optimizer/aggregate_optimizer.cpp @@ -195,15 +195,15 @@ static PushedAggregate TryPushAggregateToMySQL(const AggregateOptimizer::Config auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style); auto new_filter = table_scan::FilterPushdown::TransformFilter(scan_config, column_name.GetIdentifierName(), entry.Filter(), table_col_idx); - if (new_filter.sql.empty() || !new_filter.exact) { - // An inexact WHERE under a remote aggregate returns wrong numbers; + if (new_filter.empty()) { + // A partial WHERE under a remote aggregate returns wrong numbers; // there is no local re-check once the rows are aggregated away. return res; } if (!where_clause.empty()) { where_clause += " AND "; } - where_clause += new_filter.sql; + where_clause += new_filter; } if (!where_clause.empty()) { res.where_clause = where_clause; diff --git a/src/table_scan/filter_pushdown.cpp b/src/table_scan/filter_pushdown.cpp index 523778f..47668df 100644 --- a/src/table_scan/filter_pushdown.cpp +++ b/src/table_scan/filter_pushdown.cpp @@ -34,25 +34,31 @@ FilterPushdown::Config FilterPushdown::CreateConfig(char identifier_quote, char return res; } +static bool IsOptionalFilterExpression(const Expression &expr) { + if (expr.GetExpressionClass() != ExpressionClass::BOUND_FUNCTION) { + return false; + } + auto &name = expr.Cast().Function().GetName(); + return name == OptionalFilterScalarFun::NAME || name == SelectivityOptionalFilterScalarFun::NAME || + name == DynamicFilterScalarFun::NAME; +} + std::string FilterPushdown::CreateExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, const vector> &filters, const std::string &op, - column_t column_id, bool &exact) { - const bool is_or = op == "OR"; + column_t column_id) { vector filter_entries; for (auto &filter : filters) { - auto new_filter = - TransformExpression(identifier_config, constant_config, column_name, *filter, column_id, exact); + auto new_filter = TransformExpression(identifier_config, constant_config, column_name, *filter, column_id); if (new_filter.empty()) { - // Dropping an OR branch would push a wrong subset -> the whole - // disjunction stays local. Dropping an AND conjunct pushes a - // superset the caller must re-apply locally. - if (is_or) { - return std::string(); + // Optional (advisory) pieces may be dropped from the SQL; dropping a + // required piece would make the SQL wider (AND) or narrower (OR) than + // the filter, so the whole filter renders empty and stays local. + if (IsOptionalFilterExpression(*filter)) { + continue; } - exact = false; - continue; + return std::string(); } filter_entries.push_back(std::move(new_filter)); } @@ -151,7 +157,7 @@ string FilterPushdown::TransformExpressionSubject(const query::QueryWriter::Conf std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, const Expression &expr, - column_t column_id, bool &exact) { + column_t column_id) { if (BoundComparisonExpression::IsComparison(expr)) { auto &comparison = expr.Cast(); auto comparison_type = comparison.GetExpressionType(); @@ -180,10 +186,10 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config switch (conjunction.GetExpressionType()) { case ExpressionType::CONJUNCTION_AND: return CreateExpression(identifier_config, constant_config, column_name, conjunction.GetChildren(), "AND", - column_id, exact); + column_id); case ExpressionType::CONJUNCTION_OR: return CreateExpression(identifier_config, constant_config, column_name, conjunction.GetChildren(), "OR", - column_id, exact); + column_id); default: return std::string(); } @@ -234,13 +240,13 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config if (func.Function().GetName() == OptionalFilterScalarFun::NAME && func.BindInfo()) { auto &data = func.BindInfo()->Cast(); return data.child_filter_expr ? TransformExpression(identifier_config, constant_config, column_name, - *data.child_filter_expr, column_id, exact) + *data.child_filter_expr, column_id) : std::string(); } if (func.Function().GetName() == SelectivityOptionalFilterScalarFun::NAME && func.BindInfo()) { auto &data = func.BindInfo()->Cast(); return data.child_filter_expr ? TransformExpression(identifier_config, constant_config, column_name, - *data.child_filter_expr, column_id, exact) + *data.child_filter_expr, column_id) : std::string(); } if (func.Function().GetName() == DynamicFilterScalarFun::NAME) { @@ -253,9 +259,8 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config } } -FilterPushdown::RenderedFilter FilterPushdown::TransformFilter(const FilterPushdown::Config &config, - const std::string &column_name, - const TableFilter &filter, column_t column_id) { +std::string FilterPushdown::TransformFilter(const FilterPushdown::Config &config, const std::string &column_name, + const TableFilter &filter, column_t column_id) { auto identifier_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style, std::string(), std::string(), config.dialect); @@ -264,10 +269,7 @@ FilterPushdown::RenderedFilter FilterPushdown::TransformFilter(const FilterPushd config.dialect); std::string column_name_quoted = query::QueryWriter::WriteQuotedAndEscaped(identifier_config, column_name); auto &expr = FilterUtil::GetExpression(filter, "FilterPushdown::TransformFilter"); - RenderedFilter result; - result.sql = - TransformExpression(identifier_config, constant_config, column_name_quoted, expr, column_id, result.exact); - return result; + return TransformExpression(identifier_config, constant_config, column_name_quoted, expr, column_id); } } // namespace table_scan From bc836fc46b858ad06364c33d7825107d7514d20f Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 16:11:05 +0000 Subject: [PATCH 06/14] fix: close all-or-nothing edge cases in the filter renderer - an unrenderable OPTIONAL branch under OR now fails the whole disjunction (dropping it would narrow the result; AND-only skips) - WriteConstant renders NULL instead of throwing InternalException (VARCHAR) or emitting toInt128('NULL') (CH casts) - virtual-column (rowid) filters render empty -- unrenderable, stays local -- instead of FALSE, which narrowed the result to zero rows - PruneColumnsAfterOrderByRemoval resolves projection-map positions through GetColumnBindings() instead of indexing column_ids with table-column indices (OOB/wrong column on non-identity projections) --- .../order_by_and_limit_optimizer.cpp | 20 +++++------- src/query/query_writer.cpp | 6 ++++ src/table_scan/filter_pushdown.cpp | 31 +++++++++---------- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/optimizer/order_by_and_limit_optimizer.cpp b/src/optimizer/order_by_and_limit_optimizer.cpp index d2a3303..7f80f54 100644 --- a/src/optimizer/order_by_and_limit_optimizer.cpp +++ b/src/optimizer/order_by_and_limit_optimizer.cpp @@ -197,12 +197,15 @@ static void PruneProjectionLayer(LogicalProjection &proj, const unordered_set &projection_map) { + const vector &projection_map) { if (child.type == LogicalOperatorType::LOGICAL_GET) { - auto &column_ids = get.GetColumnIds(); + // projection_map entries are positions in the child's OUTPUT; resolve them + // through the bindings (which honor projection_ids) instead of indexing + // column_ids positionally -- the two spaces differ on non-identity scans. + auto bindings = get.GetColumnBindings(); vector new_ids; - for (auto idx : projection_map) { - new_ids.push_back(column_ids[idx]); + for (auto pos : projection_map) { + new_ids.push_back(get.GetColumnIndex(bindings[pos])); } get.SetColumnIds(std::move(new_ids)); get.projection_ids.clear(); @@ -312,14 +315,7 @@ void OrderByAndLimitOptimizer::Optimize(const OrderByAndLimitOptimizer::Config & auto &order_by_and_limit_bind_data = bind_data->GetOrderByAndLimitBindData(); order_by_and_limit_bind_data.order_by_clause = order_clause; if (!order.projection_map.empty()) { - vector indices; - indices.reserve(order.projection_map.size()); - for (auto proj_idx : order.projection_map) { - ColumnIndex col_idx = get->GetColumnIndex(proj_idx); - column_t table_col_idx = col_idx.GetPrimaryIndex(); - indices.emplace_back(table_col_idx); - } - PruneColumnsAfterOrderByRemoval(*op->children[0], *get, indices); + PruneColumnsAfterOrderByRemoval(*op->children[0], *get, order.projection_map); } op = std::move(op->children[0]); return; diff --git a/src/query/query_writer.cpp b/src/query/query_writer.cpp index c3735a6..24ce0e0 100644 --- a/src/query/query_writer.cpp +++ b/src/query/query_writer.cpp @@ -61,6 +61,12 @@ std::string QueryWriter::EncodeBlob(const QueryWriter::Config &config, const std } std::string QueryWriter::WriteConstant(const QueryWriter::Config &config, const duckdb::Value &val) { + // Every typed branch below assumes a non-NULL payload (StringValue::Get + // throws InternalException on NULL; ToString renders the word NULL inside + // a typed cast). + if (val.IsNull()) { + return "NULL"; + } // ClickHouse parses a bare literal wider than (U)Int64 as Float64, losing // precision; render (U)HugeInt and Decimal via exact typed casts instead. if (config.dialect == Dialect::ClickHouse) { diff --git a/src/table_scan/filter_pushdown.cpp b/src/table_scan/filter_pushdown.cpp index 47668df..848da42 100644 --- a/src/table_scan/filter_pushdown.cpp +++ b/src/table_scan/filter_pushdown.cpp @@ -48,14 +48,17 @@ std::string FilterPushdown::CreateExpression(const query::QueryWriter::Config &i const std::string &column_name, const vector> &filters, const std::string &op, column_t column_id) { + const bool is_or = op == "OR"; vector filter_entries; for (auto &filter : filters) { auto new_filter = TransformExpression(identifier_config, constant_config, column_name, *filter, column_id); if (new_filter.empty()) { - // Optional (advisory) pieces may be dropped from the SQL; dropping a - // required piece would make the SQL wider (AND) or narrower (OR) than - // the filter, so the whole filter renders empty and stays local. - if (IsOptionalFilterExpression(*filter)) { + // Optional (advisory) pieces may be dropped from an AND (widens the + // SQL, never the result). Dropping anything from an OR narrows the + // result, and dropping a required AND conjunct widens the SQL past + // the filter -- both make the SQL lie, so the whole filter renders + // empty and stays local. + if (!is_or && IsOptionalFilterExpression(*filter)) { continue; } return std::string(); @@ -100,12 +103,12 @@ static bool IsDirectReference(const Expression &expr) { string FilterPushdown::TransformConstantFilter(const query::QueryWriter::Config &constant_config, const string &column_name, ExpressionType comparison_type, const Value &constant, column_t column_id) { - string constant_string; if (IsVirtualColumn(column_id)) { - return "FALSE"; - } else { - constant_string = query::QueryWriter::WriteConstant(constant_config, constant); + // A rowid has no remote SQL identity; rendering anything (the old FALSE) + // makes the SQL narrower than the filter. Unrenderable -> stays local. + return string(); } + string constant_string = query::QueryWriter::WriteConstant(constant_config, constant); auto operator_string = TransformComparison(comparison_type); string comparison = StringUtil::Format("%s %s %s", column_name, operator_string, constant_string); // Postgres forces byte-wise comparison to match DuckDB; ClickHouse's String @@ -211,7 +214,7 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config } return std::string(); case ExpressionType::COMPARE_IN: { - if (subject.empty()) { + if (subject.empty() || IsVirtualColumn(column_id)) { return string(); } std::string in_list; @@ -222,14 +225,10 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config if (!in_list.empty()) { in_list += ", "; } - if (IsVirtualColumn(column_id)) { - in_list += "FALSE"; - } else { - in_list += query::QueryWriter::WriteConstant( - constant_config, op.GetChildren()[i]->Cast().GetValue()); - } + in_list += query::QueryWriter::WriteConstant( + constant_config, op.GetChildren()[i]->Cast().GetValue()); } - return IsVirtualColumn(column_id) ? "FALSE" : subject + " IN (" + in_list + ")"; + return subject + " IN (" + in_list + ")"; } default: return std::string(); From f066143a5ad323bfc939b80f527136f9d8097026 Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 16:31:10 +0000 Subject: [PATCH 07/14] refactor: reuse shared helpers, prebuilt configs, minor dedup - TraceColumnToGet delegates the projection-chain walk to OptimizerUtil::TraceBindingToColumn (new col_idx on the result) - the LIMIT branch locates the scan via FindExtensionGet like the TOP_N and ORDER_BY branches - FilterPushdown::Config holds the two prebuilt QueryWriter configs instead of six scalars reassembled per call - BOUND_FUNCTION optional-filter unwrap: one recursion site, dead dynamic_filter branch removed - NULLS placement fragment builder: four branches -> two ternaries - misc: unused include, loop-invariant config hoist --- .../dbconnector/optimizer/optimizer_util.hpp | 1 + .../order_by_and_limit_optimizer.hpp | 1 - .../table_scan/filter_pushdown.hpp | 8 +- src/optimizer/aggregate_optimizer.cpp | 2 +- src/optimizer/optimizer_util.cpp | 1 + .../order_by_and_limit_optimizer.cpp | 78 +++---------------- src/table_scan/filter_pushdown.cpp | 45 ++++------- 7 files changed, 32 insertions(+), 104 deletions(-) diff --git a/src/include/dbconnector/optimizer/optimizer_util.hpp b/src/include/dbconnector/optimizer/optimizer_util.hpp index 167e097..a9ab673 100644 --- a/src/include/dbconnector/optimizer/optimizer_util.hpp +++ b/src/include/dbconnector/optimizer/optimizer_util.hpp @@ -15,6 +15,7 @@ namespace optimizer { struct TracedBindingColumn { std::string col_name; duckdb::LogicalType col_type; + duckdb::idx_t col_idx = 0; bool Found() { return !col_name.empty(); diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp index e90ef30..90cb3a9 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp @@ -4,7 +4,6 @@ #include "duckdb/main/client_context.hpp" #include "duckdb/optimizer/optimizer_extension.hpp" -#include "duckdb/planner/operator/logical_get.hpp" #include "dbconnector/query/query_writer.hpp" diff --git a/src/include/dbconnector/table_scan/filter_pushdown.hpp b/src/include/dbconnector/table_scan/filter_pushdown.hpp index bfa46f0..ee5ee9f 100644 --- a/src/include/dbconnector/table_scan/filter_pushdown.hpp +++ b/src/include/dbconnector/table_scan/filter_pushdown.hpp @@ -12,12 +12,8 @@ namespace table_scan { class FilterPushdown { struct Config { - char identifier_quote = '"'; - char constant_quote = '\''; - query::QuoteEscapeStyle escape_style = query::QuoteEscapeStyle::DOUBLE_QUOTE; - std::string blob_literal_prefix; - std::string blob_literal_suffix; - query::Dialect dialect = query::Dialect::Postgres; + query::QueryWriter::Config identifier; + query::QueryWriter::Config constant; }; public: diff --git a/src/optimizer/aggregate_optimizer.cpp b/src/optimizer/aggregate_optimizer.cpp index 2fb1b91..39b7a4f 100644 --- a/src/optimizer/aggregate_optimizer.cpp +++ b/src/optimizer/aggregate_optimizer.cpp @@ -184,6 +184,7 @@ static PushedAggregate TryPushAggregateToMySQL(const AggregateOptimizer::Config if (get.table_filters.HasFilters()) { string where_clause; + auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style); for (auto &entry : get.table_filters) { ProjectionIndex proj_idx = entry.GetIndex(); ColumnIndex col_idx = get.GetColumnIndex(proj_idx); @@ -192,7 +193,6 @@ static PushedAggregate TryPushAggregateToMySQL(const AggregateOptimizer::Config return res; } auto column_name = get.names[table_col_idx]; - auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style); auto new_filter = table_scan::FilterPushdown::TransformFilter(scan_config, column_name.GetIdentifierName(), entry.Filter(), table_col_idx); if (new_filter.empty()) { diff --git a/src/optimizer/optimizer_util.cpp b/src/optimizer/optimizer_util.cpp index 19368cf..0e005cc 100644 --- a/src/optimizer/optimizer_util.cpp +++ b/src/optimizer/optimizer_util.cpp @@ -70,6 +70,7 @@ TracedBindingColumn OptimizerUtil::TraceBindingToColumn(ColumnBinding binding, L } res.col_name = get.names[actual_col_idx].GetIdentifierName(); res.col_type = get.returned_types[actual_col_idx]; + res.col_idx = actual_col_idx; return res; } diff --git a/src/optimizer/order_by_and_limit_optimizer.cpp b/src/optimizer/order_by_and_limit_optimizer.cpp index 7f80f54..6e2c499 100644 --- a/src/optimizer/order_by_and_limit_optimizer.cpp +++ b/src/optimizer/order_by_and_limit_optimizer.cpp @@ -46,54 +46,19 @@ static string TraceColumnToGet(const OrderByAndLimitOptimizer::Config &config, E if (col_ref.Depth() > 0) { return std::string(); } - auto binding = col_ref.BindingMutable(); - - reference current = child; - while (current.get().type == LogicalOperatorType::LOGICAL_PROJECTION) { - auto &proj = current.get().Cast(); - if (binding.table_index != proj.table_index) { - break; - } - if (binding.column_index >= proj.expressions.size()) { - return std::string(); - } - auto &proj_expr = *proj.expressions[binding.column_index]; - if (proj_expr.GetExpressionClass() != ExpressionClass::BOUND_COLUMN_REF) { - return std::string(); - } - auto &inner_ref = proj_expr.Cast(); - if (inner_ref.Depth() > 0) { - return std::string(); - } - binding = inner_ref.BindingMutable(); - current = *current.get().children[0]; - } - - if (binding.table_index != get.table_index) { - return std::string(); - } - auto &column_ids = get.GetColumnIds(); - if (binding.column_index >= column_ids.size()) { - return std::string(); - } - auto &col_index = column_ids[binding.column_index]; - if (col_index.IsRowIdColumn()) { - return std::string(); - } - - auto actual_col_idx = col_index.GetPrimaryIndex(); - if (actual_col_idx >= get.names.size()) { + auto traced = OptimizerUtil::TraceBindingToColumn(col_ref.BindingMutable(), child, get); + if (!traced.Found()) { return std::string(); } auto &traced_bind_data = get.bind_data->Cast(); const auto &unsafe_keys = traced_bind_data.GetOrderByAndLimitBindData().order_key_unsafe; - if (actual_col_idx < unsafe_keys.size() && unsafe_keys[actual_col_idx]) { + if (traced.col_idx < unsafe_keys.size() && unsafe_keys[traced.col_idx]) { // The connector marked this column: the remote engine's ordering for its // type diverges from DuckDB's, so a remote sort would return the wrong rows. return std::string(); } auto query_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style); - return query::QueryWriter::WriteQuotedAndEscaped(query_config, get.names[actual_col_idx].GetIdentifierName()); + return query::QueryWriter::WriteQuotedAndEscaped(query_config, traced.col_name); } static bool LimitFoldUnsafe(const OrderByAndLimitOptimizer::Config &config, const LogicalGet &get) { @@ -135,19 +100,9 @@ static string TryBuildOrderByClause(const OrderByAndLimitOptimizer::Config &conf // sorts NULLs first on ASC; Postgres and ClickHouse sort them last). A bare // "col ASC" for NULLS FIRST returns the wrong rows on the latter two -- and // since the fold removes the local sort node, nothing downstream corrects it. - if (direction == OrderType::ASCENDING) { - if (null_order == OrderByNullType::NULLS_FIRST) { - fragments.push_back(col_name + " IS NOT NULL, " + col_name + " ASC"); - } else { - fragments.push_back(col_name + " IS NULL, " + col_name + " ASC"); - } - } else { - if (null_order == OrderByNullType::NULLS_FIRST) { - fragments.push_back(col_name + " IS NOT NULL, " + col_name + " DESC"); - } else { - fragments.push_back(col_name + " IS NULL, " + col_name + " DESC"); - } - } + const char *null_key = (null_order == OrderByNullType::NULLS_FIRST) ? " IS NOT NULL, " : " IS NULL, "; + const char *dir = (direction == OrderType::ASCENDING) ? " ASC" : " DESC"; + fragments.push_back(col_name + null_key + col_name + dir); } return " ORDER BY " + StringUtil::Join(fragments, ", "); } @@ -328,18 +283,10 @@ void OrderByAndLimitOptimizer::Optimize(const OrderByAndLimitOptimizer::Config & } if (op->type == LogicalOperatorType::LOGICAL_LIMIT) { auto &limit = op->Cast(); - reference child = *op->children[0]; - while (child.get().type == LogicalOperatorType::LOGICAL_PROJECTION) { - child = *child.get().children[0]; - } - if (child.get().type != LogicalOperatorType::LOGICAL_GET) { - return; - } - auto &get = child.get().Cast(); - if (get.function.name != config.table_scan_name) { - return; - } - if (LimitFoldUnsafe(config, get)) { + LogicalGet *get = nullptr; + dbconnector::BindData *bind_data = nullptr; + if (!OptimizerUtil::FindExtensionGet(config.table_scan_name, *op->children[0], get, bind_data) || + LimitFoldUnsafe(config, *get)) { return; } switch (limit.limit_val.Type()) { @@ -356,8 +303,7 @@ void OrderByAndLimitOptimizer::Optimize(const OrderByAndLimitOptimizer::Config & default: return; } - auto &bind_data = get.bind_data->Cast(); - auto &order_by_and_limit_bind_data = bind_data.GetOrderByAndLimitBindData(); + auto &order_by_and_limit_bind_data = bind_data->GetOrderByAndLimitBindData(); if (!order_by_and_limit_bind_data.limit_clause.empty()) { return; } diff --git a/src/table_scan/filter_pushdown.cpp b/src/table_scan/filter_pushdown.cpp index 848da42..c56d71c 100644 --- a/src/table_scan/filter_pushdown.cpp +++ b/src/table_scan/filter_pushdown.cpp @@ -25,12 +25,10 @@ FilterPushdown::Config FilterPushdown::CreateConfig(char identifier_quote, char const std::string &blob_literal_prefix, const std::string &blob_literal_suffix, query::Dialect dialect) { Config res; - res.identifier_quote = identifier_quote; - res.constant_quote = constant_quote; - res.escape_style = escape_style; - res.blob_literal_prefix = blob_literal_prefix; - res.blob_literal_suffix = blob_literal_suffix; - res.dialect = dialect; + res.identifier = + query::QueryWriter::CreateConfig(identifier_quote, escape_style, std::string(), std::string(), dialect); + res.constant = + query::QueryWriter::CreateConfig(constant_quote, escape_style, blob_literal_prefix, blob_literal_suffix, dialect); return res; } @@ -143,9 +141,7 @@ string FilterPushdown::TransformExpressionSubject(const query::QueryWriter::Conf auto field = StructType::GetChildName(struct_type, child_idx).GetIdentifierName(); if (identifier_config.dialect == query::Dialect::ClickHouse) { // ClickHouse addresses a Tuple field as tupleElement(col, 'name'). - auto constant_config = query::QueryWriter::CreateConfig('\'', identifier_config.escape_style, - std::string(), std::string(), - identifier_config.dialect); + auto constant_config = query::QueryWriter::CreateConfig('\'', identifier_config.escape_style); return "tupleElement(" + parent_name + ", " + query::QueryWriter::WriteQuotedAndEscaped(constant_config, field) + ")"; } @@ -236,22 +232,17 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config } case ExpressionClass::BOUND_FUNCTION: { auto &func = expr.Cast(); - if (func.Function().GetName() == OptionalFilterScalarFun::NAME && func.BindInfo()) { - auto &data = func.BindInfo()->Cast(); - return data.child_filter_expr ? TransformExpression(identifier_config, constant_config, column_name, - *data.child_filter_expr, column_id) - : std::string(); + auto &name = func.Function().GetName(); + optional_ptr child; + if (func.BindInfo() && name == OptionalFilterScalarFun::NAME) { + child = func.BindInfo()->Cast().child_filter_expr.get(); + } else if (func.BindInfo() && name == SelectivityOptionalFilterScalarFun::NAME) { + child = func.BindInfo()->Cast().child_filter_expr.get(); } - if (func.Function().GetName() == SelectivityOptionalFilterScalarFun::NAME && func.BindInfo()) { - auto &data = func.BindInfo()->Cast(); - return data.child_filter_expr ? TransformExpression(identifier_config, constant_config, column_name, - *data.child_filter_expr, column_id) - : std::string(); - } - if (func.Function().GetName() == DynamicFilterScalarFun::NAME) { + if (!child) { return std::string(); } - return std::string(); + return TransformExpression(identifier_config, constant_config, column_name, *child, column_id); } default: return std::string(); @@ -260,15 +251,9 @@ std::string FilterPushdown::TransformExpression(const query::QueryWriter::Config std::string FilterPushdown::TransformFilter(const FilterPushdown::Config &config, const std::string &column_name, const TableFilter &filter, column_t column_id) { - auto identifier_config = - query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style, std::string(), std::string(), - config.dialect); - auto constant_config = query::QueryWriter::CreateConfig( - config.constant_quote, config.escape_style, config.blob_literal_prefix, config.blob_literal_suffix, - config.dialect); - std::string column_name_quoted = query::QueryWriter::WriteQuotedAndEscaped(identifier_config, column_name); + std::string column_name_quoted = query::QueryWriter::WriteQuotedAndEscaped(config.identifier, column_name); auto &expr = FilterUtil::GetExpression(filter, "FilterPushdown::TransformFilter"); - return TransformExpression(identifier_config, constant_config, column_name_quoted, expr, column_id); + return TransformExpression(config.identifier, config.constant, column_name_quoted, expr, column_id); } } // namespace table_scan From cc06934a81180ddc4618959c62fef94c7231be93 Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 17:58:36 +0000 Subject: [PATCH 08/14] fix: render TIMESTAMP_TZ constants with their UTC offset The old rendering cast to naive TIMESTAMP first, dropping the offset; a remote session in any non-UTC time zone re-parsed the wall time in its own zone, silently shifting every pushed timestamptz comparison (equality lost rows, ranges matched extras). The plain VARCHAR cast keeps the offset, which parses to the correct instant everywhere. --- src/query/query_writer.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/query/query_writer.cpp b/src/query/query_writer.cpp index 24ce0e0..056a8ed 100644 --- a/src/query/query_writer.cpp +++ b/src/query/query_writer.cpp @@ -88,11 +88,12 @@ std::string QueryWriter::WriteConstant(const QueryWriter::Config &config, const if (val.type().id() == duckdb::LogicalTypeId::BLOB) { return EncodeBlob(config, duckdb::StringValue::Get(val)); } - if (val.type().id() == duckdb::LogicalTypeId::TIMESTAMP_TZ) { - return val.DefaultCastAs(duckdb::LogicalType::TIMESTAMP) - .DefaultCastAs(duckdb::LogicalType::VARCHAR) - .ToSQLString(); - } + // TIMESTAMP_TZ deliberately falls through to the VARCHAR cast below: it + // renders WITH the UTC offset ('... 12:00:00+00'), which the remote parses + // as the correct instant regardless of its session time zone. Casting to + // naive TIMESTAMP first (the old behaviour) dropped the offset, so a + // non-UTC remote session re-interpreted the wall time in its own zone -- + // silently shifted comparisons. // A plain string constant: quote + escape per the config's escape_style so the // literal is valid in the target dialect (ClickHouse treats backslash as an // escape inside '...', so its BACKSLASH style escapes both ' and \; postgres' From 7f9bf6a0946a0bc7b29c02c40dd9fc4cee7f37dc Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 20:25:27 +0000 Subject: [PATCH 09/14] feat(optimizer): dialect-driven ORDER-key rewrites replace the veto bitmap Order keys whose remote ordering diverges from DuckDB's are rewritten per dialect instead of vetoed, so the sort still folds: - ClickHouse floats get an isNaN() prefix key in the key's direction (DuckDB sorts NaN above every number; ClickHouse compares IEEE) - ClickHouse VARCHAR/UUID keys become toString(col): text-backed columns (Enum labels, IPv4/6, JSON, Decimal(>38)) surface locally as their toString() text, so the remote byte-wise text order matches the local order by construction (identity for plain String); UUIDs regain canonical order from their half-swapped internal one - compound keys nesting a divergent scalar (visible in the DuckDB type) cannot be rewritten field-wise and stay local - Postgres VARCHAR keys get COLLATE "C": locale collation is not DuckDB's byte order OrderByAndLimitBindData.order_key_unsafe and its connector-filled bitmap are gone; Config gains the dialect (defaulted Postgres, so existing callers are unchanged). --- .../order_by_and_limit_bind_data.hpp | 8 -- .../order_by_and_limit_optimizer.hpp | 6 +- .../order_by_and_limit_optimizer.cpp | 114 +++++++++++++++--- 3 files changed, 102 insertions(+), 26 deletions(-) diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp index b93745a..5fd3191 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_bind_data.hpp @@ -1,7 +1,6 @@ #pragma once #include -#include namespace dbconnector { namespace optimizer { @@ -9,13 +8,6 @@ namespace optimizer { struct OrderByAndLimitBindData { std::string limit_clause; std::string order_by_clause; - //! Per TABLE column (parallel to the scan's names): true = the remote engine's - //! ordering for this column's type diverges from DuckDB's, so an ORDER BY key - //! tracing to it must not be folded remotely (the sorted row set would differ). - //! Empty = every column is safe (the legacy behaviour). Filled by the connector - //! from its bind-time type metadata, typically lazily in its - //! GetOrderByAndLimitBindData() override. - std::vector order_key_unsafe; }; } // namespace optimizer diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp index 90cb3a9..4698581 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp @@ -24,10 +24,14 @@ class OrderByAndLimitOptimizer { //! unaffected (local filtering preserves the row order). True = always fold //! (exact-pushdown engines, the legacy behaviour). bool fold_limit_with_table_filters = true; + //! Selects the per-type ORDER-key rewrites that make the remote sort + //! reproduce DuckDB's ordering (see TryBuildOrderByClause). + query::Dialect dialect = query::Dialect::Postgres; }; static Config CreateConfig(duckdb::ClientContext &ctx, const std::string &enabled_option, char identifier_quote, - query::QuoteEscapeStyle escape_style, std::string table_scan_name); + query::QuoteEscapeStyle escape_style, std::string table_scan_name, + query::Dialect dialect = query::Dialect::Postgres); static void Optimize(const Config &config, duckdb::OptimizerExtensionInput &input, duckdb::unique_ptr &op); diff --git a/src/optimizer/order_by_and_limit_optimizer.cpp b/src/optimizer/order_by_and_limit_optimizer.cpp index 6e2c499..8d675d2 100644 --- a/src/optimizer/order_by_and_limit_optimizer.cpp +++ b/src/optimizer/order_by_and_limit_optimizer.cpp @@ -21,7 +21,8 @@ using namespace duckdb; OrderByAndLimitOptimizer::Config OrderByAndLimitOptimizer::CreateConfig(ClientContext &ctx, const std::string &enabled_option, char identifier_quote, - query::QuoteEscapeStyle escape_style, std::string table_scan_name) { + query::QuoteEscapeStyle escape_style, std::string table_scan_name, + query::Dialect dialect) { Config res; res.enabled = false; @@ -33,32 +34,64 @@ OrderByAndLimitOptimizer::CreateConfig(ClientContext &ctx, const std::string &en res.identifier_quote = identifier_quote; res.escape_style = escape_style; res.table_scan_name = std::move(table_scan_name); + res.dialect = dialect; return res; } -static string TraceColumnToGet(const OrderByAndLimitOptimizer::Config &config, Expression &expr, LogicalOperator &child, - LogicalGet &get) { +// Traces an ORDER BY key expression to a scan column; fills `quoted` (the +// dialect-quoted column reference) and `type`. False when the key is not a +// plain scan column. +static bool TraceOrderKey(const OrderByAndLimitOptimizer::Config &config, Expression &expr, LogicalOperator &child, + LogicalGet &get, string "ed, LogicalType &type) { if (expr.GetExpressionClass() != ExpressionClass::BOUND_COLUMN_REF) { - return std::string(); + return false; } auto &col_ref = expr.Cast(); if (col_ref.Depth() > 0) { - return std::string(); + return false; } auto traced = OptimizerUtil::TraceBindingToColumn(col_ref.BindingMutable(), child, get); if (!traced.Found()) { - return std::string(); - } - auto &traced_bind_data = get.bind_data->Cast(); - const auto &unsafe_keys = traced_bind_data.GetOrderByAndLimitBindData().order_key_unsafe; - if (traced.col_idx < unsafe_keys.size() && unsafe_keys[traced.col_idx]) { - // The connector marked this column: the remote engine's ordering for its - // type diverges from DuckDB's, so a remote sort would return the wrong rows. - return std::string(); + return false; } auto query_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style); - return query::QueryWriter::WriteQuotedAndEscaped(query_config, traced.col_name); + quoted = query::QueryWriter::WriteQuotedAndEscaped(query_config, traced.col_name); + type = traced.col_type; + return true; +} + +// True when `type` nests a scalar whose ClickHouse ordering diverges from +// DuckDB's. A compound key compares field-wise on both sides, but a divergent +// field cannot be rewritten inside a whole-value comparison (no per-field +// isNaN/toString), so such keys stay local. VARCHAR fields are conservative: +// an Enum surfaces as VARCHAR locally but sorts by ordinal remotely, and the +// DuckDB type cannot tell the two apart. +static bool CompoundContainsDivergent(const LogicalType &type) { + switch (type.id()) { + case LogicalTypeId::FLOAT: + case LogicalTypeId::DOUBLE: + case LogicalTypeId::UUID: + case LogicalTypeId::VARCHAR: + return true; + case LogicalTypeId::STRUCT: { + for (auto &child : StructType::GetChildTypes(type)) { + if (CompoundContainsDivergent(child.second)) { + return true; + } + } + return false; + } + case LogicalTypeId::LIST: + return CompoundContainsDivergent(ListType::GetChildType(type)); + case LogicalTypeId::ARRAY: + return CompoundContainsDivergent(ArrayType::GetChildType(type)); + case LogicalTypeId::MAP: + return CompoundContainsDivergent(MapType::KeyType(type)) || + CompoundContainsDivergent(MapType::ValueType(type)); + default: + return false; + } } static bool LimitFoldUnsafe(const OrderByAndLimitOptimizer::Config &config, const LogicalGet &get) { @@ -79,8 +112,9 @@ static string TryBuildOrderByClause(const OrderByAndLimitOptimizer::Config &conf LogicalOperator &child, LogicalGet &get) { vector fragments; for (auto &order : orders) { - string col_name = TraceColumnToGet(config, *order.expression, child, get); - if (col_name.empty()) { + string quoted; + LogicalType key_type; + if (!TraceOrderKey(config, *order.expression, child, get, quoted, key_type)) { return std::string(); } @@ -102,7 +136,53 @@ static string TryBuildOrderByClause(const OrderByAndLimitOptimizer::Config &conf // since the fold removes the local sort node, nothing downstream corrects it. const char *null_key = (null_order == OrderByNullType::NULLS_FIRST) ? " IS NOT NULL, " : " IS NULL, "; const char *dir = (direction == OrderType::ASCENDING) ? " ASC" : " DESC"; - fragments.push_back(col_name + null_key + col_name + dir); + + // Rewrite the value key per dialect so the remote reproduces DuckDB's + // ordering (the NULLS prefix stays on the bare column): + // - ClickHouse floats compare IEEE, leaving NaN unordered; DuckDB sorts + // NaN above every number. An isNaN() prefix key in the key's own + // direction pins NaN to the greatest position. + // - ClickHouse text-backed columns (Enum labels, IPv4/6, JSON, + // Decimal(>38)) surface locally as their toString() text, and UUIDs + // sort by a half-swapped byte order: ordering by toString(col) matches + // the local byte-wise order in every case (identity for plain String). + // - Compound keys nesting a divergent scalar cannot be rewritten + // field-wise -- the sort stays local. + // - Postgres sorts text by locale collation; DuckDB compares bytes, so + // the key gets COLLATE "C". Its floats/UUIDs already match DuckDB. + string value_key = quoted; + string nan_key; + switch (config.dialect) { + case query::Dialect::ClickHouse: + switch (key_type.id()) { + case LogicalTypeId::FLOAT: + case LogicalTypeId::DOUBLE: + nan_key = "isNaN(" + quoted + ")" + dir + ", "; + break; + case LogicalTypeId::VARCHAR: + case LogicalTypeId::UUID: + value_key = "toString(" + quoted + ")"; + break; + case LogicalTypeId::STRUCT: + case LogicalTypeId::LIST: + case LogicalTypeId::ARRAY: + case LogicalTypeId::MAP: + if (CompoundContainsDivergent(key_type)) { + return std::string(); + } + break; + default: + break; + } + break; + case query::Dialect::Postgres: + if (key_type.id() == LogicalTypeId::VARCHAR) { + value_key = quoted + " COLLATE \"C\""; + } + break; + } + + fragments.push_back(quoted + null_key + nan_key + value_key + dir); } return " ORDER BY " + StringUtil::Join(fragments, ", "); } From 5b9274c1971e0728c174aed744869640455c0ccc Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 20:36:12 +0000 Subject: [PATCH 10/14] refactor: dialect is a required CreateConfig parameter A defaulted Postgres dialect let a new connector silently inherit postgres rendering semantics (COLLATE, order-key rewrites); every caller now states its dialect. FilterPushdown::CreateConfig moves the dialect before the (still-defaulted) blob literal affixes. Comment dedup in the order-key rewrite. --- .../order_by_and_limit_optimizer.hpp | 2 +- .../table_scan/filter_pushdown.hpp | 5 ++-- src/optimizer/aggregate_optimizer.cpp | 3 +- .../order_by_and_limit_optimizer.cpp | 30 +++++++------------ src/table_scan/filter_pushdown.cpp | 4 +-- 5 files changed, 17 insertions(+), 27 deletions(-) diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp index 4698581..4602bb9 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp @@ -31,7 +31,7 @@ class OrderByAndLimitOptimizer { static Config CreateConfig(duckdb::ClientContext &ctx, const std::string &enabled_option, char identifier_quote, query::QuoteEscapeStyle escape_style, std::string table_scan_name, - query::Dialect dialect = query::Dialect::Postgres); + query::Dialect dialect); static void Optimize(const Config &config, duckdb::OptimizerExtensionInput &input, duckdb::unique_ptr &op); diff --git a/src/include/dbconnector/table_scan/filter_pushdown.hpp b/src/include/dbconnector/table_scan/filter_pushdown.hpp index ee5ee9f..c7d899e 100644 --- a/src/include/dbconnector/table_scan/filter_pushdown.hpp +++ b/src/include/dbconnector/table_scan/filter_pushdown.hpp @@ -18,9 +18,8 @@ class FilterPushdown { public: static Config CreateConfig(char identifier_quote, char constant_quote, query::QuoteEscapeStyle escape_style, - const std::string &blob_literal_prefix = std::string(), - const std::string &blob_literal_suffix = std::string(), - query::Dialect dialect = query::Dialect::Postgres); + query::Dialect dialect, const std::string &blob_literal_prefix = std::string(), + const std::string &blob_literal_suffix = std::string()); //! All-or-nothing: returns SQL equivalent to the filter, or an empty string //! when any required piece cannot be rendered (the filter must then be diff --git a/src/optimizer/aggregate_optimizer.cpp b/src/optimizer/aggregate_optimizer.cpp index 39b7a4f..0d01a29 100644 --- a/src/optimizer/aggregate_optimizer.cpp +++ b/src/optimizer/aggregate_optimizer.cpp @@ -184,7 +184,8 @@ static PushedAggregate TryPushAggregateToMySQL(const AggregateOptimizer::Config if (get.table_filters.HasFilters()) { string where_clause; - auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style); + auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style, + query::Dialect::Postgres); for (auto &entry : get.table_filters) { ProjectionIndex proj_idx = entry.GetIndex(); ColumnIndex col_idx = get.GetColumnIndex(proj_idx); diff --git a/src/optimizer/order_by_and_limit_optimizer.cpp b/src/optimizer/order_by_and_limit_optimizer.cpp index 8d675d2..ebf8e14 100644 --- a/src/optimizer/order_by_and_limit_optimizer.cpp +++ b/src/optimizer/order_by_and_limit_optimizer.cpp @@ -39,9 +39,8 @@ OrderByAndLimitOptimizer::CreateConfig(ClientContext &ctx, const std::string &en return res; } -// Traces an ORDER BY key expression to a scan column; fills `quoted` (the -// dialect-quoted column reference) and `type`. False when the key is not a -// plain scan column. +// Traces an ORDER BY key expression to a scan column; false when the key is +// not a plain scan column. static bool TraceOrderKey(const OrderByAndLimitOptimizer::Config &config, Expression &expr, LogicalOperator &child, LogicalGet &get, string "ed, LogicalType &type) { if (expr.GetExpressionClass() != ExpressionClass::BOUND_COLUMN_REF) { @@ -62,11 +61,9 @@ static bool TraceOrderKey(const OrderByAndLimitOptimizer::Config &config, Expres } // True when `type` nests a scalar whose ClickHouse ordering diverges from -// DuckDB's. A compound key compares field-wise on both sides, but a divergent -// field cannot be rewritten inside a whole-value comparison (no per-field -// isNaN/toString), so such keys stay local. VARCHAR fields are conservative: -// an Enum surfaces as VARCHAR locally but sorts by ordinal remotely, and the -// DuckDB type cannot tell the two apart. +// DuckDB's: a divergent field cannot be rewritten inside a whole-value +// comparison, so such keys stay local. VARCHAR is included because an Enum +// (ordinal order remotely) is indistinguishable from a plain String here. static bool CompoundContainsDivergent(const LogicalType &type) { switch (type.id()) { case LogicalTypeId::FLOAT: @@ -138,18 +135,11 @@ static string TryBuildOrderByClause(const OrderByAndLimitOptimizer::Config &conf const char *dir = (direction == OrderType::ASCENDING) ? " ASC" : " DESC"; // Rewrite the value key per dialect so the remote reproduces DuckDB's - // ordering (the NULLS prefix stays on the bare column): - // - ClickHouse floats compare IEEE, leaving NaN unordered; DuckDB sorts - // NaN above every number. An isNaN() prefix key in the key's own - // direction pins NaN to the greatest position. - // - ClickHouse text-backed columns (Enum labels, IPv4/6, JSON, - // Decimal(>38)) surface locally as their toString() text, and UUIDs - // sort by a half-swapped byte order: ordering by toString(col) matches - // the local byte-wise order in every case (identity for plain String). - // - Compound keys nesting a divergent scalar cannot be rewritten - // field-wise -- the sort stays local. - // - Postgres sorts text by locale collation; DuckDB compares bytes, so - // the key gets COLLATE "C". Its floats/UUIDs already match DuckDB. + // ordering: an isNaN() prefix pins NaN to the greatest position (CH + // compares IEEE); toString() restores byte-wise text order for CH's + // text-backed columns and half-swapped UUIDs (the connector surfaces + // those AS their toString() text, so the orders match by construction); + // COLLATE "C" replaces postgres' locale collation with byte order. string value_key = quoted; string nan_key; switch (config.dialect) { diff --git a/src/table_scan/filter_pushdown.cpp b/src/table_scan/filter_pushdown.cpp index c56d71c..e96bfe4 100644 --- a/src/table_scan/filter_pushdown.cpp +++ b/src/table_scan/filter_pushdown.cpp @@ -21,9 +21,9 @@ namespace table_scan { using namespace duckdb; FilterPushdown::Config FilterPushdown::CreateConfig(char identifier_quote, char constant_quote, - query::QuoteEscapeStyle escape_style, + query::QuoteEscapeStyle escape_style, query::Dialect dialect, const std::string &blob_literal_prefix, - const std::string &blob_literal_suffix, query::Dialect dialect) { + const std::string &blob_literal_suffix) { Config res; res.identifier = query::QueryWriter::CreateConfig(identifier_quote, escape_style, std::string(), std::string(), dialect); From 4c066d71547fd926fde159bc453ef4ee0f284b2b Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 21:50:41 +0000 Subject: [PATCH 11/14] refactor(optimizer): derive the filtered-LIMIT-fold policy from the dialect fold_limit_with_table_filters is gone. Postgres pushdown is exact-or-error -- every required filter runs in the remote statement, WHERE before LIMIT -- so folding is always safe there. Other engines' comparison semantics (ClickHouse: NaN, Enum ordinals, server-time-zone literals) force connectors to keep some required filters local; a folded LIMIT would truncate the stream before that re-check, so non-Postgres dialects refuse LIMIT-bearing folds over scans with required filters. Fail-closed for future dialects. --- .../optimizer/order_by_and_limit_optimizer.hpp | 11 +++-------- src/optimizer/order_by_and_limit_optimizer.cpp | 11 ++++++++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp index 4602bb9..6e51d39 100644 --- a/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp +++ b/src/include/dbconnector/optimizer/order_by_and_limit_optimizer.hpp @@ -17,15 +17,10 @@ class OrderByAndLimitOptimizer { char identifier_quote = '"'; query::QuoteEscapeStyle escape_style = query::QuoteEscapeStyle::DOUBLE_QUOTE; std::string table_scan_name; - //! False when the connector may re-apply the scan's table filters locally - //! (inexact remote pushdown): folding LIMIT/TOP_N would then truncate the - //! stream BEFORE the local re-check and drop rows that belong in the result. - //! A filterless scan still folds either way, and a pure ORDER BY fold is - //! unaffected (local filtering preserves the row order). True = always fold - //! (exact-pushdown engines, the legacy behaviour). - bool fold_limit_with_table_filters = true; //! Selects the per-type ORDER-key rewrites that make the remote sort - //! reproduce DuckDB's ordering (see TryBuildOrderByClause). + //! reproduce DuckDB's ordering (see TryBuildOrderByClause), and whether + //! LIMIT-bearing folds are allowed over filtered scans (see + //! LimitFoldUnsafe). query::Dialect dialect = query::Dialect::Postgres; }; diff --git a/src/optimizer/order_by_and_limit_optimizer.cpp b/src/optimizer/order_by_and_limit_optimizer.cpp index ebf8e14..2dd171d 100644 --- a/src/optimizer/order_by_and_limit_optimizer.cpp +++ b/src/optimizer/order_by_and_limit_optimizer.cpp @@ -92,11 +92,16 @@ static bool CompoundContainsDivergent(const LogicalType &type) { } static bool LimitFoldUnsafe(const OrderByAndLimitOptimizer::Config &config, const LogicalGet &get) { - if (config.fold_limit_with_table_filters) { + // Postgres filter pushdown is exact-or-error: every required filter runs in + // the remote statement, WHERE before LIMIT, so folding is always safe. Other + // engines' comparison semantics (ClickHouse: NaN, Enum ordinals, literals + // parsed in the server time zone) force connectors to keep some required + // filters local and re-apply them AFTER the fetch -- a folded LIMIT would + // truncate the stream before that re-check and drop qualifying rows. + // Optional (advisory) filters never change the row set and do not block. + if (config.dialect == query::Dialect::Postgres) { return false; } - // The connector re-applies non-optional table filters locally: a remote LIMIT - // would truncate the stream before that re-check and drop qualifying rows. for (auto &entry : get.table_filters) { if (!ExpressionFilter::IsOptionalFilter(entry.Filter())) { return true; From 3f98ca4b9cc39be53390898ca69080e809100117 Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 22:13:59 +0000 Subject: [PATCH 12/14] chore: delete the consumer-less AggregateOptimizer No connector registers it (mysql removed its wiring upstream; the postgres and clickhouse connectors never had one), yet every binary compiled it. The AggregateBindData contract member and the col_idx field it alone consumed go with it. --- src/include/dbconnector/bind_data.hpp | 3 - .../optimizer/aggregate_bind_data.hpp | 16 - .../optimizer/aggregate_optimizer.hpp | 34 -- .../dbconnector/optimizer/optimizer_util.hpp | 1 - src/optimizer/aggregate_optimizer.cpp | 339 ------------------ src/optimizer/optimizer_util.cpp | 1 - test/extension/CMakeLists.txt | 1 - 7 files changed, 395 deletions(-) delete mode 100644 src/include/dbconnector/optimizer/aggregate_bind_data.hpp delete mode 100644 src/include/dbconnector/optimizer/aggregate_optimizer.hpp delete mode 100644 src/optimizer/aggregate_optimizer.cpp diff --git a/src/include/dbconnector/bind_data.hpp b/src/include/dbconnector/bind_data.hpp index 4e81dfd..3ebb7e9 100644 --- a/src/include/dbconnector/bind_data.hpp +++ b/src/include/dbconnector/bind_data.hpp @@ -2,15 +2,12 @@ #include "duckdb/function/function.hpp" -#include "dbconnector/optimizer/aggregate_bind_data.hpp" #include "dbconnector/optimizer/order_by_and_limit_bind_data.hpp" namespace dbconnector { class BindData : public duckdb::FunctionData { public: - virtual optimizer::AggregateBindData &GetAggregateBindData() = 0; - virtual optimizer::OrderByAndLimitBindData &GetOrderByAndLimitBindData() = 0; }; diff --git a/src/include/dbconnector/optimizer/aggregate_bind_data.hpp b/src/include/dbconnector/optimizer/aggregate_bind_data.hpp deleted file mode 100644 index 7528650..0000000 --- a/src/include/dbconnector/optimizer/aggregate_bind_data.hpp +++ /dev/null @@ -1,16 +0,0 @@ -#pragma once - -#include - -namespace dbconnector { -namespace optimizer { - -struct AggregateBindData { - std::string aggregate_select_list; - std::string group_by_clause; - std::string aggregate_where_clause; - bool has_aggregate_pushdown = false; -}; - -} // namespace optimizer -} // namespace dbconnector diff --git a/src/include/dbconnector/optimizer/aggregate_optimizer.hpp b/src/include/dbconnector/optimizer/aggregate_optimizer.hpp deleted file mode 100644 index 447f9eb..0000000 --- a/src/include/dbconnector/optimizer/aggregate_optimizer.hpp +++ /dev/null @@ -1,34 +0,0 @@ -#pragma once - -#include - -#include "duckdb/main/client_context.hpp" -#include "duckdb/optimizer/optimizer_extension.hpp" - -#include "dbconnector/query/query_writer.hpp" - -namespace dbconnector { -namespace optimizer { - -typedef bool (*should_push_aggregate_t)(duckdb::ClientContext &context, duckdb::LogicalAggregate &aggr); - -class AggregateOptimizer { -public: - struct Config { - bool enabled = false; - char identifier_quote = '"'; - query::QuoteEscapeStyle escape_style = query::QuoteEscapeStyle::DOUBLE_QUOTE; - std::string table_scan_name; - should_push_aggregate_t should_push_aggregate = nullptr; - }; - - static Config CreateConfig(duckdb::ClientContext &ctx, const std::string &enabled_option, char identifier_quote, - query::QuoteEscapeStyle escape_style, std::string table_scan_name, - should_push_aggregate_t should_push_aggregate = nullptr); - - static void Optimize(const Config &config, duckdb::OptimizerExtensionInput &input, - duckdb::unique_ptr &op); -}; - -} // namespace optimizer -} // namespace dbconnector diff --git a/src/include/dbconnector/optimizer/optimizer_util.hpp b/src/include/dbconnector/optimizer/optimizer_util.hpp index a9ab673..167e097 100644 --- a/src/include/dbconnector/optimizer/optimizer_util.hpp +++ b/src/include/dbconnector/optimizer/optimizer_util.hpp @@ -15,7 +15,6 @@ namespace optimizer { struct TracedBindingColumn { std::string col_name; duckdb::LogicalType col_type; - duckdb::idx_t col_idx = 0; bool Found() { return !col_name.empty(); diff --git a/src/optimizer/aggregate_optimizer.cpp b/src/optimizer/aggregate_optimizer.cpp deleted file mode 100644 index 0d01a29..0000000 --- a/src/optimizer/aggregate_optimizer.cpp +++ /dev/null @@ -1,339 +0,0 @@ -#include "dbconnector/optimizer/aggregate_optimizer.hpp" - -#include "fmt/format.h" - -#include "duckdb/common/types/value.hpp" -#include "duckdb/planner/operator/logical_aggregate.hpp" -#include "duckdb/planner/operator/logical_comparison_join.hpp" -#include "duckdb/planner/operator/logical_get.hpp" -#include "duckdb/planner/operator/logical_limit.hpp" -#include "duckdb/planner/operator/logical_order.hpp" -#include "duckdb/planner/operator/logical_top_n.hpp" -#include "duckdb/planner/operator/logical_projection.hpp" -#include "duckdb/planner/expression/bound_aggregate_expression.hpp" -#include "duckdb/planner/expression/bound_columnref_expression.hpp" -#include "duckdb/planner/expression_iterator.hpp" - -#include "dbconnector/bind_data.hpp" -#include "dbconnector/optimizer/aggregate_bind_data.hpp" -#include "dbconnector/optimizer/optimizer_util.hpp" -#include "dbconnector/query/query_writer.hpp" -#include "dbconnector/table_scan/filter_pushdown.hpp" - -namespace dbconnector { -namespace optimizer { - -using namespace duckdb; - -AggregateOptimizer::Config AggregateOptimizer::CreateConfig(ClientContext &ctx, const std::string &enabled_option, - char identifier_quote, query::QuoteEscapeStyle escape_style, - std::string table_scan_name, - should_push_aggregate_t should_push_aggregate) { - Config res; - - res.enabled = false; - Value enabled_val; - if (ctx.TryGetCurrentSetting(enabled_option, enabled_val) && !enabled_val.IsNull()) { - res.enabled = BooleanValue::Get(enabled_val); - } - - res.identifier_quote = identifier_quote; - res.escape_style = escape_style; - res.table_scan_name = std::move(table_scan_name); - res.should_push_aggregate = should_push_aggregate; - - return res; -} - -static const unordered_set PUSHABLE_AGGREGATES = {"count_star", "count", "sum", "avg", "min", "max"}; - -static bool CanPushAggregate(LogicalAggregate &aggr) { - if (aggr.grouping_sets.size() > 1) { - return false; - } - if (!aggr.grouping_functions.empty()) { - return false; - } - for (auto &group : aggr.groups) { - if (group->GetExpressionClass() != ExpressionClass::BOUND_COLUMN_REF) { - return false; - } - } - for (auto &expr : aggr.expressions) { - if (expr->GetExpressionClass() != ExpressionClass::BOUND_AGGREGATE) { - return false; - } - auto &agg_expr = expr->Cast(); - if (agg_expr.IsDistinct()) { - return false; - } - if (agg_expr.GetFilterMutable()) { - return false; - } - if (agg_expr.GetOrderBysMutable() && !agg_expr.GetOrderBysMutable()->orders.empty()) { - return false; - } - if (PUSHABLE_AGGREGATES.find(agg_expr.FunctionMutable().GetName().GetIdentifierName()) == - PUSHABLE_AGGREGATES.end()) { - return false; - } - if (agg_expr.FunctionMutable().GetName() != "count_star") { - if (agg_expr.GetChildrenMutable().size() != 1) { - return false; - } - if (agg_expr.GetChildrenMutable()[0]->GetExpressionClass() != ExpressionClass::BOUND_COLUMN_REF) { - return false; - } - } - } - return true; -} - -struct PushedAggregate { - string select_list; - string group_by_clause; - string where_clause; - bool success = false; - - bool PushedDown() { - return success; - } -}; - -static PushedAggregate TryPushAggregateToMySQL(const AggregateOptimizer::Config &config, LogicalAggregate &aggr, - LogicalOperator &aggr_child, LogicalGet &get) { - PushedAggregate res; - if (!CanPushAggregate(aggr)) { - return res; - } - - auto query_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style); - vector select_fragments; - vector group_names; - vector new_types; - vector new_names; - - for (auto &group : aggr.groups) { - auto &col_ref = group->Cast(); - TracedBindingColumn traced_binding = - OptimizerUtil::TraceBindingToColumn(col_ref.BindingMutable(), aggr_child, get); - if (!traced_binding.Found()) { - return res; - } - string quoted = query::QueryWriter::WriteQuotedAndEscaped(query_config, traced_binding.col_name); - select_fragments.push_back(quoted); - group_names.push_back(quoted); - new_types.push_back(traced_binding.col_type); - new_names.push_back(traced_binding.col_name); - } - - idx_t agg_idx = 0; - for (auto &expr : aggr.expressions) { - auto &agg_expr = expr->Cast(); - string fragment; - string alias = "_agg_" + to_string(agg_idx); - - if (agg_expr.FunctionMutable().GetName() == "count_star") { - fragment = "COUNT(*) AS " + query::QueryWriter::WriteQuotedAndEscaped(query_config, alias); - } else { - auto &child_ref = agg_expr.GetChildrenMutable()[0]->Cast(); - TracedBindingColumn traced_binding = - OptimizerUtil::TraceBindingToColumn(child_ref.BindingMutable(), aggr_child, get); - if (!traced_binding.Found()) { - return res; - } - string quoted_col = query::QueryWriter::WriteQuotedAndEscaped(query_config, traced_binding.col_name); - string func_upper; - if (agg_expr.FunctionMutable().GetName() == "count") { - func_upper = "COUNT"; - } else if (agg_expr.FunctionMutable().GetName() == "sum") { - func_upper = "SUM"; - } else if (agg_expr.FunctionMutable().GetName() == "avg") { - func_upper = "AVG"; - } else if (agg_expr.FunctionMutable().GetName() == "min") { - func_upper = "MIN"; - } else if (agg_expr.FunctionMutable().GetName() == "max") { - func_upper = "MAX"; - } else { - return res; - } - if (func_upper == "SUM" && traced_binding.col_type.id() == LogicalTypeId::DECIMAL) { - return res; - } - if (func_upper == "AVG" && traced_binding.col_type.id() == LogicalTypeId::DECIMAL) { - return res; - } - string agg_sql = func_upper + "(" + quoted_col + ")"; - if (func_upper == "AVG") { - agg_sql = "CAST(" + agg_sql + " AS DOUBLE)"; - } - fragment = agg_sql + " AS " + query::QueryWriter::WriteQuotedAndEscaped(query_config, alias); - } - - select_fragments.push_back(fragment); - new_types.push_back(agg_expr.GetReturnType()); - new_names.push_back(alias); - agg_idx++; - } - - res.select_list = StringUtil::Join(select_fragments, ", "); - - if (!group_names.empty()) { - res.group_by_clause = " GROUP BY " + StringUtil::Join(group_names, ", "); - } - - if (get.table_filters.HasFilters()) { - string where_clause; - auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style, - query::Dialect::Postgres); - for (auto &entry : get.table_filters) { - ProjectionIndex proj_idx = entry.GetIndex(); - ColumnIndex col_idx = get.GetColumnIndex(proj_idx); - column_t table_col_idx = col_idx.GetPrimaryIndex(); - if (table_col_idx >= get.names.size()) { - return res; - } - auto column_name = get.names[table_col_idx]; - auto new_filter = table_scan::FilterPushdown::TransformFilter(scan_config, column_name.GetIdentifierName(), - entry.Filter(), table_col_idx); - if (new_filter.empty()) { - // A partial WHERE under a remote aggregate returns wrong numbers; - // there is no local re-check once the rows are aggregated away. - return res; - } - if (!where_clause.empty()) { - where_clause += " AND "; - } - where_clause += new_filter; - } - if (!where_clause.empty()) { - res.where_clause = where_clause; - } - } - - get.returned_types = new_types; - get.names.clear(); - for (auto &new_name : new_names) { - get.names.push_back(Identifier(new_name)); - } - vector new_column_ids; - for (idx_t i = 0; i < new_types.size(); i++) { - new_column_ids.push_back(ColumnIndex(i)); - } - get.SetColumnIds(std::move(new_column_ids)); - get.projection_ids.clear(); - get.table_filters.ClearFilters(); - - res.success = true; - return res; -} - -struct AggregateRewriteInfo { - TableIndex group_index; - TableIndex aggregate_index; - TableIndex scan_table_index; - idx_t num_groups; -}; - -static void RewriteExpression(unique_ptr &expr, AggregateRewriteInfo &info) { - if (expr->GetExpressionClass() == ExpressionClass::BOUND_COLUMN_REF) { - auto &col_ref = expr->Cast(); - if (col_ref.Depth() > 0) { - return; - } - if (col_ref.BindingMutable().table_index == info.group_index) { - col_ref.BindingMutable().table_index = info.scan_table_index; - } else if (col_ref.BindingMutable().table_index == info.aggregate_index) { - col_ref.BindingMutable().table_index = info.scan_table_index; - col_ref.BindingMutable().column_index = - ProjectionIndex(col_ref.BindingMutable().column_index.GetIndex() + info.num_groups); - } - } - ExpressionIterator::EnumerateChildren(*expr, - [&](unique_ptr &child) { RewriteExpression(child, info); }); -} - -static void RewriteBindingsInOperator(LogicalOperator &op, AggregateRewriteInfo &info) { - for (auto &expr : op.expressions) { - RewriteExpression(expr, info); - } - if (op.type == LogicalOperatorType::LOGICAL_ORDER_BY) { - auto &order = op.Cast(); - for (auto &node : order.orders) { - RewriteExpression(node.expression, info); - } - } - if (op.type == LogicalOperatorType::LOGICAL_TOP_N) { - auto &topn = op.Cast(); - for (auto &node : topn.orders) { - RewriteExpression(node.expression, info); - } - } - if (op.type == LogicalOperatorType::LOGICAL_COMPARISON_JOIN) { - auto &join = op.Cast(); - for (auto &cond : join.conditions) { - RewriteExpression(cond.LeftReference(), info); - RewriteExpression(cond.RightReference(), info); - } - } -} - -static void RewriteBindingsInTree(LogicalOperator &op, AggregateRewriteInfo &info) { - RewriteBindingsInOperator(op, info); - for (auto &child : op.children) { - RewriteBindingsInTree(*child, info); - } -} - -static void OptimizeAggregates(const AggregateOptimizer::Config &config, ClientContext &context, - unique_ptr &op, vector &rewrites) { - if (!config.enabled) { - return; - } - - for (idx_t i = 0; i < op->children.size(); i++) { - auto &child = op->children[i]; - if (child->type == LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY) { - auto &aggr = child->Cast(); - LogicalGet *get = nullptr; - dbconnector::BindData *bind_data = nullptr; - if (!aggr.children.empty() && - OptimizerUtil::FindExtensionGet(config.table_scan_name, *aggr.children[0], get, bind_data)) { - if (config.should_push_aggregate && !config.should_push_aggregate(context, aggr)) { - OptimizeAggregates(config, context, child, rewrites); - continue; - } - - PushedAggregate pushed_aggr = TryPushAggregateToMySQL(config, aggr, *aggr.children[0], *get); - auto &aggr_bind_data = bind_data->GetAggregateBindData(); - aggr_bind_data.aggregate_select_list = pushed_aggr.select_list; - aggr_bind_data.group_by_clause = pushed_aggr.group_by_clause; - aggr_bind_data.aggregate_where_clause = pushed_aggr.where_clause; - aggr_bind_data.has_aggregate_pushdown = pushed_aggr.PushedDown(); - if (pushed_aggr.PushedDown()) { - AggregateRewriteInfo info; - info.group_index = aggr.group_index; - info.aggregate_index = aggr.aggregate_index; - info.scan_table_index = get->table_index; - info.num_groups = aggr.groups.size(); - rewrites.push_back(info); - op->children[i] = std::move(aggr.children[0]); - continue; - } - } - } - OptimizeAggregates(config, context, child, rewrites); - } -} - -void AggregateOptimizer::Optimize(const AggregateOptimizer::Config &config, OptimizerExtensionInput &input, - unique_ptr &op) { - vector rewrites; - OptimizeAggregates(config, input.context, op, rewrites); - for (auto &info : rewrites) { - RewriteBindingsInTree(*op, info); - } -} - -} // namespace optimizer -} // namespace dbconnector diff --git a/src/optimizer/optimizer_util.cpp b/src/optimizer/optimizer_util.cpp index 0e005cc..19368cf 100644 --- a/src/optimizer/optimizer_util.cpp +++ b/src/optimizer/optimizer_util.cpp @@ -70,7 +70,6 @@ TracedBindingColumn OptimizerUtil::TraceBindingToColumn(ColumnBinding binding, L } res.col_name = get.names[actual_col_idx].GetIdentifierName(); res.col_type = get.returned_types[actual_col_idx]; - res.col_idx = actual_col_idx; return res; } diff --git a/test/extension/CMakeLists.txt b/test/extension/CMakeLists.txt index 1454702..abe8415 100644 --- a/test/extension/CMakeLists.txt +++ b/test/extension/CMakeLists.txt @@ -13,7 +13,6 @@ if(NOT DEFINED ENV{DUCKDB_SRC_PATH}) endif() add_executable(${PROJECT_NAME} - ../../src/optimizer/aggregate_optimizer.cpp ../../src/optimizer/optimizer_util.cpp ../../src/optimizer/order_by_and_limit_optimizer.cpp ../../src/query/query_writer.cpp From 601247578283bdb0079fde192ffa336f5d7ddd9d Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Thu, 16 Jul 2026 22:29:18 +0000 Subject: [PATCH 13/14] Revert "chore: delete the consumer-less AggregateOptimizer" This reverts commit e38e6b7fb1e450318bf250368c5df2ab93803911. --- src/include/dbconnector/bind_data.hpp | 3 + .../optimizer/aggregate_bind_data.hpp | 16 + .../optimizer/aggregate_optimizer.hpp | 34 ++ .../dbconnector/optimizer/optimizer_util.hpp | 1 + src/optimizer/aggregate_optimizer.cpp | 339 ++++++++++++++++++ src/optimizer/optimizer_util.cpp | 1 + test/extension/CMakeLists.txt | 1 + 7 files changed, 395 insertions(+) create mode 100644 src/include/dbconnector/optimizer/aggregate_bind_data.hpp create mode 100644 src/include/dbconnector/optimizer/aggregate_optimizer.hpp create mode 100644 src/optimizer/aggregate_optimizer.cpp diff --git a/src/include/dbconnector/bind_data.hpp b/src/include/dbconnector/bind_data.hpp index 3ebb7e9..4e81dfd 100644 --- a/src/include/dbconnector/bind_data.hpp +++ b/src/include/dbconnector/bind_data.hpp @@ -2,12 +2,15 @@ #include "duckdb/function/function.hpp" +#include "dbconnector/optimizer/aggregate_bind_data.hpp" #include "dbconnector/optimizer/order_by_and_limit_bind_data.hpp" namespace dbconnector { class BindData : public duckdb::FunctionData { public: + virtual optimizer::AggregateBindData &GetAggregateBindData() = 0; + virtual optimizer::OrderByAndLimitBindData &GetOrderByAndLimitBindData() = 0; }; diff --git a/src/include/dbconnector/optimizer/aggregate_bind_data.hpp b/src/include/dbconnector/optimizer/aggregate_bind_data.hpp new file mode 100644 index 0000000..7528650 --- /dev/null +++ b/src/include/dbconnector/optimizer/aggregate_bind_data.hpp @@ -0,0 +1,16 @@ +#pragma once + +#include + +namespace dbconnector { +namespace optimizer { + +struct AggregateBindData { + std::string aggregate_select_list; + std::string group_by_clause; + std::string aggregate_where_clause; + bool has_aggregate_pushdown = false; +}; + +} // namespace optimizer +} // namespace dbconnector diff --git a/src/include/dbconnector/optimizer/aggregate_optimizer.hpp b/src/include/dbconnector/optimizer/aggregate_optimizer.hpp new file mode 100644 index 0000000..447f9eb --- /dev/null +++ b/src/include/dbconnector/optimizer/aggregate_optimizer.hpp @@ -0,0 +1,34 @@ +#pragma once + +#include + +#include "duckdb/main/client_context.hpp" +#include "duckdb/optimizer/optimizer_extension.hpp" + +#include "dbconnector/query/query_writer.hpp" + +namespace dbconnector { +namespace optimizer { + +typedef bool (*should_push_aggregate_t)(duckdb::ClientContext &context, duckdb::LogicalAggregate &aggr); + +class AggregateOptimizer { +public: + struct Config { + bool enabled = false; + char identifier_quote = '"'; + query::QuoteEscapeStyle escape_style = query::QuoteEscapeStyle::DOUBLE_QUOTE; + std::string table_scan_name; + should_push_aggregate_t should_push_aggregate = nullptr; + }; + + static Config CreateConfig(duckdb::ClientContext &ctx, const std::string &enabled_option, char identifier_quote, + query::QuoteEscapeStyle escape_style, std::string table_scan_name, + should_push_aggregate_t should_push_aggregate = nullptr); + + static void Optimize(const Config &config, duckdb::OptimizerExtensionInput &input, + duckdb::unique_ptr &op); +}; + +} // namespace optimizer +} // namespace dbconnector diff --git a/src/include/dbconnector/optimizer/optimizer_util.hpp b/src/include/dbconnector/optimizer/optimizer_util.hpp index 167e097..a9ab673 100644 --- a/src/include/dbconnector/optimizer/optimizer_util.hpp +++ b/src/include/dbconnector/optimizer/optimizer_util.hpp @@ -15,6 +15,7 @@ namespace optimizer { struct TracedBindingColumn { std::string col_name; duckdb::LogicalType col_type; + duckdb::idx_t col_idx = 0; bool Found() { return !col_name.empty(); diff --git a/src/optimizer/aggregate_optimizer.cpp b/src/optimizer/aggregate_optimizer.cpp new file mode 100644 index 0000000..0d01a29 --- /dev/null +++ b/src/optimizer/aggregate_optimizer.cpp @@ -0,0 +1,339 @@ +#include "dbconnector/optimizer/aggregate_optimizer.hpp" + +#include "fmt/format.h" + +#include "duckdb/common/types/value.hpp" +#include "duckdb/planner/operator/logical_aggregate.hpp" +#include "duckdb/planner/operator/logical_comparison_join.hpp" +#include "duckdb/planner/operator/logical_get.hpp" +#include "duckdb/planner/operator/logical_limit.hpp" +#include "duckdb/planner/operator/logical_order.hpp" +#include "duckdb/planner/operator/logical_top_n.hpp" +#include "duckdb/planner/operator/logical_projection.hpp" +#include "duckdb/planner/expression/bound_aggregate_expression.hpp" +#include "duckdb/planner/expression/bound_columnref_expression.hpp" +#include "duckdb/planner/expression_iterator.hpp" + +#include "dbconnector/bind_data.hpp" +#include "dbconnector/optimizer/aggregate_bind_data.hpp" +#include "dbconnector/optimizer/optimizer_util.hpp" +#include "dbconnector/query/query_writer.hpp" +#include "dbconnector/table_scan/filter_pushdown.hpp" + +namespace dbconnector { +namespace optimizer { + +using namespace duckdb; + +AggregateOptimizer::Config AggregateOptimizer::CreateConfig(ClientContext &ctx, const std::string &enabled_option, + char identifier_quote, query::QuoteEscapeStyle escape_style, + std::string table_scan_name, + should_push_aggregate_t should_push_aggregate) { + Config res; + + res.enabled = false; + Value enabled_val; + if (ctx.TryGetCurrentSetting(enabled_option, enabled_val) && !enabled_val.IsNull()) { + res.enabled = BooleanValue::Get(enabled_val); + } + + res.identifier_quote = identifier_quote; + res.escape_style = escape_style; + res.table_scan_name = std::move(table_scan_name); + res.should_push_aggregate = should_push_aggregate; + + return res; +} + +static const unordered_set PUSHABLE_AGGREGATES = {"count_star", "count", "sum", "avg", "min", "max"}; + +static bool CanPushAggregate(LogicalAggregate &aggr) { + if (aggr.grouping_sets.size() > 1) { + return false; + } + if (!aggr.grouping_functions.empty()) { + return false; + } + for (auto &group : aggr.groups) { + if (group->GetExpressionClass() != ExpressionClass::BOUND_COLUMN_REF) { + return false; + } + } + for (auto &expr : aggr.expressions) { + if (expr->GetExpressionClass() != ExpressionClass::BOUND_AGGREGATE) { + return false; + } + auto &agg_expr = expr->Cast(); + if (agg_expr.IsDistinct()) { + return false; + } + if (agg_expr.GetFilterMutable()) { + return false; + } + if (agg_expr.GetOrderBysMutable() && !agg_expr.GetOrderBysMutable()->orders.empty()) { + return false; + } + if (PUSHABLE_AGGREGATES.find(agg_expr.FunctionMutable().GetName().GetIdentifierName()) == + PUSHABLE_AGGREGATES.end()) { + return false; + } + if (agg_expr.FunctionMutable().GetName() != "count_star") { + if (agg_expr.GetChildrenMutable().size() != 1) { + return false; + } + if (agg_expr.GetChildrenMutable()[0]->GetExpressionClass() != ExpressionClass::BOUND_COLUMN_REF) { + return false; + } + } + } + return true; +} + +struct PushedAggregate { + string select_list; + string group_by_clause; + string where_clause; + bool success = false; + + bool PushedDown() { + return success; + } +}; + +static PushedAggregate TryPushAggregateToMySQL(const AggregateOptimizer::Config &config, LogicalAggregate &aggr, + LogicalOperator &aggr_child, LogicalGet &get) { + PushedAggregate res; + if (!CanPushAggregate(aggr)) { + return res; + } + + auto query_config = query::QueryWriter::CreateConfig(config.identifier_quote, config.escape_style); + vector select_fragments; + vector group_names; + vector new_types; + vector new_names; + + for (auto &group : aggr.groups) { + auto &col_ref = group->Cast(); + TracedBindingColumn traced_binding = + OptimizerUtil::TraceBindingToColumn(col_ref.BindingMutable(), aggr_child, get); + if (!traced_binding.Found()) { + return res; + } + string quoted = query::QueryWriter::WriteQuotedAndEscaped(query_config, traced_binding.col_name); + select_fragments.push_back(quoted); + group_names.push_back(quoted); + new_types.push_back(traced_binding.col_type); + new_names.push_back(traced_binding.col_name); + } + + idx_t agg_idx = 0; + for (auto &expr : aggr.expressions) { + auto &agg_expr = expr->Cast(); + string fragment; + string alias = "_agg_" + to_string(agg_idx); + + if (agg_expr.FunctionMutable().GetName() == "count_star") { + fragment = "COUNT(*) AS " + query::QueryWriter::WriteQuotedAndEscaped(query_config, alias); + } else { + auto &child_ref = agg_expr.GetChildrenMutable()[0]->Cast(); + TracedBindingColumn traced_binding = + OptimizerUtil::TraceBindingToColumn(child_ref.BindingMutable(), aggr_child, get); + if (!traced_binding.Found()) { + return res; + } + string quoted_col = query::QueryWriter::WriteQuotedAndEscaped(query_config, traced_binding.col_name); + string func_upper; + if (agg_expr.FunctionMutable().GetName() == "count") { + func_upper = "COUNT"; + } else if (agg_expr.FunctionMutable().GetName() == "sum") { + func_upper = "SUM"; + } else if (agg_expr.FunctionMutable().GetName() == "avg") { + func_upper = "AVG"; + } else if (agg_expr.FunctionMutable().GetName() == "min") { + func_upper = "MIN"; + } else if (agg_expr.FunctionMutable().GetName() == "max") { + func_upper = "MAX"; + } else { + return res; + } + if (func_upper == "SUM" && traced_binding.col_type.id() == LogicalTypeId::DECIMAL) { + return res; + } + if (func_upper == "AVG" && traced_binding.col_type.id() == LogicalTypeId::DECIMAL) { + return res; + } + string agg_sql = func_upper + "(" + quoted_col + ")"; + if (func_upper == "AVG") { + agg_sql = "CAST(" + agg_sql + " AS DOUBLE)"; + } + fragment = agg_sql + " AS " + query::QueryWriter::WriteQuotedAndEscaped(query_config, alias); + } + + select_fragments.push_back(fragment); + new_types.push_back(agg_expr.GetReturnType()); + new_names.push_back(alias); + agg_idx++; + } + + res.select_list = StringUtil::Join(select_fragments, ", "); + + if (!group_names.empty()) { + res.group_by_clause = " GROUP BY " + StringUtil::Join(group_names, ", "); + } + + if (get.table_filters.HasFilters()) { + string where_clause; + auto scan_config = table_scan::FilterPushdown::CreateConfig('`', '\'', config.escape_style, + query::Dialect::Postgres); + for (auto &entry : get.table_filters) { + ProjectionIndex proj_idx = entry.GetIndex(); + ColumnIndex col_idx = get.GetColumnIndex(proj_idx); + column_t table_col_idx = col_idx.GetPrimaryIndex(); + if (table_col_idx >= get.names.size()) { + return res; + } + auto column_name = get.names[table_col_idx]; + auto new_filter = table_scan::FilterPushdown::TransformFilter(scan_config, column_name.GetIdentifierName(), + entry.Filter(), table_col_idx); + if (new_filter.empty()) { + // A partial WHERE under a remote aggregate returns wrong numbers; + // there is no local re-check once the rows are aggregated away. + return res; + } + if (!where_clause.empty()) { + where_clause += " AND "; + } + where_clause += new_filter; + } + if (!where_clause.empty()) { + res.where_clause = where_clause; + } + } + + get.returned_types = new_types; + get.names.clear(); + for (auto &new_name : new_names) { + get.names.push_back(Identifier(new_name)); + } + vector new_column_ids; + for (idx_t i = 0; i < new_types.size(); i++) { + new_column_ids.push_back(ColumnIndex(i)); + } + get.SetColumnIds(std::move(new_column_ids)); + get.projection_ids.clear(); + get.table_filters.ClearFilters(); + + res.success = true; + return res; +} + +struct AggregateRewriteInfo { + TableIndex group_index; + TableIndex aggregate_index; + TableIndex scan_table_index; + idx_t num_groups; +}; + +static void RewriteExpression(unique_ptr &expr, AggregateRewriteInfo &info) { + if (expr->GetExpressionClass() == ExpressionClass::BOUND_COLUMN_REF) { + auto &col_ref = expr->Cast(); + if (col_ref.Depth() > 0) { + return; + } + if (col_ref.BindingMutable().table_index == info.group_index) { + col_ref.BindingMutable().table_index = info.scan_table_index; + } else if (col_ref.BindingMutable().table_index == info.aggregate_index) { + col_ref.BindingMutable().table_index = info.scan_table_index; + col_ref.BindingMutable().column_index = + ProjectionIndex(col_ref.BindingMutable().column_index.GetIndex() + info.num_groups); + } + } + ExpressionIterator::EnumerateChildren(*expr, + [&](unique_ptr &child) { RewriteExpression(child, info); }); +} + +static void RewriteBindingsInOperator(LogicalOperator &op, AggregateRewriteInfo &info) { + for (auto &expr : op.expressions) { + RewriteExpression(expr, info); + } + if (op.type == LogicalOperatorType::LOGICAL_ORDER_BY) { + auto &order = op.Cast(); + for (auto &node : order.orders) { + RewriteExpression(node.expression, info); + } + } + if (op.type == LogicalOperatorType::LOGICAL_TOP_N) { + auto &topn = op.Cast(); + for (auto &node : topn.orders) { + RewriteExpression(node.expression, info); + } + } + if (op.type == LogicalOperatorType::LOGICAL_COMPARISON_JOIN) { + auto &join = op.Cast(); + for (auto &cond : join.conditions) { + RewriteExpression(cond.LeftReference(), info); + RewriteExpression(cond.RightReference(), info); + } + } +} + +static void RewriteBindingsInTree(LogicalOperator &op, AggregateRewriteInfo &info) { + RewriteBindingsInOperator(op, info); + for (auto &child : op.children) { + RewriteBindingsInTree(*child, info); + } +} + +static void OptimizeAggregates(const AggregateOptimizer::Config &config, ClientContext &context, + unique_ptr &op, vector &rewrites) { + if (!config.enabled) { + return; + } + + for (idx_t i = 0; i < op->children.size(); i++) { + auto &child = op->children[i]; + if (child->type == LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY) { + auto &aggr = child->Cast(); + LogicalGet *get = nullptr; + dbconnector::BindData *bind_data = nullptr; + if (!aggr.children.empty() && + OptimizerUtil::FindExtensionGet(config.table_scan_name, *aggr.children[0], get, bind_data)) { + if (config.should_push_aggregate && !config.should_push_aggregate(context, aggr)) { + OptimizeAggregates(config, context, child, rewrites); + continue; + } + + PushedAggregate pushed_aggr = TryPushAggregateToMySQL(config, aggr, *aggr.children[0], *get); + auto &aggr_bind_data = bind_data->GetAggregateBindData(); + aggr_bind_data.aggregate_select_list = pushed_aggr.select_list; + aggr_bind_data.group_by_clause = pushed_aggr.group_by_clause; + aggr_bind_data.aggregate_where_clause = pushed_aggr.where_clause; + aggr_bind_data.has_aggregate_pushdown = pushed_aggr.PushedDown(); + if (pushed_aggr.PushedDown()) { + AggregateRewriteInfo info; + info.group_index = aggr.group_index; + info.aggregate_index = aggr.aggregate_index; + info.scan_table_index = get->table_index; + info.num_groups = aggr.groups.size(); + rewrites.push_back(info); + op->children[i] = std::move(aggr.children[0]); + continue; + } + } + } + OptimizeAggregates(config, context, child, rewrites); + } +} + +void AggregateOptimizer::Optimize(const AggregateOptimizer::Config &config, OptimizerExtensionInput &input, + unique_ptr &op) { + vector rewrites; + OptimizeAggregates(config, input.context, op, rewrites); + for (auto &info : rewrites) { + RewriteBindingsInTree(*op, info); + } +} + +} // namespace optimizer +} // namespace dbconnector diff --git a/src/optimizer/optimizer_util.cpp b/src/optimizer/optimizer_util.cpp index 19368cf..0e005cc 100644 --- a/src/optimizer/optimizer_util.cpp +++ b/src/optimizer/optimizer_util.cpp @@ -70,6 +70,7 @@ TracedBindingColumn OptimizerUtil::TraceBindingToColumn(ColumnBinding binding, L } res.col_name = get.names[actual_col_idx].GetIdentifierName(); res.col_type = get.returned_types[actual_col_idx]; + res.col_idx = actual_col_idx; return res; } diff --git a/test/extension/CMakeLists.txt b/test/extension/CMakeLists.txt index abe8415..1454702 100644 --- a/test/extension/CMakeLists.txt +++ b/test/extension/CMakeLists.txt @@ -13,6 +13,7 @@ if(NOT DEFINED ENV{DUCKDB_SRC_PATH}) endif() add_executable(${PROJECT_NAME} + ../../src/optimizer/aggregate_optimizer.cpp ../../src/optimizer/optimizer_util.cpp ../../src/optimizer/order_by_and_limit_optimizer.cpp ../../src/query/query_writer.cpp From 2484b0fd8df235aefc6f806130fc586e68de351b Mon Sep 17 00:00:00 2001 From: Pavel && Konstantin Vedernikoffs Date: Sat, 18 Jul 2026 02:23:15 +0000 Subject: [PATCH 14/14] refactor(filter): recognise optional conjuncts via duckdb's IsRootOptionalExpression The all-or-nothing renderer hand-matched three internal filter-function names (optional / selectivity-optional / dynamic) to decide which conjuncts are safe to drop from an AND. Reuse duckdb's own recogniser ExpressionFilter::IsRootOptionalExpression instead: it takes an Expression directly (so the BOUND_FUNCTION guard and the local helper are gone) and matches the optional / selectivity-optional wrappers -- a dynamic filter rides inside one of those wrappers, so the set is equivalent. These wrappers are advisory: the query's real predicate is enforced by the operators above the scan, so dropping one from the pushed SQL only widens the scan, never the result. --- src/table_scan/filter_pushdown.cpp | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/table_scan/filter_pushdown.cpp b/src/table_scan/filter_pushdown.cpp index e96bfe4..5725991 100644 --- a/src/table_scan/filter_pushdown.cpp +++ b/src/table_scan/filter_pushdown.cpp @@ -6,6 +6,7 @@ #include "duckdb/planner/expression/bound_constant_expression.hpp" #include "duckdb/planner/expression/bound_function_expression.hpp" #include "duckdb/planner/expression/bound_operator_expression.hpp" +#include "duckdb/planner/filter/expression_filter.hpp" #include "duckdb/planner/filter/table_filter_functions.hpp" #include "duckdb/common/enum_util.hpp" #include "duckdb/common/string_util.hpp" @@ -32,15 +33,6 @@ FilterPushdown::Config FilterPushdown::CreateConfig(char identifier_quote, char return res; } -static bool IsOptionalFilterExpression(const Expression &expr) { - if (expr.GetExpressionClass() != ExpressionClass::BOUND_FUNCTION) { - return false; - } - auto &name = expr.Cast().Function().GetName(); - return name == OptionalFilterScalarFun::NAME || name == SelectivityOptionalFilterScalarFun::NAME || - name == DynamicFilterScalarFun::NAME; -} - std::string FilterPushdown::CreateExpression(const query::QueryWriter::Config &identifier_config, const query::QueryWriter::Config &constant_config, const std::string &column_name, @@ -51,12 +43,12 @@ std::string FilterPushdown::CreateExpression(const query::QueryWriter::Config &i for (auto &filter : filters) { auto new_filter = TransformExpression(identifier_config, constant_config, column_name, *filter, column_id); if (new_filter.empty()) { - // Optional (advisory) pieces may be dropped from an AND (widens the - // SQL, never the result). Dropping anything from an OR narrows the - // result, and dropping a required AND conjunct widens the SQL past - // the filter -- both make the SQL lie, so the whole filter renders - // empty and stays local. - if (!is_or && IsOptionalFilterExpression(*filter)) { + // An optional (advisory) filter wrapper may be dropped from an AND: it + // widens the SQL, never the result (the real predicate is enforced above + // the scan). Dropping anything from an OR narrows the result, and dropping + // a real AND conjunct widens the SQL past the filter -- both make the SQL + // lie, so the whole filter renders empty and stays local. + if (!is_or && ExpressionFilter::IsRootOptionalExpression(*filter)) { continue; } return std::string();