Repository navigation
Conversation
…rom manifests. The shortcut has to answer those aggregates from manifest `record_count` for `IcebergLocal`, `IcebergS3`, `Iceberg`, their table functions, and `icebergS3Cluster`, including a partition range and a subquery. The test fails until that read exists. Co-authored-by: Cursor <cursoragent@cursor.com>
…rom manifests The partition value and `record_count` are already in the manifest, so these aggregates do not scan the data files. Plain `count()` stays on the trivial-count path. The generic `Iceberg` engine forwards `isIcebergConfiguration` so `system.iceberg_files` and this shortcut see the real backend. Co-authored-by: Cursor <cursoragent@cursor.com>
…s off `SELECT count()` and `count(column)` with no `WHERE` or `GROUP BY` follow the setting. Partition `min`, `max`, and grouped `count` still come from manifests. Co-authored-by: Cursor <cursoragent@cursor.com>
`max(read_rows)` kept a scan from an earlier run with the same `log_comment` and classified the new execution as a data read. 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. |
|
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: 9a2d4a9fa9
ℹ️ 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".
|
|
||
| std::optional<std::vector<IdentityPartitionFile>> IcebergMetadata::getDataFileIdentityPartitions(ContextPtr local_context) const | ||
| { | ||
| auto [actual_data_snapshot, actual_table_state_snapshot] = getRelevantState(local_context); |
There was a problem hiding this comment.
When an Iceberg commit lands after query analysis but before this planner call, getRelevantState resolves the latest snapshot instead of the datalake_table_state already pinned in the table's StorageSnapshot. The prepared aggregate can therefore include newly appended files while using column types and schema mappings from the older analyzed snapshot; the normal read path explicitly pins one coherent snapshot to prevent this race. Pass the pinned table state into this API and resolve the manifest files from that state.
Useful? React with 👍 / 👎.
| const auto & function = node->as<const FunctionNode &>(); | ||
| if (!function.isOrdinaryFunction() || function.getFunctionName() == "arrayJoin") | ||
| return false; |
There was a problem hiding this comment.
For a predicate such as WHERE rand() % 2 = 0, this accepts rand as an ordinary zero-argument function and evaluates it once per manifest data file; the regular scan evaluates it once per data row. Consequently count() returns sums of entire record_count values rather than the selected row count, and stateful/block-dependent functions have the same problem. Only predicates deterministic in the scope of the query and independent of row/block state should use this optimization.
Useful? React with 👍 / 👎.
A commit after analysis must not change `min`, `max`, or grouped `count`. The files come from `datalake_table_state`, not the latest metadata. Co-authored-by: Cursor <cursoragent@cursor.com>
A per-row function must not keep a whole file `record_count`. Deterministic functions of the identity column, such as `toYear`, stay on the manifest path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
ianton-ru
added
antalya
port-antalya
|
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 |
|
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". |
…on-aggregate-metadata
|
@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: 4eb80bfbb9
ℹ️ 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".
| size_t group_index = 0; | ||
| if (!group_keys.empty()) | ||
| { | ||
| auto [it, inserted] = group_positions.emplace(key, groups.size()); |
There was a problem hiding this comment.
When an identity-partition query sets max_rows_to_group_by below the number of partitions, this loop still materializes every key. For example, GROUP BY d with two partitions and max_rows_to_group_by = 1, group_by_overflow_mode = 'throw' should raise TOO_MANY_ROWS, while this prepared source returns both groups; ANY and BREAK likewise exceed the requested cap. The merge phase receives a pre-aggregated chunk and does not reapply the limit after ingesting it, so decline this optimization when max_rows_to_group_by is nonzero or implement the configured overflow behavior here.
Useful? React with 👍 / 👎.
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: b92b134d3c
ℹ️ 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 auto & settings = query_context->getSettingsRef(); | ||
| /// `ReadFromPreparedSource` cannot be cloned by the Cascades optimizer. | ||
| if (settings[Setting::make_distributed_plan] && settings[Setting::enable_cascades_optimizer]) | ||
| return false; |
There was a problem hiding this comment.
When allow_experimental_iceberg_read_optimization = 0, this new planner path still answers min, max, filtered counts, and grouped counts from manifests. The existing Iceberg read optimization is explicitly gated by this setting in StorageObjectStorageSource.cpp, so users disabling the experimental optimization—particularly as a correctness workaround—can no longer force these queries to scan the data files. Check this setting before applying the metadata aggregation.
Useful? React with 👍 / 👎.
…ad_optimization` is off Co-authored-by: Cursor <cursoragent@cursor.com>
CI triage for
|
| Check | Failing tests | Verdict |
|---|---|---|
| Stateless tests (amd_debug, parallel) | 05428_iceberg_partition_aggregate_metadata (failed 5 times, including reruns without randomized settings) |
PR-caused (test vs. CI config) |
| Stateless tests (amd_asan_ubsan, distributed plan, parallel, selected tests) | 05428_iceberg_partition_aggregate_metadata |
PR-caused (same issue, plus a second one, see below) |
| Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/8) | test_storage_iceberg_with_spark/test_export_partition_iceberg.py::* (21 tests) + session timeout |
Pre-existing |
| Integration tests (amd_asan_ubsan, db disk, old analyzer, 6/8) | test_database_backup/test.py::* (17 tests) |
Pre-existing |
| Integration tests (amd_asan_ubsan, db disk, old analyzer, 7/8) | test_backward_compatibility/test_custom_s3_disk_remote_url_allow_hosts.py::* (2 tests) |
Pre-existing |
| Integration tests (amd_asan_ubsan, targeted) | Same test_database_backup and test_custom_s3_disk_remote_url_allow_hosts tests (18 total) |
Pre-existing |
1. 05428: GROUP BY queries never use the metadata path in CI (PR-caused)
The same diff appears in every engine block and every run:
-groupby_d metadata
-groupby_d_range metadata
-max_of_groupby_count metadata
-max_of_groupby metadata
-groupby_d_only metadata
+groupby_d data read_rows=10000 limit=10
+groupby_d_range data read_rows=2000 limit=2
+max_of_groupby_count data read_rows=10000 limit=10
+max_of_groupby data read_rows=10000 limit=10
+groupby_d_only data read_rows=10000 limit=10
The CI diagnosis says: "Test also fails without randomized settings (not a randomization issue)." max/min/count without GROUP BY do use metadata. Only the GROUP BY shapes fall back to a scan.
Cause: the CI server profile sets a GROUP BY row limit for every query. In tests/config/users.d/limits.yaml:18:
max_rows_to_group_by: 10GThe PR's guard in src/Planner/PlannerJoinTree.cpp:794 turns the optimization off whenever that setting is non-zero:
if (query_node.hasGroupBy() && settings[Setting::max_rows_to_group_by])
return false;So every GROUP BY query in this test scans the data in CI. This probably passed locally because the default max_rows_to_group_by is 0.
Suggested fix (test only): add max_rows_to_group_by = 0 to the SETTINGS block in run() in 05428_iceberg_partition_aggregate_metadata.sh:
SETTINGS
log_comment = '${tag}',
log_queries = 1,
max_rows_to_group_by = 0,
optimize_trivial_count_query = 1,
...The group_by_row_limit* cases set max_rows_to_group_by = 1 themselves, so they are not affected.
Design question, not a CI blocker: since CI (and some production profiles) set a very large max_rows_to_group_by as a safety net, the optimization will be off for those users too. One option is to keep the guard only when the number of partition groups could exceed the limit, i.e. compare the group count against the limit instead of bailing out whenever the setting is set.
2. 05428: group_by_row_limit_any result depends on thread count (PR-caused, distributed plan job only)
group_by_row_limit_any
-2
+4
This query runs on the normal scan path, which is correct here. With group_by_overflow_mode = 'any', every aggregating thread stops on its own after it passes the limit. That job ran with max_threads 2, so the result was 2 threads × 2 keys = 4, not 2. The test's comment ("limit 1 keeps two groups") only holds for a single thread.
Suggested fix: add max_threads = 1 to that inner query. Alternatively, assert something that doesn't depend on threads, such as SELECT count() < 10, which checks that not every partition was emitted.
3. Integration failures: pre-existing on antalya-26.8
I compared the praktika reports of two unrelated PRs into the same base:
- Fixes for Iceberg Puffin Delete Vectors #2715 (
afa617e, Puffin delete vectors) - Fix early manifest deletion in CAS GC #2730 (
fa0e5fe, CAS GC fix)
Both have exactly the same failing tests in the same shards:
test_export_partition_iceberg.py::*in db disk 5/8test_database_backup/test.py::*(same parametrizations) in db disk 6/8test_custom_s3_disk_remote_url_allow_hosts.py::test_{table,database}_on_custom_s3_disk_loads_after_upgradein db disk 7/8
This PR only touches the Iceberg read path, planner, and storage hooks. It does not touch backup, disks, or the export-partition write path. Nothing to do here for this PR; these need fixing on the base branch.
Next step: apply the two test changes to 05428_iceberg_partition_aggregate_metadata.sh; no C++ change is needed for CI. I haven't pushed anything; ask me if you want me to commit these. The rerun on the PR will confirm the fix.
…p_by` fits Co-authored-by: Cursor <cursoragent@cursor.com>
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Iceberg
min,max, andcountover identity partition columns, includingGROUP BYand a filter on those columns, are answered from manifestrecord_countinstead of reading the data files. Unfilteredcount()still uses the existing trivial-count optimization, andoptimize_trivial_count_query = 0still makes that query scan the data.Documentation entry for user-facing changes
No new setting or syntax.
SELECT max(d),SELECT min(d),SELECT count() ... WHERE d >= ..., andSELECT d, count() ... GROUP BY dread identity partition values and per-filerecord_countfrom the current snapshot when every aggregate argument,GROUP BYkey, andWHEREcolumn is a plain identity partition column. Unfilteredcount()stays onoptimize_trivial_count_query.CI/CD Options
Exclude tests:
Regression jobs to run:
Title: Answer Iceberg identity partition
min,max, and groupedcountfrom manifestsMotivation
count()on an Iceberg table already avoids reading data files.minandmaxof an identity partition column, andcount()withGROUP BYorWHEREon that column, still opened every Parquet file. On a large table that is one file per partition value, the result is already stored in the manifest: the identity partition value andrecord_countof each live data file.The same scan happened for table functions and for
icebergS3Cluster. A cluster with two local shards would be wrong if each shard answered the full snapshot.Approach
After trivial count and the sparsity-count shortcut decline, a single Iceberg table or table function can be answered from the current snapshot. The query may contain only
count,min, andmaxof plain identity partition columns, an optionalGROUP BYof those columns, and an optionalWHEREthat uses only those columns. Each live data file contributes one row. The planner builds one mergeable aggregate state per group and finishes the query withMergingAggregated, soHAVING,ORDER BY,LIMIT, and windows still run.Unfiltered
count()is left to the existing trivial-count path whenoptimize_trivial_count_query = 1, including theIcebergTrivialCountOptimizationAppliedprofile event.optimize_trivial_count_query = 0makes unfilteredcount()andcount(column)read the data files.min,max,WHERE, andGROUP BYstay on the manifest path.The shortcut returns to a normal scan when it cannot prove the answer. That includes equality and position deletes, deletion vectors, a negative
record_count, an unreadable path, a missing identity value, a row policy,PREWHERE, an additional table filter, a transaction,FINAL,SAMPLE,STREAM, a non-plain expression,TOTALS,ROLLUP,CUBE, grouping sets, andarrayJoininside an aggregate. Custom-key parallel replicas and secondary queries also scan. On a cluster table function the initiator evaluates the snapshot once, so two shards do not add the same files twice.bucket,truncate, and time transforms are not identity values and are not used. A non-partition column still scans. Snapshottotal-recordsis not used; the count is the sum of live data-filerecord_count.The generic
Iceberg(url)engine reports the same Iceberg configuration flag asIcebergS3, so this path,system.iceberg_files, andsystem.iceberg_historysee it.Tests
05428_iceberg_partition_aggregate_metadataloads 10 identity partitions of 1000 rows and checksIcebergLocal,icebergLocal,IcebergS3,icebergS3,icebergS3Cluster,Iceberg(url), andicebergwithstorage_type = 's3'. Unfilteredcount()stays on trivial count.maxof a non-partition column scans. Partitionmin/maxand groupedcountread at most the matching file count, and a cluster does not double that count.optimize_trivial_count_query = 0makes unfilteredcount()scan. Thequery_logcheck uses the latestQueryFinishrow for each statement.