Fix the _headers virtual column's position, its cached-count value and a Hive key of the same name - #119878
Open
groeneai wants to merge 12 commits into
Open
Fix the _headers virtual column's position, its cached-count value and a Hive key of the same name#119878groeneai wants to merge 12 commits into
_headers virtual column's position, its cached-count value and a Hive key of the same name#119878groeneai wants to merge 12 commits into
Conversation
`prepareReadingFromFormat()` builds `ReadFromFormatInfo::source_header` with the
requested virtual columns in the order the query referenced them, and
`updateFormatPrewhereInfo()` rebuilds it by the same rule. The two sources that
expose `_headers` did not follow that order: `StorageURLSource` erased `_headers`
from its copy of the list and appended it to the chunk unconditionally last,
and `StorageObjectStorageSource` did the same after the shared virtuals loop
(which has no `_headers` branch, so it contributed nothing for that name).
Whenever `_headers` was not the last requested virtual, the chunk's virtual tail
was therefore a rotation of the header's tail. The column count still matched, so
the structural guard in `OutputPort::pushData` never fired and the mismatch
travelled downstream: a later expression read a column of one type through the
declared type of another. In a debug or sanitizer build that aborts on a failed
`assert_cast`; in a release build `assert_cast` degrades to `static_cast`, so the
column is reinterpreted and the behaviour is undefined.
Make the shared ordered loop the single writer of every requested virtual: give
`addRequestedFileLikeStorageVirtualsToChunk()` a `_headers` branch, pass the
materialized map through `VirtualsForFileLikeStorage`, and drop both out-of-band
appends and the `eraseNames({"_headers"})`. The order invariant then holds by
construction instead of by two sites agreeing on a special case.
The response-header fetch in `StorageURLSource::generate()` moves above the loop
call. It stays behind `need_headers_virtual_column` and keeps its
`http_response_headers_initialized` once-per-file semantics, so a query that does
not request `_headers` still performs no extra work.
`_headers` has existed since e4aceed (2024-08-25), so every release since
24.9 is affected.
`addRequestedFileLikeStorageVirtualsToChunk()` is now the single writer of every
requested virtual, but only the URL arm had a test pinning that order.
`tests/integration/test_storage_url/test.py` already reaches the `Web`
object-storage arm three times, through
`allow_experimental_url_wildcard_from_index_pages=1` with
`url('http://resolver:8087/data/**/part*.tsv')`, which
`TableFunctionURL.cpp:359-388` routes to `StorageObjectStorage` with
`StorageWebConfiguration`. Each of those cases requests `_headers` alone or
after `_file`, which is the order the previous append-last behaviour already
satisfied, so none of them constrains the position of `_headers`.
Add the missing ordered case on the fixture already running in that module:
`_headers` before `_time`, plus the reverse order as a control. The predicate
reads the map (`mapKeys`), because the failure surfaces on the map side and a
predicate that never touches the map cannot fail. `_time` is the second virtual
because the module already pins it NULL on this fixture, the mock index server
sending no `Last-Modified`.
Measured with `-k url_wildcard` on the same pair of binaries: 53 passed with the
fix. Without it, the module is 52 passed when the new case is excluded and
43 failed / 10 passed when it is included, the new case aborting the server with
the mirror of the originally reported signature, `Bad cast from type
DB::ColumnNullable to DB::ColumnMap` raised from `mapKeys`. That comparison
isolates the new case as the only test which distinguishes the two binaries.
On the cached-row-count branch `StorageURLSource::initialize` builds a `ConstChunkGenerator` and the data `GET` is never performed. That request's response is the only source of `_headers`: `ReadWriteBufferFromHTTP::initialize()` is the sole writer of `response_headers`, and it runs from the constructor only when `!delay_initialization`, which `StorageURLSource` sets to false for a single URL. The `HEAD` behind `tryGetLastModificationTime()` parses a local `Poco::Net::HTTPResponse` into `file_info` and never touches `response_headers`. So `SELECT _headers FROM url(<single url>)` returned an empty map, silently, as soon as the row count for that URI was cached. All three preconditions are ordinary: `need_only_count` is true whenever no data column is referenced, which is exactly the shape of a query selecting only virtual columns; `addNumRowsToCache` warms the entry on any prior full read of the same URI and format; and the cache accepts an entry with no modification time once `schema_inference_cache_require_modification_time_for_url` is 0. Decline the shortcut when `_headers` is requested. `StorageObjectStorageSource` already does exactly this, so this restores the symmetry between the two sources that expose the column. Forcing the buffer open before the cache check was rejected: it would defeat the optimization for every query and change the request pattern. `addNumRowsToCache` is untouched, so writing counts stays correct and every other query still reads them. `_time` and `_size` need no equivalent guard, because both are filled from the `HEAD` before the cache check; only `_headers` depends on the `GET`. The appended `05210` statement pair warms the count cache and then reads `_headers` through it, with the three relevant settings pinned per statement so randomization cannot silence it. The cache hit itself was measured rather than assumed: on a private probe URI, a second `SELECT count()` records `ProfileEvents['SchemaInferenceCacheNumRowsHits'] = 1` while the `_headers` statement records 0, so the two differ only in whether the column is requested. With the guard reverted the new statement returns 0 where 1 is expected, and the three pre-existing reference lines are unchanged, which isolates it as the only discriminator.
The `_headers` branch this PR added to
`addRequestedFileLikeStorageVirtualsToChunk` matched on the name alone, so it
also intercepted a Hive partition key of that name and filled the column with a
default instead of the path value. `SELECT _headers FROM
file('d/_headers=abc/data.tsv', TSV, 'x UInt64')` returned an empty string
where it used to return `abc`, on default settings.
A Hive key becomes a reader-place virtual under its own name, verbatim:
`extractHivePartitionColumnsFromPath` takes the key as it appears in the path
with no prefixing, `add_virtual` registers it unless a physical column already
has that name, and `prepareReadingFromFormat` then finds it and lets the shared
loop materialize it. `use_hive_partitioning` is on by default.
`_headers` is the only branch name in that loop exposed to this, because it is
the only one registered per storage rather than by
`getCommonVirtualsForFileLikeStorage()`. A Hive key equal to any of the other
twelve names hits `VirtualColumnsDescription::add`'s `DUPLICATE_COLUMN` throw
and fails loudly at storage creation, and `url`/`URL`/`urlCluster`/`Web` are
protected by the same throw when they register the `Map` on top. That leaves
`file()`, `s3()`, `azureBlobStorage()`, `hdfs()` and `ObjectStorageQueue`,
which never register the `Map`.
Gate the branch on the supplied map, not on the name alone, so any other
source's `_headers` falls through to the Hive branch as before. Gating on the
column's type instead would still shadow the Hive column whenever the types
happened to agree, and moving the branch below the Hive lookup would feed a
`String` Hive value into a `Map(LowCardinality(String), LowCardinality(String))`
column for a glob whose sample path lacks the key.
The now-unreachable default arm is deleted rather than kept as a fallback. A
source that requested `_headers` without supplying it would leave the column
absent, the chunk's column count would stop matching the header, and
`OutputPort::pushData` would throw `Invalid number of columns in chunk pushed to
OutputPort`. That is loud, which is the opposite of the silent tail rotation
this PR fixes, and is the correct failure mode.
`05211` pins the restored behaviour on both the value and the type: the type
alone does not discriminate, because the shadowing filled a default of the Hive
column's own type. Before this commit the test returns an empty string for
`abc`; after it, both lines match.
The warm-then-read pair asserted `1` on both statements, so the read passed both when the guard correctly declined a live cache entry and when nothing had ever been cached. If warming silently stopped working the pair would go quiet instead of failing. Assert the entry exists between the two statements. `system.schema_inference_cache` is the instrument rather than the `SchemaInferenceCacheNumRowsHits` profile event: there is no stateless precedent for that event, reading it needs `SYSTEM FLUSH LOGS` plus a `log_comment` round-trip through `system.query_log`, and asserting a hit would make the test depend on the entry surviving eviction, which cannot be pinned from a query because `schema_inference_cache_max_elements_for_url` is a server setting. The entry's existence is the precondition the pair actually needs, and it is observable directly. `max()` is load-bearing. The flaky check runs the file about fifty times against one server with different randomized format settings, and `getKeyForSchemaCache` folds format settings into the key, so the table accumulates one row per settings combination for the same source; an unaggregated select would return several rows on the second run. It also returns `\N` when there is no entry at all, which is exactly the vacuity being closed: with the warm-up statement removed the new line reports `\N` and the test fails. With the cache guard reverted the new line still reports `1` while the `_headers` read flips to `0`, so the two statements fail for different reasons and neither masks the other. The `source` filter keys on a URI unique to this file, so the statement stays parallel-safe.
… type `StorageObjectStorageSource` decides to read the response headers from the requested column NAME and then falls back to the object's attributes (`tryGetHeadersFromReadBuffer(reader.readBuffer()).value_or(objectAttributesToMap(...))`), so on every object storage `VirtualsForFileLikeStorage::headers` is non-null as soon as `_headers` is requested. Gating the shared loop's branch on that pointer alone therefore only covers `file()`: on `s3()`, `azureBlobStorage()` and `hdfs()` the branch still claimed a Hive path key named `_headers` and fed it a `Map` `Field` through the key's own type, which throws `Bad get: has Map, requested String`. Only `Web` object storage and the `url` / `URL` / `urlCluster` sources register `_headers` as a `Map`, so the registered type is the discriminator, and putting it on the virtual column covers all four callers and any future one instead of requiring each caller to opt out. A Hive key of that name carries a type inferred from the path value (`LowCardinality(String)` for `abc`, `Int64` for `42`, `Date` for `2020-01-01`), never a `Map`, so it reaches the Hive branch as intended; all three of those keys were `Bad get` failures before this commit. The pointer check is kept alongside the type check: a source that registers the `Map` and forgets to supply it must fail loudly rather than dereference an empty optional. The new test needs Minio and therefore carries `no-fasttest`, which is why it is a separate file from the `file()` case in 05211 rather than another statement there: 05211 stays tag-free and keeps running in the Fast test job.
`SELECT max(number_of_rows) FROM system.schema_inference_cache` shows the entry was populated, but it reads the cache through `StorageSystemSchemaInferenceCache`, while the lookup the guard declines is `SchemaCache::tryGetNumRows`. Nothing in the test observed that lookup, so if it silently stopped hitting, the `_headers` read would still return `1` through an ordinary `GET` and the test would keep passing while protecting nothing. Move the case into its own `.sh` test and read the lookup itself: `ProfileEvents['SchemaInferenceCacheNumRowsHits']` is incremented only in `SchemaCache::tryGetNumRows`, and `system.query_log` exposes it per `query_id`. A plain `count()` on the URI must report `1` (the cached row count is servable) while the `_headers` read reports `0` (the guard declined it), so an empty cache cannot pass the test: with the warm-up statement removed the control itself drops to `0`. With the guard reverted the control stays `1` while the guarded read reports `1` and the map goes empty, so the two assertions fail for different reasons and neither masks the other. This replaces the reasoning recorded in the previous commit, which is wrong on its main point: the per-`query_id` idiom has stateless precedent (`02263_lazy_mark_load.sh` reads `ProfileEvents['FileOpen']` the same way), and it cannot be forged by another test's queries because the id is derived from `$CLICKHOUSE_TEST_UNIQUE_NAME`. The eviction concern is real but bounded: the three statements run back to back in one invocation with one settings string, hence one schema-cache key, and 50 randomized runs showed the control at `1` and the guarded read at `0` every time. 05210 goes back to pinning only the requested virtual-column order.
`StorageObjectStorageSource::generate()` materialized the `_headers` map from the
requested-column NAME alone, but only `Web` object storage registers `_headers`
as the HTTP response-header `Map`: `StorageObjectStorage.cpp` guards that
registration with `getType() == ObjectStorageType::Web`. On `S3`, `Azure`, `HDFS`
and `Local` a requested `_headers` can only be a Hive path key, whose type is
inferred from the path VALUE.
The previous commit's premise, that the registered type tells the two apart, is
wrong. Hive inference runs the full single-field inference
(`HivePartitioningUtils.cpp` -> `tryInferDataTypeByEscapingRule` with
`EscapingRule::Raw` -> `tryInferDataTypeForSingleField`), which maps a value
starting with `{` to `DataTypeMap`: measured through `file()` on an out-of-band
directory `_headers={1:2}`, `toTypeName(_headers)` is `Map(Int64, Int64)`. So for
an object key `.../_headers={1:2}/data.tsv` read through a wildcard on `s3`,
`isMap` holds, the response-header branch wins over the Hive branch, and the
column is filled from `objectAttributesToMap`, which is empty on a plain S3
object: the query returns `{}` instead of `{1:2}`.
Gate the materialization on the object storage type, so the arm that registers
the virtual is the arm that supplies it. As a side effect the attribute map is no
longer built per chunk for a value discarded everywhere but `Web`.
The `isMap` check stays, guarding a different case: on the `Web` arm and on
`url` / `URL` / `urlCluster`, a path that also carries a Hive segment named
`_headers` registers that name from the Hive branch first, and if the resolved
type is the Hive-inferred one, `isMap` keeps a `Map` field out of a
`LowCardinality(String)` column.
No test accompanies this. `containsGlobs` treats any path holding `{` as a glob,
so an `INSERT INTO FUNCTION s3(...)` on such a key is refused ("contains globs,
the table is in readonly mode") and a `SELECT` cannot address one literally
either; the reachable form needs an object created out of band. The `Web` arm
this must not break is covered by
`test_storage_url/test.py::test_url_wildcard_headers_virtual_column` and
`::test_url_wildcard_headers_virtual_column_order`, which both fail with
`Invalid number of columns in chunk pushed to OutputPort` when the new condition
is inverted.
The comment claimed that a Hive path key named `_headers` "gets a type inferred
from the path value instead" of a `Map`, i.e. that the registered type tells the
response-header virtual and a Hive key apart. That is refuted: Hive inference runs
the full single-field inference, so a key `_headers={1:2}` registers as
`Map(Int64, Int64)` and `isMap` holds for the Hive virtual too. The discriminator
is the supplier. `virtual_values.headers` is filled only by `url` / `URL` /
`urlCluster` and by `Web` object storage, which are exactly the storages that
register `_headers` as the response-header `Map`.
The `isMap` conjunct stays, but not for the reason the previous commit gave. That
message justified it with a `url` or `Web` path that also carries a `_headers=`
Hive segment, and that case cannot occur.
`VirtualColumnUtils::getVirtualsForFileLikeStorage` registers Hive keys as
ephemeral reader virtuals first, and its `add_virtual` skips a key only when a
physical column already has the name, so the storage's own `_headers` `Map`
registration (`StorageURL.cpp:254`, `StorageObjectStorage.cpp:369`) then reaches
`VirtualColumnsDescription::add` and throws `DUPLICATE_COLUMN`: the table fails at
creation. The conjunct is kept as cheap defence for a future caller that registers
the map and forgets to supply it.
Nothing in the tree failed if the `ObjectStorageType::Web` conjunct added by the
previous commit were deleted. `05213` reads a Hive key `_headers=abc`, which infers
as `LowCardinality(String)`, so `isMap` already blocks interception there with or
without the gate, and the `Web` cases in `test_storage_url` legitimately register
`_headers` as a `Map` and pass either way. The uncovered case is a Hive key whose
value infers AS a `Map`, where `isMap` is true and only the storage type keeps the
path value from being replaced by the empty attribute map of a plain S3 object.
Such a key holds `{`, which `containsGlobs` treats as a glob, so no SQL statement
can create it and the object has to be created through the storage client. That
makes it an integration test rather than a stateless one: `put_s3_file_content`
calls `minio_client.put_object` with an arbitrary key and never goes through SQL.
The read addresses the object through a wildcard in place of the Hive value, so the
SQL text carries no `{`, and asserts the registered type together with the value,
`Map(Int64, Int64)` and `{1:2}`. Asserting the type is what makes the oracle
specific, because it is the statement that `isMap` is true here and therefore that
the storage-type gate, not the type test, is what protects the value.
Measured with the gate dropped, leaving the name-only test: the new test fails on
the value, `{}` where `{1:2}` is expected, with the type still `Map(Int64, Int64)`.
`05212_url_headers_no_cached_count` failed 8 times in 150 randomized runs, always on the same line: the `hit` control arm reported `SchemaInferenceCacheNumRowsHits = 0`. The runner randomizes `optimize_trivial_count_query`, and `StorageURL.cpp` computes `need_only_count` as `(query_info.optimize_trivial_count || no columns requested) && optimize_count_from_files`, so at `optimize_trivial_count_query = 0` a `count()` never reaches `tryGetNumRowsFromCache` at all. The control arm then reads as a declined lookup for a reason that has nothing to do with the guard under test, and the run fails. The test already pins the three other settings the shortcut needs; this is the fourth. Measured: with the setting injected at 0 the test failed 5/5 and at 1 passed 5/5 before the pin, and passes 5/5 under the same injection after it; 100/100 under full randomization, against 6/100 failures before. With the count-cache guard reverted it still fails 5/5, on both the `mapContains` line and the guarded arm's hit count, so the pin does not weaken the oracle.
groeneai
added
can be tested
Collaborator
Author
Internal second-model review: adjudication log (click to expand)Pre-publication review by an independent model (engine: codex; 11 findings over 7 rounds).
Severity: ❌ blocker / Noted, not blocking: A Hive partition key named Session id: cron:clickhouse-review-slot-8:20260914-011200 |
Collaborator
Author
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-4:20260914-001600 |
Contributor
|
Workflow [PR], commit [c60aa79] Summary: ⏳
AI ReviewSummaryThis PR moves Final VerdictStatus: ✅ Approve |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| } | ||
|
|
||
| chassert(dynamic_cast<ReadWriteBufferFromHTTP *>(read_buf.get())); | ||
| if (need_headers_virtual_column && !http_response_headers_initialized) |
Contributor
There was a problem hiding this comment.
This still leaves the common single-URL One path returning the wrong _headers. OneInputFormat::read synthesizes a row without touching its ReadBuffer (src/Processors/Formats/Impl/OneFormat.cpp:26), and getFirstAvailableURIAndReadBuffer(..., delay_initialization = true) keeps ReadWriteBufferFromHTTP unopened when there is only one URL candidate (src/Storages/StorageURL.cpp:441). On that path getResponseHeaders() here just serializes the still-empty response_headers map, so SELECT mapContains(_headers, 'X-ClickHouse-Query-Id') FROM url(..., One, 'dummy UInt8') still returns 0.
The cached-count guard fixes one shortcut, but One is another shortcut that also skips initializing the real GET response. Please either force the HTTP buffer to initialize before reading _headers, or disable the One fast path when _headers is requested, and add a focused regression for url / URL / urlCluster with format One.
Collaborator
Author
There was a problem hiding this comment.
The mechanism is right, the consequence is pre-existing, so I am declining this on scope.
Checked the mechanism first: OneInputFormat::read synthesizes its row without touching the buffer (src/Processors/Formats/Impl/OneFormat.cpp:26-35), and response_headers is filled only in ReadWriteBufferFromHTTP::initialize() (src/IO/ReadWriteBufferFromHTTP.cpp:453), which is reached only from nextImpl() (:477). Your query does return 0.
It returns 0 without this PR too. Same query on two debug servers, this branch against a master commit that is an ancestor of its base:
| probe | this branch (Build ID 08305bdd...) |
master 195780d64 (Build ID cbab6a4b...) |
|---|---|---|
One, mapContains(_headers, 'X-ClickHouse-Query-Id') |
0 | 0 |
LineAsString, same expression |
1 | 1 |
| cached row count, same expression | 1 | 0 |
Row 3 is the sensitivity control: the harness does see what this PR changes. Row 1 does not move, because the value expression is master's getResponseHeaders() call, only hoisted above the shared virtuals loop. This PR changes where _headers is written, not what it holds.
The One query issues exactly one request, a HEAD for the file size, and no data GET (server access log), which is what the format is for (OneFormat.cpp:57). Forcing initialization would turn SELECT _headers FROM url(..., One) into a full download. One is only ever chosen explicitly by the user, whereas use_cache_for_count_from_files is an internal optimization nobody asked for, which is why the guard for that one belongs in this PR. Changing a user-selected format's contract is a maintainer's call, not something I will fold into a crash fix.
|
|
||
| std::optional<Map> headers; | ||
| if (configuration->getType() == ObjectStorageType::Web | ||
| && read_from_format_info.requested_virtual_columns.contains("_headers")) |
Contributor
There was a problem hiding this comment.
This is still wrong for the Web object-storage One path. In createReader() we replace the underlying reader with EmptyReadBuffer whenever the format is One (src/Storages/ObjectStorage/StorageObjectStorageSource.cpp:1266), so tryGetHeadersFromReadBuffer(reader.readBuffer()) can never observe the data GET response here. On Web, _headers therefore falls back to object_metadata->attributes (or {}) instead of the actual response headers.
That means the same invariant you fixed for cached row counts is still violated for queries like url('http://resolver:8087/data/**/part*.tsv', One, 'dummy UInt8') that dispatch through StorageObjectStorageSource. Please skip the EmptyReadBuffer optimization when Web + _headers is requested and add a regression for the wildcard/Web One shape.
Collaborator
Author
There was a problem hiding this comment.
Measured this one, and the observable claim does not hold: on the Web arm _headers under One is neither empty nor stale.
That arm is reachable only through the experimental index-page wildcard expansion of url / ENGINE = URL (src/TableFunctions/TableFunctionURL.cpp:359, src/Storages/StorageURL.cpp:2632), so I served a local HTTP directory tree and ran it with allow_experimental_url_wildcard_from_index_pages = 1:
| format | this branch | master 195780d64 |
|---|---|---|
TSV (buffer opened) |
5 headers | 5 headers |
One (EmptyReadBuffer) |
the same 5 headers | the same 5 headers |
tryGetHeadersFromReadBuffer does return nullopt there, as you say, but the objectAttributesToMap(object_metadata->attributes) fallback is populated, and it is live rather than cached: _headers['Date'] was 02:38:44 for a run at 02:38:44 and 02:38:52 for a run at 02:38:52, and its content matched the data GET headers in every probe.
The two columns are identical, so this PR does not move this path either: the expression is master's, hoisted above the shared loop so that loop stays the single writer of every requested virtual. Declining the change and the regression test with it: dropping the EmptyReadBuffer swap would make One read the object it exists in order not to read, and there is no wrong value here to pay for that.
clickhouse-gh
Bot
added
the
comp-table-functions
Contributor
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Compile time of recompiled translation units77 translation units recompiled, 739 s compile time in total, 77 of them have a recent master baseline. |
The `hit` statement is a control: it shows the schema-inference row count IS servable for the URI, so the guarded query's declined lookup is a decision and not an empty cache. It reads `ProfileEvents['SchemaInferenceCacheNumRowsHits']` from the `query_log` row of its own `query_id`. Parallel replicas over a multi-replica cluster rewrite `url(...)` into `urlCluster(<cluster>, ...)`, so the plan is `ReadFromCluster` instead of `ReadFromURL`: the initiator builds no `StorageURLSource` and never reaches `tryGetNumRowsFromCache`. The lookup runs on the replica that gets the file and its counter lands in that replica's own `query_log` row under a generated `query_id`, so the control reads 0. That is what failed in `Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel)`, 3 out of 3 reruns, on the control line alone. The guard under test holds there: the `_headers` query still returns 1 and still declines the lookup, on the initiator and on all three replicas. The pin joins the three settings the shortcut already needs, so all three statements stay on the local read path and the control keeps measuring the cache rather than the plan shape. Measured: with parallel replicas injected the test failed 3/3 before the pin and passes 5/5 after it, at `parallel_replicas_local_plan` 0 and 1; setting the pin to 1 brings the same failure back 3/3. 50/50 under full randomization, where the runner's `automatic_parallel_replicas_mode = 2` bundle set `enable_parallel_replicas = 1` for 63 of 333 statements and the pin overrode every one. With the count-cache guard reverted the pinned test still fails 3/3, on the `mapContains` line and on the guarded arm's hit count, with and without the injection, so the pin neither weakens the oracle nor makes the test vacuous in the flavour that reported it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed reading the
_headersvirtual column of aurl/URL/urlCluster/Webobject-storage table together with another virtual column the query references after it, e.g.SELECT _headers['k'], _time FROM url(...):_headerswas appended to the chunk last regardless of the requested order, so one column's data was read through another column's declared type: undefined behaviour in release builds, aLOGICAL_ERRORin debug and sanitizer builds. Also fixed_headersbeing an empty map for aurltable whose row count was already cached, and aLOGICAL_ERRORwhen a Hive partition key in the path of ans3/azureBlobStorage/hdfstable is named_headers.Description
ReadFromFormatInfo::source_headerlists the requested virtual columns in query order. Neithersource exposing
_headersfollowed it: both appended it to the chunk unconditionally last, outsidethe shared virtuals loop, which had no
_headersbranch.Whenever
_headerswas not the last requested virtual, the chunk's virtual tail was a rotation ofthe header's. The column count still matched, so no structural check fired.
Fix: make the shared ordered loop the single writer of every requested virtual. It gains a
_headersbranch fed throughVirtualsForFileLikeStorage, and both out-of-band appends and theeraseNames({"_headers"})are gone, so the order invariant holds by construction.That object-storage append also fired for a Hive path key named
_headers, which the loop hadalready produced from the path, so on
s3/azureBlobStorage/hdfsthe chunk carried onecolumn too many and the query raised
Invalid number of columns in chunk pushed to OutputPort.The header map is now supplied only where a storage registers it, so such a key resolves to the
path value.
A cached row count skips the data
GET, the only source of the header map, soSELECT _headers FROM url(...)returned{}once that URI's count was cached. The lookup is nowdeclined when
_headersis requested, mirroring the object-storage arm's existing guard.Each new test fails before its own fix and passes after, 50/50 with randomized settings, with a
control for the case that already worked. Also verified across virtual-column positions, the
URLengine, the PREWHERE header rebuild, lazy materialization and the
Webarm.Found by the AST fuzzer, no matching issue exists:
CI report.
All three are pre-existing on
master; the fuzzer seed only made the first reachable.Workflow [PR]
Sync PR [sync-upstream/pr/119878]