Repository navigation
Conversation
…oups. Separate GETs for each filter dominate cold reads from object storage. `input_format_parquet_bloom_filter_coalesce_max_bytes` merges contiguous filters, and reads a small file once when that is cheaper. Co-authored-by: Cursor <cursoragent@cursor.com>
… buffer. The bloom-filter header that starts the read is reset as soon as it is parsed, so charging that handle let the scheduler treat a live buffer as free. Related: #2526 Co-authored-by: Cursor <cursoragent@cursor.com>
That field is optional. Filters that only store an offset were left out of coalescing, even though their length is already derived later. Related: #2526 Co-authored-by: Cursor <cursoragent@cursor.com>
…or a whole-file read. Co-authored-by: Cursor <cursoragent@cursor.com>
…compatibility` values restore separate reads. Co-authored-by: Cursor <cursoragent@cursor.com>
…is below `bytes_per_read_task`. Co-authored-by: Cursor <cursoragent@cursor.com>
…and the filters after it stay inside `input_format_parquet_bloom_filter_coalesce_max_bytes`. Co-authored-by: Cursor <cursoragent@cursor.com>
….6 so older `compatibility` values restore separate reads, and count only the bloom ranges `Reader` registers. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 356269bf74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | `input_format_parquet_filter_push_down` | When reading Parquet files, skip whole row groups based on the WHERE/PREWHERE expressions and min/max statistics in the Parquet metadata. | `1` | | ||
| | `input_format_parquet_bloom_filter_push_down` | When reading Parquet files, skip whole row groups based on the WHERE expressions and bloom filter in the Parquet metadata. | `0` | | ||
| | `input_format_parquet_dictionary_filter_push_down` | When reading Parquet files (with reader v3), skip whole row groups based on the WHERE/PREWHERE expressions and the dictionary page contents, for equality and `IN` conditions, when all data pages of a column chunk are dictionary-encoded. The value is the maximum dictionary page size (in bytes) for which this optimization is applied; set to `0` to disable. Takes precedence over the bloom filter when both are available. | `1048576` | | ||
| | `input_format_parquet_bloom_filter_coalesce_max_bytes` | Max bytes of one read when loading Parquet bloom filters. Contiguous filters are read together. A file not larger than this, with many filters, is read once. 0 keeps one read per bloom filter. | `67108864` | |
There was a problem hiding this comment.
This line is inside the page's AUTOGENERATED_START/AUTOGENERATED_END region, so editing it in this feature commit will fail the documentation check unless the PR is the dedicated autogeneration change. Keep the corresponding source-registration update in ParquetV3BlockInputFormat.cpp and let the documentation workflow regenerate this file.
AGENTS.md reference: docs/reference/AGENTS.md:L1-L3
Useful? React with 👍 / 👎.
| const size_t prospective_start = std::min(start_offset, r.start); | ||
| const size_t prospective_end = std::max(end_offset, r.end); | ||
| const bool end_past_cap = same_group && prospective_end - prospective_start > span_limit; | ||
| if ((initial_req->coalesce_group != 0 && !same_group) || |
There was a problem hiding this comment.
When an ungrouped request starts first, this condition does not stop it from absorbing an adjacent grouped bloom-filter header because it checks only whether initial_req belongs to a group. This is reachable when a row group without an applicable bloom filter advances to index or data prefetching while another row group's bloom-header work remains queued; the bloom read then follows ordinary bytes_per_read_task rules and can exceed input_format_parquet_bloom_filter_coalesce_max_bytes (especially when configured below 4 MiB). Treat a group mismatch as a boundary whenever either request has a nonzero group, symmetrically in both scan directions.
Useful? React with 👍 / 👎.
…_bloom_filter_coalesce_max_bytes` row stays in the `Documentation` source. Co-authored-by: Cursor <cursoragent@cursor.com>
…ichever range starts the coalescing scan. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24138136ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bool group_boundary = initial_req->coalesce_group != r.request->coalesce_group | ||
| && (initial_req->coalesce_group != 0 || r.request->coalesce_group != 0); | ||
| if (group_boundary || | ||
| r.end + gap_limit <= start_offset || // short gap |
There was a problem hiding this comment.
When remote_read_min_bytes_for_seek=0 (or the corresponding local-file setting is zero), gap_limit is zero and this <= check stops at two exactly adjacent ranges because r.end == start_offset; the symmetric right-hand scan has the same behavior. ClickHouse writes bloom filters back-to-back in flushBloomFilters, so this valid configuration still issues one request per filter despite the new coalescing setting. Treat zero-byte gaps as contiguous independently of the ordinary seek threshold.
Useful? React with 👍 / 👎.
…or_seek` is 0. Co-authored-by: Cursor <cursoragent@cursor.com>
…ip a filter for a row group that produces no rows. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…er row group. https: //altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2588&sha=6e16e1d87e4b679d422fd7bd4b4f35df4e0bd4be&name_0=PR&name_1=Stateless%20tests%20%28amd_asan_ubsan%2C%20distributed%20plan%2C%20parallel%2C%20selected%20tests%29 https: //github.com//pull/2588 Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac5724ff5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// `end_past_cap` is the same-group cap. This addition wraps when the cap is `UInt64` max. | ||
| (!same_group && r.start + span_limit <= initial_offset) || // task not too big | ||
| end_past_cap || | ||
| (!same_group && !r.request->allow_incidental_read.load(std::memory_order_relaxed))) // range wants to be coalesced |
There was a problem hiding this comment.
When a column chunk is eligible for dictionary filtering and also has a bloom filter, starting its header now absorbs the overlapping full bloom-filter body because same-group ranges bypass allow_incidental_read. This defeats the contract in Reader::initializePrefetches that only the small header is read unless dictionary pruning cannot be used: even when the exact dictionary filter succeeds, the query has already downloaded and pinned potentially every bloom body, causing substantial unnecessary object-storage traffic across many files. Keep fallback bloom bodies out of header-triggered coalescing until the fallback is actually selected.
Useful? React with 👍 / 👎.
…r read. The header and the body were in one coalesce group, so the header read downloaded the body before the dictionary filter was tried. Co-authored-by: Cursor <cursoragent@cursor.com>
…iffers from `remote_read_min_bytes_for_seek`. The seek threshold can merge small adjacent filters, but it also applies to other Parquet ranges and has no bloom-only cap. Co-authored-by: Cursor <cursoragent@cursor.com>
ianton-ru
changed the title
Defects in #2588Head MediumA plain column's bloom read still downloads the previous column's dictionary-fallback bloom body. The fallback body is registered in coalesce group 0 so its own header will not pull it in. The header stays in group 1. Filters are written back-to-back, and the header is only the first few hundred bytes of the body. When the next column is not dictionary-encoded, the left scan starts at that column's header, includes its own body, then steps onto the previous column's short header because the gap (
const uint8_t bloom_data_group = column.use_dictionary_filter ? uint8_t{0} : bloom_group;
column.bloom_filter_data_prefetch = prefetcher.registerRange(
size_t(column.meta->meta_data.bloom_filter_offset),
len, /*likely_to_be_used=*/ false, bloom_data_group);
}
column.use_bloom_filter = true;
column.bloom_filter_header_prefetch = prefetcher.registerRange(
size_t(column.meta->meta_data.bloom_filter_offset),
max_header_length, /*likely_to_be_used=*/ true, bloom_group);
const bool group_boundary = initial_req->coalesce_group != r.request->coalesce_group
&& (initial_req->coalesce_group != 0 || r.request->coalesce_group != 0);
const bool positive_gap = same_group ? r.end < start_offset : true;
if (group_boundary ||
(positive_gap && r.end + gap_limit <= start_offset) ||
(!same_group && r.start + span_limit <= initial_offset) ||
end_past_cap ||
(!same_group && !r.request->allow_incidental_read.load(std::memory_order_relaxed)))
break;For dictionary column A then plain column B, both of length
MediumThe coalesced buffer is charged once on
if (req->coalesce_group != 0 || task->coalesce_memory_state.load(std::memory_order_acquire) == 1)
{
if (req->coalesce_group != 0 && task->length > 0)
{
uint8_t expected = 0;
if (task->coalesce_memory_state.compare_exchange_strong(expected, 2, std::memory_order_acq_rel))
{
task->coalesce_memory = MemoryUsageToken(task->length, diff);
task->coalesce_memory_state.store(1, std::memory_order_release);
}
}
memory_usage = 0;
}
set_fractions(ReadStage::BloomFilterHeader, 0.05, issuer_thread_fraction);
auto is_privileged_task = [&](size_t row_group_idx)
{
size_t i = first_incomplete_row_group.load();
if (row_group_idx != i)
return false;
const RowGroup & row_group = reader.row_groups[row_group_idx];
/// Before column data, read_ptr is not advanced yet. This row group still has to run: a coalesced
/// prefetch is charged until every handle that pins it has finished, including handles of later
/// row groups. Blocking this row group would leave that charge stuck.
if (row_group.stage.load(std::memory_order_acquire) < ReadStage::ColumnData)
return true;
return row_group.read_ptr.load() == row_group.delivery_ptr.load();
};LowThe setting is recorded as introduced in
addSettingsChanges(settings_changes_history, "26.8.6.20001.altinityantalya",
{
{"input_format_parquet_bloom_filter_coalesce_max_bytes", 0, 64 * 1024 * 1024, "Read contiguous Parquet bloom filters, or a small file that has many of them, in one request."},
}); |
…lback body. A dictionary-encoded column keeps its bloom body as a fallback in coalesce group 0, while its header stays in group 1. Both start at the same offset, and the body sorts first. The coalescing scans could take the header of the next plain column, which pulled the fallback body into the task even though no request was attached to it. The bytes were downloaded and then discarded. Both scans now refuse to cover such a body: the left scan stops before a header whose body precedes it, and the right scan does not extend past an initial header whose body is still ahead. Test 05428 checks that a longer seek issues the same number of prefetcher reads as a short one. Without this change the counts differ (6 and 5). Related: #2588 Co-authored-by: Cursor <cursoragent@cursor.com>
…over the header budget. The coalesced bloom buffer is charged once, on the `BloomFilterHeader` stage, and the charge stays until every row group pinning that buffer has finished. A large coalesced read can exceed that stage's budget, which is a share of `input_format_parquet_memory_high_watermark` split across files. The scheduler then refused every row group except the first incomplete one, so the file was read one row group at a time. A row group whose bloom header is already covered by a charged buffer adds no memory. Such row groups are now scheduled even when the budget is exceeded. A row group that would start a new read is still held back. Test 05429 compares task batches for one query under the default and a 4 MiB watermark. Related: #2588 Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
`05428_parquet_bloom_filter_dictionary_neighbor` writes a Parquet file, and the fast test build has no Parquet reader, so the test failed with `Unknown format Parquet` before checking any read counts. Tag it `no-fasttest`, like the other Parquet bloom-filter tests. CI report: https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2588&sha=19f1aceff6bcc1a00e6ccbcc21c774bf7165ca82&name_0=PR&name_1=Fast%20test PR: #2588 Co-authored-by: Cursor <cursoragent@cursor.com>
Separate GETs for each filter dominate cold reads from object storage.
input_format_parquet_bloom_filter_coalesce_max_bytesmerges contiguous filters, and reads a small file once when that is cheaper.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Read Parquet bloom filters in one request when a file has many row groups
Documentation entry for user-facing changes
ClickHouse can build Bloom-filter for Iceberg tables for queries with 'WHERE' condition not on key columns. For this ClickHouse reads metainfo for each row group from source files. In initial implementation it use single read operation for each row group, which is converted to separate GET request when data files are on object storage like S3. For large table it can produce thousands of requests. In customer test first request takes near 500 seconds, while second (when data are in local cache) less than 4 seconds.
This PR introduced reading metainfo for multiple row groups in single request, when possible. It can increase total traffic, but decrease number of requests, which can reduce time for query execution.
Benchmark
Script: https://gist.github.com/ianton-ru/50851f884c9df12d600ac0064aae43c7
Emulated latency in 0.5 seconds for one GET request.
Result on my comuter:
separate: coalescing off,
remote_read_min_bytes_for_seek= 1. Each filter is its own GET.seek: coalescing off, seek = 8 KiB. A filter shorter than 8 KiB is merged into a neighbor. A longer filter stays its own GET.
coalesce: coalescing on (64 MiB), same seek of 8 KiB. Back-to-back filters share one GET even when each filter is longer than the seek.
CI/CD Options
Exclude tests:
Regression jobs to run: