Fix logical error in topK dynamic filtering when the sort column contains a Nullable by groeneai · Pull Request #113406 · ClickHouse/ClickHouse

groeneai · 2026-08-05T02:56:54Z

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix Unexpected return type from __topKFilter logical error raised by ORDER BY ... LIMIT over a MergeTree table with more than one part when the sort column contains a Nullable element, for example Tuple(UInt64, Nullable(UInt64)). use_top_k_dynamic_filtering is enabled by default, so no opt-in setting was required to hit this.

Description

__topKFilter declares its return type as UInt8, but its vectorized fast path returns whatever the comparison it delegates to resolves to. When a sort column contains a Nullable without being Nullable, tuple comparison propagates that nullability outwards and yields Nullable(UInt8), which ExpressionActions rejects. The guard routing to the safe path tested only top level nullability, and DataTypeTuple::isNullable() is false.

The fix tests the resolved comparison type where it is already computed and falls back to the general path when it is not UInt8. That path builds a ColumnUInt8 directly, so it cannot diverge. Keying on the resolved type rather than the argument's structure keeps the optimization for Array(Nullable(T)) and Map(K, Nullable(V)), which resolve to plain UInt8. Correcting an earlier claim of mine: those two are not otherwise correct, because their generic comparison hardcodes NULLs as greatest, so ASC NULLS FIRST drops a nested NULL row. That is pre-existing on master and out of scope; a separate fix follows.

hasEmptyTuple from #114182 gates the same function before the comparison is built, which empty tuples need because building one throws; this guard runs after, so the two are complementary.

Found by the AST fuzzer on an unrelated PR, STID 1611-483a AST fuzzer (amd_tsan). #100742 was an incomplete earlier fix for the same STID: it enumerated the two reported types, so the Tuple carrier remained. Its test is extended here.

Validation, re-run on the merged tree: both shapes in #117013 abort a debug server on default settings and return the expected rows with the fix, while use_top_k_dynamic_filtering = 0 is clean on both arms. Reverting only the guard reddens this PR's test; #114182's own test passes either way. The general path is row at a time, but runs only for arguments that throw today, which Nullable and collated columns already do.

Workflow [PR]
Sync PR [sync-upstream/pr/113406]

…ains a Nullable FunctionTopKFilter::getReturnTypeImpl declares UInt8 unconditionally, but executeVectorized returns whatever the lessOrEquals/greaterOrEquals it delegates to resolves to. FunctionComparison::getReturnTypeImpl propagates Nullable transitively out of tuple elements, so for a sort column that contains a Nullable without being Nullable the fast path produces Nullable(UInt8) and the columnMatchesType guard in ExpressionActions throws LOGICAL_ERROR. The guard meant to route such columns to the safe path tested only top level nullability, and DataTypeTuple does not override IDataType::isNullable(). Test the resolved comparison type where it is already computed and fall back to executeGeneral, which builds a ColumnUInt8 directly and cannot diverge. Keying on the resolved type rather than on the argument's structure is deliberate: a structural predicate such as hasNullable is also true for Array(Nullable(T)) and Map(K, Nullable(V)), whose comparisons resolve to plain UInt8 and work correctly today, so it would silently disable use_top_k_dynamic_filtering for them. executeGeneral also applies nulls_direction, which the lessOrEquals fast path did not, so the general path is both safe and more correct for these types. It is row at a time, but it now runs only for arguments that throw today, and Nullable and collated columns already took it unconditionally. Found by the AST fuzzer on an unrelated PR, STID 1611-483a, reachable from ordinary DDL with no opt-in setting since use_top_k_dynamic_filtering defaults to true. ClickHouse#100742 was an incomplete earlier fix for the same STID: it enumerated the two reported types, Dynamic and Variant, instead of keying on the causal property, so the Tuple carrier remained. Its regression test is extended here with the Tuple, Tuple-of-LowCardinality(Nullable) and NULL bearing arms. The LowCardinality arm pins max_threads = 1 because without it the threshold is not always set before the remaining parts are read, and the arm only reaches the filter in about seven runs out of ten.

The three arms added for the Tuple carriers could pass while covering nothing, and the NULL arm could not observe NULL ordering at all. tests/clickhouse-test randomizes query_plan_max_limit_for_top_k_optimization over [0, 1, 10, 100, 1000, 100000]. The value 1 makes tryOptimizeTopK return early at optimizeTopK.cpp:99 for a LIMIT 5 query, so no __topKFilter prewhere is installed and the arm never reaches the fixed code, in roughly one CI run in six. Pin the setting per query to 100, the form already used by 04205_top_k_dynamic_filtering_variable_length. Each arm also gets an EXPLAIN presence assertion in the count() > 0 ... LIKE '%__topKFilter%' form, so a silently declined optimization is caught rather than passing quietly. The NULL arm used a distinct first tuple element, and ColumnTuple::compareAtImpl returns on the first differing element, so the Nullable element was never compared and nulls_direction was inert: NULLS LAST and NULLS FIRST produced byte-identical output. Make the first element repeat so the Nullable element decides the order. ASC NULLS LAST and ASC NULLS FIRST now differ, and the rows tied at each boundary are identical, so the output does not depend on which tied row is picked. max_threads = 1 on the tuple and NULL arms for the reason already documented on the sibling LowCardinality arm: with several readers the threshold tracker is not always set before the remaining parts are read. Measured against a binary with the guard reverted, the arms aborted 18/20 and 19/20 without it and 20/20 with it, and the reference is byte-identical either way. SYSTEM STOP MERGES keeps the three inserts as three parts, since a single part never sets the tracker. Removing the pin flips the EXPLAIN assertion from 1 to 0 on all three arms, which is what shows it is load bearing. No source change; the binary is unchanged.

groeneai · 2026-08-05T02:56:56Z

Internal second-model review (2 rounds, 2 gate passes, 0 open findings)

Every change here was reviewed by a second model independently of the one that wrote
it, and the reviewer never edits code. Two rounds ran. Round 1 approved the source fix
and rejected the tests; round 2 approved both.

Round 1 — source accepted, tests rejected as not measuring the fix

⚠️ The three new test arms silently covered nothing in roughly one CI run in six.
query_plan_max_limit_for_top_k_optimization is randomized by the test runner over
[0, 1, 10, 100, 1000, 100000]; the value 1 makes the optimizer return early for a
LIMIT 5 query, so no __topKFilter was installed and the arms passed without
exercising the fix. Fixed by pinning the setting to 100 per query, as an existing
top_k test already does for the same reason, plus an EXPLAIN presence assertion per
arm so an arm can no longer pass while the optimizer declines to install the filter.
This was not theoretical: the first runner invocation of the following round happened to
draw the value 1.

⚠️ The NULL-ordering arm could not observe NULL ordering. Tuple element 0 was
distinct across all 3000 rows, and tuple comparison stops at the first differing
element, so the Nullable element was never compared and NULLS LAST / NULLS FIRST
were byte-identical by construction. Fixed by making element 0 repeat so element 1
decides the order; the two orderings now produce genuinely different rows.

💡 SYSTEM STOP MERGES added on the new tables: three inserts are only three parts
while nothing merges, and a single part never sets the threshold tracker, so a merge
could have made an arm vacuous while still passing.

❌ One reviewer finding was withdrawn with evidence rather than actioned (discarded
work on the fallback path: it happens per block, not per row, and only for arguments
that abort today).

Round 2 — jointly clean on code and tests

Both gate passes at the final commit returned 0 findings. The reviewer re-derived
every reference value by enumeration rather than accepting the recorded run, and
re-verified the source fix from scratch: the guard's predicate is the exact negation of
the condition that throws, and the fallback path forwards the NULLS FIRST/LAST hint
through tuple and nullable comparison, which the fast path did not.

One objection the reviewer raised against itself was refuted by measurement: an
EXPLAIN-presence assertion could in principle turn a legitimate optimizer decline into
a hard failure, but an existing test ships the identical assertion and has 0 failures in
14 days across roughly 70000 runs, including the distributed-plan, s3, old-analyzer,
tsan, msan and parallel-replicas configurations.

⚠️ Two inaccuracies in this PR's own text were found and corrected before publishing: a
liveness figure that still quoted the superseded first-round measurement (10 runs) while
the shipped tests were measured at 20, and one source line number off by four. Every
other citation in the description and the validation gate was verified line by line.

A jointly disclosed, deliberately unfixed residual is noted in the description: a
nested-NULL ordering question in the threshold tracker that could not be made to change
any result, so per the no-speculative-changes rule it is documented rather than patched.
A NULL-bearing arm now asserts the filter does not change the answer, which is what
would catch it if it ever does.

groeneai · 2026-08-05T02:58:20Z

Pre-PR validation gate (a-i)

#	Check	Answer
a	Deterministic repro?	Yes. `Tuple(UInt64, Nullable(UInt64))` sort column, MergeTree with 20 parts, default settings, no NULLs in the data: `SELECT k FROM t ORDER BY k ASC NULLS LAST LIMIT 3` aborts a debug build 10 times out of 10 with `Unexpected return type from __topKFilter. Expected UInt8. Got Nullable(UInt8).`, matching the CI message byte for byte. More than one part is required, since a single part never sets the threshold tracker.
b	Root cause explained?	Yes. `getReturnTypeImpl` (`FunctionTopKFilter.cpp:68`) declares `UInt8`; `executeVectorized` (`:107`) returns `elem_compare->getResultType()`. `FunctionComparison::getReturnTypeImpl` propagates `Nullable` out of tuple elements (`FunctionsComparison.h:1415`, `:1435`). The guard at `:85` tests only top level nullability, and `DataTypeTuple` does not override `IDataType::isNullable()` (`IDataType.h:310`), so the column takes the fast path and `ExpressionActions.cpp:694-699` throws.
c	Fix matches root cause?	Yes. It tests the resolved comparison type at the line that already computes it and delegates to the existing general path when it is not `UInt8`. No widened bound, no tag, no guard at the throw site.
d	Test intent preserved / tests added?	Yes. Both existing sections are untouched and their 10 reference rows are byte identical. Three arms added: `Tuple(UInt64, Nullable(UInt64))`, `Tuple(LowCardinality(Nullable(String)))`, and a NULL bearing arm asserting the filter does not change the answer for `NULLS LAST`, `NULLS FIRST` and `DESC`. Each new arm pins `query_plan_max_limit_for_top_k_optimization = 100` and `max_threads = 1`, and carries an `EXPLAIN` presence assertion, so it cannot pass while the optimizer silently declines to install the filter. Removing the pin flips that assertion from 1 to 0 on all three arms, which is what proves it is load bearing. The NULL arm's first tuple element repeats so the `Nullable` element decides the order: tuple comparison stops at the first differing element, so `NULLS LAST` and `NULLS FIRST` now produce genuinely different rows.
e	Both directions demonstrated?	Yes. Reverting only the guard and rebuilding: the whole test fails 20 out of 20 runs, and each new arm in isolation aborts 20 out of 20 with the signature `Expected UInt8. Got Nullable(UInt8)`. With the fix: 20 out of 20 match the reference with zero aborts and zero error signatures, 50 randomized runs pass, and every value the runner can randomize `query_plan_max_limit_for_top_k_optimization` to (0, 1, 10, 100, 1000, 100000) passes when forced explicitly. The arms measured were extracted from the shipped test file rather than written by hand. Build IDs were asserted before every verdict, and returned exactly to the reviewed value after restoring the guard.
f	General across code paths?	Yes. `__topKFilter` has exactly one construction site (`optimizeTopK.cpp:213`); `grep -rn "topKFilter\|TopKFilter" src/ tests/` finds no other producer. The sibling `use_skip_indexes_for_top_k` arm consumes the tracker via `isValueInsideThreshold`, not via this function, and has its own guard at `optimizeTopK.cpp:157-159`. Fixed in the function that misdeclares its type, not at the throw site.
g	General across inputs?	Yes, by construction: the guard keys on the resolved comparison type. Measured carriers now routed safely: `Tuple(UInt64, Nullable(UInt64))`, `Tuple(Tuple(Nullable))`, `Tuple(LowCardinality(Nullable(UInt64)))`, `Tuple(LowCardinality(Nullable(String)))`. Measured columns that keep the fast path because their comparison resolves to plain `UInt8`: `Tuple(UInt64, UInt64)`, `UInt64`, `Array(Nullable(UInt64))`, `Map(String, Nullable(UInt64))`. Bare `Nullable(T)` is already caught at `:85`; bare `LowCardinality(Nullable(T))` is unwrapped before `executeImpl`. A structural predicate was rejected because it would disable the optimization for the `Array` and `Map` cases. Correction (2026-08-05): an earlier version of this row said those two "work today". That is wrong, and I measured it wrong by using an abort-only oracle. They do not abort, but `GenericComparisonImpl` hardcodes the nulls-direction argument as `1` (`FunctionsComparison.h:643-664`), so under `ASC NULLS FIRST` a nested NULL row is dropped: `Array(Nullable(UInt64))` returns `[100]` with the filter on and `[NULL]` with it off, and `Map(String, Nullable(UInt64))` likewise. `ASC NULLS LAST` and both `DESC` directions agree, which is what shows the probe discriminates. That reproduces byte-identically on unpatched master, so it is pre-existing and untouched by this diff; a separate fix follows. The trade-off is therefore performance against a pre-existing correctness gap, not a free win.
h	Backward compatible?	Yes. No setting added or changed, so no `SettingsChangesHistory.cpp` entry; no serialization or format change; no experimental gate. The only behavior change is a thrown logical error becoming a correct answer.
i	Invariants and contracts preserved?	Yes. The change restores the function's own declared type postcondition; `executeGeneral` builds `ColumnUInt8` directly (`:143`). No lock, allocation, lifetime or error path is added, and the branch is taken before any state is mutated. `executeGeneral` handles the new carriers: `ColumnTuple` does not override `compareColumn`, so it devirtualizes to `ColumnTuple::compareAtImpl` (`ColumnTuple.cpp:674-688`) into `ColumnNullable::compareAtImpl` (`ColumnNullable.cpp:449-474`) with `null_direction_hint`, which also applies `nulls_direction` where the fast path did not. The NULL bearing test arm pins filter on equal to filter off so any divergence surfaces.

Suite runs: 46 of the 47 top_k / topk stateless tests pass. The one failure,
02984_topk_empty_merge, fails identically on a pristine base binary with
ALL_CONNECTION_TRIES_FAILED: it queries remote('127.0.0.{1,2}', ...) and a single local test
server binds only 127.0.0.1. A wider earlier run showed the same pattern for three other tests
that need a cluster on the default port. None of these reference topK filtering.

groeneai · 2026-08-05T02:59:19Z

cc @george-larionov @KochetovNicolai, could you review this? __topKFilter declares UInt8 but its vectorized path returned whatever the delegated comparison resolved to, and a sort column that contains a Nullable without being Nullable (for example Tuple(UInt64, Nullable(UInt64))) makes that Nullable(UInt8), so the fix tests the resolved type and falls back to the general path rather than keying on the argument's structure.

clickhouse-gh · 2026-08-05T04:29:13Z

Workflow [PR], commit [f2d8b9c]

Summary: ⏳

job_name	test_name	status	info
Stress test (amd_asan_ubsan)		FAIL
	Logical error: Can't determine table for parallel replicas (STID: 3634-4123)	FAIL	cidb
Stress test (arm_asan_ubsan)		FAIL
	Segmentation fault (STID: 3773-4073)	FAIL	cidb

AI Review

Summary

This PR fixes the Nullable(UInt8) type mismatch that made __topKFilter throw on tuple carriers such as Tuple(UInt64, Nullable(UInt64)), and the added stateless tests do exercise that path. The current head still leaves one existing correctness issue open, though: the new isUInt8 guard is not sufficient to identify composite carriers that are safe to keep on the vectorized path.

Findings

❌ Blockers

[src/Functions/FunctionTopKFilter.cpp:137] Already raised inline and still applicable on the current head: Array(Nullable(...)), Map(...Nullable...), and equivalent nested-null composite carriers still resolve to plain UInt8, so they bypass executeGeneral, but GenericComparisonImpl compares them with compareAt(..., 1) and therefore hardcodes NULLS LAST. Under ORDER BY ... ASC NULLS FIRST LIMIT, __topKFilter can still drop rows that the sorter would keep.
Suggested fix: route nested-null composite carriers to executeGeneral (ideally only for the broken direction == 1 && nulls_direction == -1 quadrant if the broader fallback is too expensive), or teach the generic comparison path to use the actual nulls_direction.

Final Verdict

Not ready to merge until the vectorized path stops evaluating nested-null composite carriers with a hardcoded NULLS LAST ordering.

clickhouse-gh · 2026-08-05T04:49:14Z

        auto elem_compare = compare_function->build(args);
+        /// getReturnTypeImpl always declares UInt8, so a comparison resolving to anything else
+        /// (a Nullable nested in a Tuple makes it Nullable(UInt8)) must not be returned here.
+        if (!isUInt8(elem_compare->getResultType()))


UInt8 is not a strong enough predicate for "safe to stay vectorized" here. For non-tuple composite columns the delegated comparison goes through FunctionComparison::executeGenericIdenticalTypes / GenericComparisonImpl, and that path hardcodes a.compareAt(..., 1) (src/Functions/FunctionsComparison.h:643-664, src/Functions/FunctionsComparison.h:1268-1305). So Array(Nullable(...)) / Map(...Nullable...) are still compared as nested NULLS LAST regardless of the query's actual nulls_direction, while executeGeneral and the sorter honor nulls_direction.

That leaves a wrong-result surface open on the exact carriers this guard deliberately keeps on the fast path. A minimal trace is Array(Nullable(UInt8)) with ORDER BY arr ASC NULLS FIRST LIMIT 1 SETTINGS use_top_k_dynamic_filtering = 1, use_top_k_dynamic_filtering_for_variable_length_types = 1: if one part publishes [1] as the threshold, a later [NULL] row is evaluated by lessOrEquals using compareAt(..., 1), so [NULL] <= [1] is false and the row is dropped even though the sort would rank [NULL] before [1]. I think we need to either thread nulls_direction into the generic comparison path or conservatively fall back to executeGeneral for nested-null composite carriers, not just for non-UInt8 result types.

Confirmed, and both citations check out. GenericComparisonImpl does hardcode the direction argument as 1 in all four members (FunctionsComparison.h:643-664), and SortDescription.h:51 documents 1 as "NULLs and NaNs are greater", so the generic path is permanently NULLS LAST.

I measured it rather than reasoning about it. Fixture is 3 parts with SYSTEM STOP MERGES and NULLs only in the last part, so the threshold is published from non-NULL parts first; max_threads = 1 and query_plan_max_limit_for_top_k_optimization = 100 pinned, and an EXPLAIN + LIKE '%__topKFilter%' row reads 1 in every arm so none of them is vacuous. Filter on versus use_top_k_dynamic_filtering = 0 on the same binary, 5 runs each:

carrier query filter on filter off

Array(Nullable(UInt64)) ASC NULLS FIRST LIMIT 1 [100] [NULL] row dropped

Array(Nullable(UInt64)) ASC NULLS LAST LIMIT 1 [100] [100] agree

Map(String, Nullable(UInt64)) ASC NULLS FIRST LIMIT 1 {'k':100} {'k':NULL} row dropped

Array(Nullable(UInt64)) DESC NULLS LAST LIMIT 1 [3099] [3099] agree

Array(Nullable(UInt64)) DESC NULLS FIRST LIMIT 1 [NULL] [NULL] agree

So the defect is narrower than stated: it needs direction == 1 && nulls_direction == -1. Both DESC directions are safe because greaterOrEquals against a NULL-as-greatest threshold keeps every NULL row, which is over-inclusive and cannot lose one. The NULLS LAST row agreeing is the control that shows the probe discriminates. A top-level Nullable(UInt64) column, which takes executeGeneral at :85, returns \N both ways, which localises this to the fast path rather than to the tracker or the sorter.

The important part of your comment is what it says about this PR's write-up, and you are right there too. The body claimed those two carriers "work today". That was measured with an abort-only oracle, which by construction cannot see a wrong result. I have corrected the body and row (g) of the validation gate comment: the trade-off for rejecting a structural predicate is performance against a pre-existing correctness gap, not a free win.

I am not fixing it here. The same fixture gives byte-identical output on this branch, on the pristine master snapshot and on my own base build, and this PR's guard cannot reach these carriers at all: their comparison resolves to plain UInt8, so isUInt8(...) is true and they keep the fast path exactly as before. The diff is unchanged and still only converts an abort into a correct answer for the Tuple carrier, where it also fixes nulls_direction as a side effect: Tuple(UInt64, Nullable(UInt64)) with the Nullable element deciding the order aborts on unpatched master and returns the correct (0,NULL) under ASC NULLS FIRST with the fix.

On your two remedies I agree with the second. Threading nulls_direction through GenericComparisonImpl changes a path every types_equal composite comparison reaches, and a bare [NULL] <= [1] returning 0 is defensible on its own; that is a much bigger semantic change than this needs. Falling back to executeGeneral is local and provably honors the hint. It should key on the general property rather than on isArray/isMap, and hasNullable (src/DataTypes/hasNullable.h:8) already sees through Array, Tuple, Map and LowCardinality(Nullable). Narrowing the fallback to the one broken direction pair is worth measuring, since routing all of them to the row-at-a-time path is a regression for the three directions that are correct today.

Separate PR, and I will link it here.

clickhouse-gh · 2026-08-05T06:05:13Z

Build profile diff (arm_release)

Comparing f2d8b9cee with master 9adeb11c7 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

Binary	Master	PR	Δ
`programs/clickhouse-stripped`	710.14 MiB	707.09 MiB	-3.05 MiB (-0.43%)

Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction.

Compile time of recompiled translation units

7 translation units recompiled, 7 s compile time in total, 7 of them have a recent master baseline.

Job report

groeneai · 2026-08-05T09:16:35Z

CI finish ledger - `72113bc`

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

CI is fully finished on this head: 174 check-runs, 0 queued or in progress, 156 success and
17 skipped, with Config Workflow and Finish Workflow both success. The Post Hooks
non-gating rows are 65 of 65 OK.

Check / test	Reason	Owner / fixing PR
`AST fuzzer (amd_debug, targeted, old_compatibility)` / `Invalid number of columns in chunk pushed to OutputPort. Expected 2, found 3` (STID 2270-3452)	a normal projection is used to read the input of a correlated subquery that was decorrelated into a join, and the rewritten read's header is wider than the declared one, so `JoiningTransform::prepare` pushes 3 columns into a 2-column port (`Port.h:431` via `JoiningTransform.cpp:101`). Not this PR: the diff is `FunctionTopKFilter.cpp` plus one stateless test, the failing query has no `topK` and reads `tab_proj` through `WHERE (SELECT __table1.v2)`, and the signature spans 18 other carriers plus one master run in 30 days	#111401 (mine, open)

#111401 is the causal fix rather than a coincidental same-area PR: it hoists the
materialize-constants and blocksHaveEqualStructure guard in
optimizeUseNormalProjection.cpp so it protects the all-parts-from-projection branch too,
and it names this STID and this JoiningTransform assert as its first carrier. It is open, so
there is no merged fix to pick up onto this branch.

Session id: cron:our-pr-ci-monitor:20260805-083000

…/fix-topkfilter-nested-nullable

groeneai added 2 commits August 4, 2026 23:44

groeneai mentioned this pull request Aug 5, 2026

URL database: do not probe the filesystem without the read source grant #113029

Merged

shankar-iyer self-assigned this Aug 5, 2026

shankar-iyer added the can be tested Allows running workflows for external contributors label Aug 5, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Aug 5, 2026

clickhouse-gh Bot reviewed Aug 5, 2026

View reviewed changes

Boulea7 mentioned this pull request Aug 10, 2026

Fix top-K dynamic filtering for empty Tuple columns #114182

Merged

groeneai mentioned this pull request Aug 14, 2026

Fix TopK filtering dropping NULL and NaN rows from the answer #114812

Open

groeneai added the groeneai-origin-request PR origin: a maintainer pinged or directed groeneai label Aug 19, 2026

pekashy mentioned this pull request Aug 20, 2026

use_top_k_dynamic_filtering puts __topKFilter in PREWHERE instead of the ones that predicate optimize_move_to_prewhere would move there #115573

Open

Merge commit '9adeb11c7ea53c34459e9799e3aa9eaca8948455' into groeneai…

f2d8b9c

…/fix-topkfilter-nested-nullable

groeneai mentioned this pull request Aug 29, 2026

__topKFilter dispatch misses Nullable inside Tuple: Nullable(UInt8) prewhere #117013

Open

Navigation Menu

Uh oh!

Fix logical error in topK dynamic filtering when the sort column contains a Nullable - #113406

Fix logical error in topK dynamic filtering when the sort column contains a Nullable#113406
groeneai wants to merge 3 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-topkfilter-nested-nullable

groeneai commented Aug 5, 2026 •

edited

Loading

Uh oh!

groeneai commented Aug 5, 2026

Round 1 — source accepted, tests rejected as not measuring the fix

Round 2 — jointly clean on code and tests

Uh oh!

groeneai commented Aug 5, 2026 •

edited

Loading

Uh oh!

groeneai commented Aug 5, 2026

Uh oh!

clickhouse-gh Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

clickhouse-gh Bot Aug 5, 2026

Uh oh!

groeneai Aug 5, 2026

Uh oh!

clickhouse-gh Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

groeneai commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

carrier	query	filter on	filter off
`Array(Nullable(UInt64))`	`ASC NULLS FIRST LIMIT 1`	`[100]`	`[NULL]`	row dropped
`Array(Nullable(UInt64))`	`ASC NULLS LAST LIMIT 1`	`[100]`	`[100]`	agree
`Map(String, Nullable(UInt64))`	`ASC NULLS FIRST LIMIT 1`	`{'k':100}`	`{'k':NULL}`	row dropped
`Array(Nullable(UInt64))`	`DESC NULLS LAST LIMIT 1`	`[3099]`	`[3099]`	agree
`Array(Nullable(UInt64))`	`DESC NULLS FIRST LIMIT 1`	`[NULL]`	`[NULL]`	agree

Uh oh!

Conversation

groeneai commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Description

Uh oh!

groeneai commented Aug 5, 2026

Round 1 — source accepted, tests rejected as not measuring the fix

Round 2 — jointly clean on code and tests

Uh oh!

groeneai commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

groeneai commented Aug 5, 2026

Uh oh!

clickhouse-gh Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Final Verdict

Uh oh!

clickhouse-gh Bot Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build profile diff (arm_release)

Uh oh!

groeneai commented Aug 5, 2026

CI finish ledger - 72113bc

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

groeneai commented Aug 5, 2026 •

edited

Loading

groeneai commented Aug 5, 2026 •

edited

Loading

clickhouse-gh Bot commented Aug 5, 2026 •

edited

Loading

clickhouse-gh Bot commented Aug 5, 2026 •

edited

Loading

CI finish ledger - `72113bc`