carnot: push the ClickHouse time window down in the timestamp column's own units - #119
Closed
ConstanzeTU wants to merge 1 commit into
Closed
ConstanzeTU wants to merge 1 commit into
ConstanzeTU wants to merge 1 commit into
Conversation
ConstanzeTU
force-pushed
the
fix/clickhouse-time-pushdown-units
branch
from
September 16, 2026 07:28
111d1c6 to
3243d5a
Compare
ConstanzeTU
force-pushed
the
fix/clickhouse-time-pushdown-units
branch
from
September 16, 2026 07:58
3243d5a to
1ee5a65
Compare
ConstanzeTU
changed the base branch from
feat/pixie-native-sbob
to
fix/ae-protocol-export-pxexport
September 16, 2026 07:58
The ClickHouse source node divided the start/end bounds from nanoseconds to seconds before rendering them into the WHERE clause. That is right for a DateTime/DateTime64 column, which ClickHouse compares against a plain integer as a unix-seconds timestamp, but wrong for an integer timestamp column holding unix-epoch NANOSECONDS, the convention several of our retention tables use. Against those, a seconds bound is smaller than every stored value, so the predicate was trivially true, no partition or primary-key range was pruned, and the table was scanned whole once per pagination page -- surfacing as "Failed to execute ClickHouse batch query: Timeout exceeded: elapsed 90.2s, maximum: 90". Resolve the timestamp column's type in ClickHouseSourceIR::ResolveType, before column pruning can drop the column from the projection, carry it in the plan proto, and let the exec node render the bound as nanoseconds for an INT64 column and as seconds for DateTime/DateTime64. An unresolved type keeps the DateTime reading. Also copy timestamp_column_ (and its type) in CopyFromNodeImpl, which dropped them and left copied source nodes with no pushdown at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017S6W3BHCmX5kCwGps9vevw
ConstanzeTU
force-pushed
the
fix/clickhouse-time-pushdown-units
branch
from
September 16, 2026 10:51
1ee5a65 to
1412500
Compare
Author
|
Closing — the connector already does time pushdown; the established mechanism is schema-side (dx_ord__* keep event_time as DateTime64 so the forwarded seconds filter windows the pull). No engine change needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A PxL script that reads a ClickHouse-backed table with
start_time=-30mfails:The time window was not being pushed down.
ClickHouseSourceNodedivided thestart/end bounds from nanoseconds to seconds before rendering them into the
WHEREclause. That is correct for aDateTime/DateTime64column, whichClickHouse compares against a plain integer as a unix-seconds timestamp — but
wrong for an integer timestamp column holding unix-epoch nanoseconds, a
convention several of our retention tables use. A seconds bound is below every
stored value, so the predicate was trivially true, no partition or primary-key
range was pruned, and the table was scanned whole, once per pagination page.
Measured against a 20M-row table with a
UInt64nanosecondevent_time, fromthe server's own
query_log: 7.4M rows read per page across ~2,500 pages, andthe predicate matched all 20,000,600 rows where the correct one matches 600.
Fix
Resolve the timestamp column's type in
ClickHouseSourceIR::ResolveType—before
PruneOutputColumnsTocan drop the column from the projection — carry itin the plan proto as
timestamp_column_type, and let the exec node render thebound in that column's own units:
INT64column → nanoseconds, as storedDateTime/DateTime64→ seconds, unchanged from todayAlso copies
timestamp_column_and its type inCopyFromNodeImpl, whichdropped them and left copied source nodes with no pushdown at all.
Verification
Same script, same cluster, same 20M-row seed, before and after:
A probe over the nanosecond-column table went from never returning (killed at
400s) to 6.8s with the correct rows. A join of a
DateTime64table against aUInt64-nanosecond table returns in 12.8s with each side pushed down in its ownunits — the
DateTime64side still receives the seconds bound and reads 50 rows.Three unit tests cover the three unit cases. Planner, plan and logical-planner
suites pass.
Rollout note
The planner runs inside the query broker, so it is the query broker that writes
timestamp_column_typeinto the plan. With an old query broker the field isunset and the exec node keeps the seconds bound: PEM, Kelvin and query broker
must roll together or the fix is inert.