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>
|
@codex review |
Codex Review SummaryThis 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. |
![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: 348489a2c0
ℹ️ 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".
| bool already_charged = false; | ||
| if (req->coalesce_group != 0 && task->length > memory_usage | ||
| && task->coalesce_memory_charged.compare_exchange_strong(already_charged, true, std::memory_order_relaxed)) | ||
| memory_usage = task->length; | ||
| handle->memory = MemoryUsageToken(memory_usage, diff); |
There was a problem hiding this comment.
When a header starts a coalesced task, its handle receives the one-time full-task charge, but processBloomFilterHeader immediately resets that header after splitting the data handle into block handles. Those children have no coalescing group and later charge only their tiny amplified ranges, while the task's buffer remains pinned by the other headers and blocks sharing it. With many files or 64 MiB clusters, the scheduler can therefore report almost no memory for multiple live coalesced buffers, bypass memory_high_watermark, and cause excessive memory use or OOMs; the full-task charge needs a lifetime tied to the shared task rather than the first handle.
Useful? React with 👍 / 👎.
| const auto & meta = row_group.columns[column_idx].meta->meta_data; | ||
| if (meta.__isset.bloom_filter_offset && meta.__isset.bloom_filter_length && meta.bloom_filter_length > 0) | ||
| ++bloom_count; |
There was a problem hiding this comment.
For valid Parquet files that provide bloom_filter_offset but omit the optional bloom_filter_length, none of those filters contribute to bloom_count, so a file containing only such filters never enables coalescing. The existing hard-way path below explicitly derives their lengths and registers them with bloom_group, but that group remains zero here, leaving these files with the original per-filter requests; count usable offsets here and let the later length derivation supply their ranges.
Useful? React with 👍 / 👎.
| const bool same_group = initial_req->coalesce_group != 0 && r.request->coalesce_group == initial_req->coalesce_group; | ||
| const size_t gap_limit = (same_group && coalesce_group_across_gaps) ? coalesce_group_span_limit : min_bytes_for_seek; | ||
| const size_t span_limit = same_group ? std::max(coalesce_group_span_limit, bytes_per_read_task) : bytes_per_read_task; | ||
| if ((initial_req->coalesce_group != 0 && !same_group) || | ||
| end_offset + gap_limit <= r.start || | ||
| initial_offset + span_limit <= r.end || |
There was a problem hiding this comment.
When input_format_parquet_bloom_filter_coalesce_max_bytes is smaller than bytes_per_read_task—for example, a 1 MiB cap with a 1 MiB seek threshold, which makes bytes_per_read_task 4 MiB—this max permits grouped bloom-filter reads up to the larger ordinary task size. That contradicts the setting's documented role as the maximum bytes of one bloom-filter read and can unexpectedly multiply network and memory usage for deliberately small limits; grouped ranges should use the configured cap rather than raise it to the generic task size.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No code change for this comment. With the default settings the std::max does not raise the cap.
bytes_per_read_task is min_bytes_for_seek * 4. On object storage that seek is 4 MiB, so a normal read task is 16 MiB. input_format_parquet_bloom_filter_coalesce_max_bytes defaults to 64 MiB, which is already larger, so a grouped bloom-filter read stops at 64 MiB as documented.
The example (a 1 MiB cap with a 1 MiB seek, so a 4 MiB task) is a non-default setting. In that case the grouped span follows the normal task size instead of the smaller cap. Filters farther apart than min_bytes_for_seek are still not merged: on the cluster path the gap limit stays min_bytes_for_seek. The whole-file path only runs when the file itself is not larger than the cap, so that read cannot grow past the file.
… 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>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d0a392ea3
ℹ️ 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".
| if (!primitive_columns[column_idx].use_bloom_filter) | ||
| continue; | ||
| const auto & meta = row_group.columns[column_idx].meta->meta_data; | ||
| /// `bloom_filter_length` is optional (Parquet 2.10). An offset without it is still a | ||
| /// filter; the hard-way path below derives the length and registers that range. | ||
| const bool length_missing_or_positive = !meta.__isset.bloom_filter_length || meta.bloom_filter_length > 0; | ||
| if (meta.__isset.bloom_filter_offset && length_missing_or_positive) | ||
| ++bloom_count; |
There was a problem hiding this comment.
When an eligible column is fully dictionary-encoded, the later initialization loop sets column.use_dictionary_filter and deliberately skips registering its bloom-filter ranges, but this preliminary count still includes the filter metadata. For a file under the configured limit, enough such chunks make whole_file true and place every range in the coalescing group, so the first dictionary-page request reads nearly the whole object even though no bloom filter will be used. Selective queries on dictionary-encoded files can therefore regress from small dictionary reads to a full-file transfer; base this heuristic on chunks that remain bloom-filter reads after dictionary-filter selection.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
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.
CI/CD Options
Exclude tests:
Regression jobs to run: