feat(pushdown): dialect-aware filter rendering by pashandor789 · Pull Request #1 · serenedb/database-connector

pashandor789 · 2026-07-15T21:35:21Z

Shared dbconnector groundwork consumed by the ClickHouse connector's migration onto the common pushdown/optimizer layers (and inherited by the postgres connector, which uses the same library).

What

feat(pushdown): dialect-aware, all-or-nothing filter rendering

TransformFilters stops hard-coding postgres-isms (e.g. the unconditional COLLATE "C" on string comparisons): rendering decisions route through a dialect hook the connector supplies, so ClickHouse gets backtick-quoted identifiers and CH-legal literals while postgres keeps its existing output byte-for-byte.
TransformFilter becomes all-or-nothing: a filter either renders SQL equivalent to itself or renders empty (the caller then keeps it local). Previously an unrenderable AND conjunct -- or even an OR branch -- was silently dropped, producing a WHERE wider (or narrower) than the filter while the plan had already deleted it, i.e. silently wrong results the moment such a filter becomes reachable. Only optional/advisory pieces (TopN dynamic filters, zonemap prunes) may still be dropped, since correctness never depends on them. This also closes AggregateOptimizer's latent hole: a partially rendered WHERE under a remote aggregate now stops the fold instead of aggregating over a superset (the optimizer stays in the tree, unregistered, to keep this fork's diff against upstream additive).

feat(optimizer): dialect-rewritten order keys + engine-agnostic NULLS placement

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 (DuckDB sorts NaN above every number; CH compares IEEE), VARCHAR/UUID keys become toString(col) (text-backed columns surface locally as their toString() text, so the byte-wise orders match by construction), and Postgres text keys get COLLATE "C" (locale collation is not DuckDB's byte order -- a latent wrong-order fix for the postgres connector too). Compound keys nesting a divergent scalar cannot be rewritten field-wise and stay local. The filtered-LIMIT-fold policy is derived from the dialect too: postgres pushdown is exact-or-error (WHERE and LIMIT run in one remote statement, in order), so it folds freely; other dialects may re-apply required filters locally, so LIMIT-bearing folds over filtered scans are refused -- fail-closed for future dialects. The connector-tuning surface is exactly one enum: the dialect.
NULLS placement is emulated portably: instead of relying on NULLS FIRST/LAST syntax (which engines support unevenly), the ordering key is prefixed with an IS [NOT] NULL sort key — correct on every SQL engine, and it fixed a NULLS-FIRST divergence observed when the connector is built as a shared lib.

Consumers

ClickHouse connector (feat : Add foreign server serenedb#871): filter pushdown + ORDER BY/LIMIT/TopN pushdown fully on the shared layers, with CH-specific vetoes.
postgres connector: unchanged behavior, now via the hooks' defaults.

Testing

Exercised end-to-end by serenedb's sqllogic suites in #871: pushdown_chscan, pushdown_correctness_chscan, pushdown_divergence_chscan (the divergence corpus pins every veto and the pushed isNaN/toString order keys), optimizer_pushdown_chscan, explain_pushdown_chscan (plan-shape assertions incl. NULLS prefix keys), plus the full _pgscan suite for postgres non-regression.

Part of the FDW/ClickHouse stack: serenedb/serenedb#871 (main PR), serenedb/duckdb#43 (grammar). Land this and #43 first; #871 pins the submodule pointers.

🤖 Generated with Claude Code

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.

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.

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<bool> 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.

…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).

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.

- 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)

- 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

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.

…itmap 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).

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.

…ialect 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.

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.

This reverts commit e38e6b7.

MBkkt · 2026-07-18T02:14:29Z

+static bool IsOptionalFilterExpression(const Expression &expr) {
+	if (expr.GetExpressionClass() != ExpressionClass::BOUND_FUNCTION) {
+		return false;
+	}
+	auto &name = expr.Cast<BoundFunctionExpression>().Function().GetName();
+	return name == OptionalFilterScalarFun::NAME || name == SelectivityOptionalFilterScalarFun::NAME ||
+	       name == DynamicFilterScalarFun::NAME;
+}


inspired by kaldb (use already existing duckdb methods)

…ionalExpression 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.

This was referenced Jul 15, 2026

WIP: PEG grammar / parser changes for FDW CREATE SERVER serenedb/duckdb#43

Open

feat : Add foreign server serenedb/serenedb#871

Draft

Pavel && Konstantin Vedernikoffs added 13 commits July 18, 2026 00:05

Revert "chore: delete the consumer-less AggregateOptimizer"

6012475

This reverts commit e38e6b7.

pashandor789 force-pushed the wip/fdw-clickhouse-dialect branch from aade6ba to 6012475 Compare July 18, 2026 00:31

MBkkt reviewed Jul 18, 2026

View reviewed changes

pashandor789 force-pushed the wip/fdw-clickhouse-dialect branch from 0bb70b3 to 2484b0f Compare July 18, 2026 02:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(pushdown): dialect-aware filter rendering#1

feat(pushdown): dialect-aware filter rendering#1
pashandor789 wants to merge 14 commits into
v2026.06.28from
wip/fdw-clickhouse-dialect

pashandor789 commented Jul 15, 2026 •

edited

Loading

Uh oh!

MBkkt Jul 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pashandor789 commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Consumers

Testing

Uh oh!

MBkkt Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pashandor789 commented Jul 15, 2026 •

edited

Loading