Preserve NULL semantics in IN-family pushdown - #315
Conversation
theory
left a comment
There was a problem hiding this comment.
If I read this right, we depend on the Postgres catalog to determine whether a var is NULLable. But it of course can lie: one can have a column marked NOT NULL in Postgres that actually has NULLs in ClickHouse. I think that's acceptable, given that users can update their foreign tables as appropriate.
| locally. Declaring columns `NOT NULL` maximizes pushdown; `IMPORT FOREIGN | ||
| SCHEMA` does so automatically for non-`Nullable` ClickHouse columns. |
There was a problem hiding this comment.
Link to import foreign schema in this doc.
| * WHERE/JOIN/HAVING condition, where a NULL qualifies a row exactly like | ||
| * FALSE; true anywhere the computed value itself is observable (target |
There was a problem hiding this comment.
I presume this is because we explicitly set join_use_nulls to match the SQL standard. Please mention that fact here if it is in fact relevant.
Perhaps ClickHouse should add a setting for SQL standard NULL behavior in IN(), too.
There was a problem hiding this comment.
Yeah, I think you've discovered this in your own exploration, but for posterity: even join_use_nulls isn't the full solution here. The transform_null_in option is the closest cousin to what we need, but it doesn't do the three-value logic we need to match Postgres.
There was a problem hiding this comment.
Yah, hence my comment on ClickHouse/ClickHouse#34737.
| -- A NULL array constant survives const-folding when the probe is a column; | ||
| -- inspecting it for the IN-list deparse would detoast a null datum. Local. | ||
| EXPLAIN (VERBOSE, COSTS OFF) | ||
| SELECT id FROM tnull WHERE xn = ANY(NULL::int[]) ORDER BY id; |
There was a problem hiding this comment.
Surely this always evaluates to false and returns no rows. Maybe we could avoid sending it to clickHouse at all.
There was a problem hiding this comment.
PG doesn't fold it, and I didn't spend code folding it because who tf would write this, but this PR goes as far as to remove the crash (by not shipping). Sound good?
I.e. yes, it always returns false, yes, we could optimize it, no, PG doesn't and I haven't, because who cares? Bug me for a follow-up if the answer is "you."
There was a problem hiding this comment.
Heh, it's not, I argued against making the same sort of optimization when Philip suggested it. I just thought it might be less complicated here. Fine the way it is.
Mirrors the review-suggested comment edits from the expected file into in_null_semantics.sql, which produces that output. Pins ClickHouse's transform_null_in = 0 through the session_settings default so a server profile cannot silently change the semantics every IN-family rule assumes, asserts the default reaches both drivers in the gucs test, and documents the reliance next to join_use_nulls. Merges the free-floating NULL-semantics block comment into saop_null_semantics_ok's header, which every reference already names. Annotates expr_never_null's Var preconditions, links IMPORT FOREIGN SCHEMA in the docs, and drops the result_map entry for a test with a single expected file. Fixes the changelog's PR references: the pull request is 315, not 313.
| SCHEMA] does so automatically for non-`Nullable` ClickHouse columns. | ||
|
|
||
| These rules assume ClickHouse's default `transform_null_in = 0`, which | ||
| pg_clickhouse pins through the default value of the |
There was a problem hiding this comment.
"pins" is the wrong word, since the user can change it.
bbcf136 to
2044cb2
Compare
theory
left a comment
There was a problem hiding this comment.
Add yourself also co-author to the README!
| ClickHouse SQL computing `<> ALL` (`1 <> ANY('{1,5}')` is true — one | ||
| element differs — but returned false). This shape evaluates locally | ||
| ([#315]). |
There was a problem hiding this comment.
I still don't understand what this is saying, and the mdash signals text extrusion rather than explanation.
There was a problem hiding this comment.
It does love those, yes; my earlier instructions were "use my text from the PR description," but I guess that bears repeating. I'll fold it in.
| empty string to fall back on the ClickHouse server's settings. | ||
| The default is `join_use_nulls 1, group_by_use_nulls 1, final 1, | ||
| transform_null_in 0`. Set it to an empty string to fall back on the | ||
| ClickHouse server's settings — but note that pushdown correctness depends on |
There was a problem hiding this comment.
| ClickHouse server's settings — but note that pushdown correctness depends on | |
| ClickHouse server's settings, but note that pushdown correctness depends on |
| These rules assume ClickHouse's default `transform_null_in = 0`, which | ||
| pg_clickhouse sets on every query through the default value of the | ||
| [`pg_clickhouse.session_settings`](#pg_clickhousesession_settings) parameter | ||
| so that a ClickHouse server profile cannot silently change it. Setting | ||
| `transform_null_in = 1` breaks the semantics of every pushed `IN`. |
There was a problem hiding this comment.
| These rules assume ClickHouse's default `transform_null_in = 0`, which | |
| pg_clickhouse sets on every query through the default value of the | |
| [`pg_clickhouse.session_settings`](#pg_clickhousesession_settings) parameter | |
| so that a ClickHouse server profile cannot silently change it. Setting | |
| `transform_null_in = 1` breaks the semantics of every pushed `IN`. | |
| These rules assume ClickHouse's default `transform_null_in = 0`, which | |
| pg_clickhouse sets on every query through the | |
| [`pg_clickhouse.session_settings`](#pg_clickhousesession_settings) default | |
| so that a ClickHouse server profile cannot change it. Setting | |
| `transform_null_in = 1` breaks the semantics of every pushed `IN`. |
ClickHouse evaluates IN under binary Boolean logic where the Postgres expression can result in NULL, so pushed `IN`-family expressions gave wrong answers whenever a NULL could reach the comparison: `x NOT IN (1, NULL)` returned rows Postgres would drop, meanwhile `has()`/`countEqual()` matched a NULL probe against NULL array elements as values, `x NOT IN (SELECT ...)` over nullable columns returned the plain set complement, and grouping by an `IN` result merged Postgres's `NULL` group into `FALSE`. The same audit caught three walker bugs: 1. `<> ANY(array)` deparsed to SQL computing `<> ALL` (wrong with no NULLs involved), 2. `CASE arg WHEN ..` shipped unchecked when the tested expression was unshippable, and 3. a bare EXPLAIN of `x = ANY(NULL::int[])` crashed the backend when detoasting the null array datum. To see each of these, run the new `in_null_semantics` test and `subplan_pushdown` section 10 against `main`: the wrong rows appear as regression diffs and the `EXPLAIN` as the crash. The shippability walker now tracks how each expression's result is consumed, which I'm generally apprehensive of doing, but it avoids blowing up this PR adding specialized handlers for different contexts. Filter conditions treat `NULL` exactly like `FALSE`, so ClickHouse's `FALSE`-for-`NULL` substitution is invisible there and those shapes ship unchanged. Value positions and negations demand exact three-valued equivalence, established by proving the operands can never be `NULL` (non-NULL constants; NOT NULL columns not nulled by outer joins). `x NOT IN (SELECT ...)` filters keep their pushdown either way: a clean query when the columns prove non-nullable, a guarded one enforcing Postgres's answer when they do not, each guard emitted only when its proof (of non-nullability) fails. Grouped or aggregated subquery bodies without proofs stay local, as do `<> ANY` and `NULL`-array shapes, which will require a different spelling as well that I think deserves a follow-up PR (modulo the pathological case of someone attempting to construct a NULL array as part of the query). `in_null_semantics` carries one expected file across PG 13-19; subplan_pushdown gains a nullable `NOT IN` section on all four run variants.
2edc340 to
215695a
Compare
ClickHouse evaluates IN under binary Boolean logic where the Postgres
expression can result in NULL, so pushed
IN-family expressions gavewrong answers whenever a NULL could reach the comparison:
x NOT IN (1, NULL)returned rows Postgres would drop, meanwhilehas()/countEqual()matched a NULL probe against NULL array elementsas values,
x NOT IN (SELECT ...)over nullable columns returned theplain set complement, and grouping by an
INresult merged Postgres'sNULLgroup intoFALSE. The same audit caught three walker bugs:<> ANY(array)deparsed to SQL computing<> ALL(wrong with no NULLs involved),
CASE arg WHEN ..shipped unchecked when the tested expression wasunshippable, and
x = ANY(NULL::int[])crashed the backend whendetoasting the null array datum.
To see each of these, run the new
in_null_semanticstest andsubplan_pushdownsection 10 againstmain: the wrong rows appear asregression diffs and the
EXPLAINas the crash.The shippability walker now tracks how each expression's result is
consumed, which I'm generally apprehensive of doing, but it avoids
blowing up this PR adding specialized handlers for different contexts.
Filter conditions treat
NULLexactly likeFALSE, so ClickHouse'sFALSE-for-NULLsubstitution is invisible there and those shapesship unchanged. Value positions and negations demand exact three-valued
equivalence, established by proving the operands can never be
NULL(non-NULL constants; NOT NULL columns not nulled by outer joins).
x NOT IN (SELECT ...)filters keep their pushdown either way:a clean query when the columns prove non-nullable, a guarded one
enforcing Postgres's answer when they do not, each guard emitted only
when its proof (of non-nullability) fails. Grouped or aggregated
subquery bodies without proofs stay local, as do
<> ANYandNULL-array shapes, which will require a different spelling as wellthat I think deserves a follow-up PR (modulo the pathological case of
someone attempting to construct a NULL array as part of the query).
in_null_semanticscarries one expected file across PG 13-19;subplan_pushdown gains a nullable
NOT INsection on all four runvariants.