Repository navigation
Conversation
…ch NULL. A clustering column can contain nulls while its manifest bounds still describe the other values. `partner_id = '...'` cannot match those nulls, so the file can be skipped. `IS NULL` still keeps a file that may contain nulls. Co-authored-by: Cursor <cursoragent@cursor.com>
…cate cannot match NULL. `KeyCondition::mayBeTrueOnNull` is the shared check for Iceberg files and Parquet chunks. An equality that rejects NULL still skips a chunk whose other values miss, including an all-null row group. `IS NULL` keeps it. Co-authored-by: Cursor <cursoragent@cursor.com>
…in` matches NULL. `mayBeTrueOnNull` treated every comparison and `IN` as unknown on NULL, so a chunk whose other values missed the probe was skipped even though the NULL rows matched. 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: 9732cac9ee
ℹ️ 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".
| case RPNElement::FUNCTION_IN_RANGE: | ||
| case RPNElement::FUNCTION_NOT_IN_RANGE: | ||
| case RPNElement::FUNCTION_ARGS_IN_HYPERRECTANGLE: | ||
| case RPNElement::FUNCTION_POINT_IN_POLYGON: | ||
| /// A bare comparison is unknown on NULL. `assumeNotNull` and other wrappers can make it true. | ||
| stack.push_back(uses_this_column && element.monotonic_functions_chain.empty() | ||
| ? TruthOnNull::isUnknown() : TruthOnNull::any()); |
There was a problem hiding this comment.
When the filter is NOT isNotDistinctFrom(nullable_col, c), KeyCondition represents the inner null-safe equality as FUNCTION_IN_RANGE, but this branch classifies its result on NULL as unknown; FUNCTION_NOT therefore leaves it unknown even though the actual result is true. For a Parquet row group/page or Iceberg file containing NULLs whose non-null values are all c, the finite range makes the negated predicate appear impossible and the new pruning path drops the chunk, losing every NULL row that should match. Preserve the atom's null-safe origin/null truth value in the RPN, or conservatively keep nullable chunks when that distinction is unavailable.
Useful? React with 👍 / 👎.
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". |
ianton-ru
changed the title
|
@codex review |
ianton-ru
added
antalya
port-antalya
|
@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 |
|
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". |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Iceberg file min/max pruning and Parquet row-group and page min/max pruning can now skip a nullable column when its non-null range misses the predicate and SQL NULL cannot make the predicate true.
IS NULL,assumeNotNull, andIN/NOT INwithtransform_null_instill read a chunk that contains NULL.Documentation entry for user-facing changes
No new setting or syntax. Existing Iceberg manifest min/max pruning and Parquet filter pushdown (
input_format_parquet_filter_push_down,input_format_parquet_page_filter_push_down) apply the predicate to the non-null bounds of a nullable column.CI/CD Options
Exclude tests:
Regression jobs to run:
Motivation
Unity liquid clustering on Iceberg is a sort order plus per-file
lower_bounds/upper_bounds. It is not a partition spec. For a nullable clustering column, ClickHouse already had those bounds, but it refused to use them unlessnull_value_countswas present and exactly 0. Parquet did the same for row groups and pages: a nullable column with any null widened the min/max to infinity, so a predicate such aspartner_id = 'sg2o27m'could not skip a chunk whose other values missed the probe.A null does not satisfy that comparison. The chunk can be skipped.
IS NULLmust still read it.Related: #2395 reads Delta
delta.clusteringonly. It does not change the Iceberg or Parquet readers.Approach
KeyCondition::mayBeTrueOnNullwalks the predicate and reports whether it can be true when one key column is SQL NULL. A bare comparison is unknown on NULL, soWHEREdoes not keep that row.IS NULLis true.IS NOT NULLis false.Iceberg manifest pruning skips a file when the non-null range misses and either the manifest says the column has no nulls or
mayBeTrueOnNullis false. Parquet row-group and page stats keep the finite non-null range in that same case. An all-null row group has no finite min/max and is dropped only when NULL cannot match.Two predicates are true for NULL and must not be pruned away:
assumeNotNullreplaces NULL with the nested value, so a monotonic wrapper is treated as able to match.nullIn/notNullIn, whichtransform_null_inrewritesIN/NOT INinto, can match NULL. The RPN stores both the normal and the null-comparing forms as the same set atom, so a chunk that contains NULL is kept for everyIN/NOT IN.A null-preserving wrapper such as
toDateis also kept when the chunk contains NULL. The rows are unchanged. The chunk is not pruned from that column's bounds.Tests
05386_iceberg_nullable_minmax_prunes_null_rejecting_predicate: an Iceberg file whose non-null range missespartner_id = '...'orNOT partner_id = '...'is pruned even if it contains NULL.IS NULLkeeps it.05387_parquet_nullable_minmax_null_rejecting: the same rule for Parquet row groups and pages, including an all-null row group,assumeNotNull, andIN/NOT INwithtransform_null_in = 1.