Repository navigation
Conversation
`allow_experimental_iceberg_read_optimization` replaced nullable columns absent from partial manifest statistics with constant NULL. Record every column from the file schema so only columns added after the file was written are skipped. Related: #2525 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: 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". |
ianton-ru
added
antalya
bugfix
port-antalya
| /// Missing metrics do not mean the column is absent from the file. Record every column of the | ||
| /// file schema so the read optimization substitutes NULL only for columns added after the file | ||
| /// was written. `emplace` keeps the statistics already stored above. | ||
| if (schema_processor.hasClickhouseTableSchemaById(file_schema_id)) |
There was a problem hiding this comment.
Should we add an option to log for missing column statistics for the table.column so that we can diagnose query perf issues easier?
There was a problem hiding this comment.
This check is executed for each data file, so can produce a large number of log records when table has many files.
Make sense, but with debug or trace log level in my opinion.
There was a problem hiding this comment.
Agreed, maybe a more useful trace to output though is that we want to filter on a column but stats are missing.
That could be a followup ticket if possible? Think of it as iceberg performance tracing highlighting where Ch cannot accelerate using a feature.
Partial manifest statistics are read from the data file. A debug message names that file and reports only the count of columns without metrics. Co-authored-by: Cursor <cursoragent@cursor.com>
mkmkme
left a comment
mkmkme
left a comment
There was a problem hiding this comment.
LGTM, one minor question
There was a problem hiding this comment.
I wonder if that would've been better to have it a proper integration test since it requires a python helper
This branch has not been deployed
3 participants
allow_experimental_iceberg_read_optimizationreplaced nullable columns absent from partial manifest statistics with constant NULL. Record every column from the file schema so only columns added after the file was written are skipped.Solved #2525
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Do not treat missing Iceberg column metrics as NULL
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: