Repository navigation
refactor(query): select if branches by mask word for all types - #20904
SkyFan2002 wants to merge 7 commits into
Conversation
Follow-up to databendlabs#20881. - `eval_if` no longer evaluates a condition or branch that no enabled row reaches (a column condition that is all true, all false or NULL on the enabled rows, or an `if` nested in a masked branch). When every enabled row reaches one branch, that branch is returned without row selection. - The selection of a binary `if` moves to `kernels::select_if_then_else` and covers every result type. The mask is read one 64-row word at a time: runs of all-set or all-unset words are copied from one side in bulk, and the rows of mixed words are selected one by one. Numbers, decimals, dates and timestamps use a branch-free select on mixed words; booleans and validity are combined with word-wide bit operations; String, Binary and Variant use their concrete value type; other types use `AnyType`. Non-strict decimal branches are read at the strict width. - `ColumnBuilder::push_repeat` handles `Opaque` values, which the bulk path needs. Add kernel tests over all test data types, mask shapes and scalar/column combinations, golden cases for uniform conditions, nested `if`, String, Boolean and nullable results, and an `if_selection` benchmark group.
SkyFan2002
added
the
hold-ci
github-actions
Bot
added
the
pr-refactor
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: f9cec06d72
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`Scalar::default_value` has no Opaque value. A branch that no enabled row reaches is now left unevaluated without a placeholder scalar and is replaced by a reached branch before selection; with no reached branch the result is `ColumnBuilder::repeat_default`. A NULL scalar side of a nullable selection takes its inner values from the other side.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
SkyFan2002
removed
the
hold-ci
⏱ CI timing · Devrun 41059987934 · attempt 3 (wall time includes re-run gap) · 59.1 min (main p50 71.1 min, −12.0 min)
Minutes unless stated; durations exclude queue time unless stated; "max" = longest job in the group. Baseline = median of the last 10 successful runs (Dev: |
| start: 0, | ||
| run: None, | ||
| }; | ||
| let mut chunks = mask.chunks::<u64>(); |
There was a problem hiding this comment.
could call BitChunksExact if offset is 0
SkyFan2002
added
the
hold-ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c76068b866
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A decimal inside an array, map or tuple with a non-strict width can only be converted row by row. `if` results whose type nests a decimal skip the single-branch return and use per-row selection.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5675a794a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
sundy-li
left a comment
sundy-li
left a comment
There was a problem hiding this comment.
AI-assisted review of f9f9f54. The branch-skipping/word-adaptive selection design looks sound in the paths examined. The nested non-strict decimal issue identified on 4e97eb3 is fixed in the current revision: both independent review probes now pass (128 rows split into a 64-row THEN run and a 64-row ELSE run, plus single-branch tuple normalization). The two selection-kernel tests and all four scalars::control tests also pass.
No additional blocking issue found in this focused recheck. Two non-blocking suggestions below: retain the full-word/multiple-run regression case in the test suite, and clarify the deliberately conservative nested-decimal fallback. Benchmarks and generated assembly were not revalidated.
| /// Whether `data_type` has a decimal inside an array, map or tuple (also | ||
| /// through the physical type of an aggregate state). A non-strict decimal (e.g. | ||
| /// `Decimal(10, 2)` carried by `Decimal128`) nested there can only be converted | ||
| /// to the strict width row by row, so such types skip the bulk paths. |
There was a problem hiding this comment.
Non-blocking: please describe this as a conservative type-based fallback, rather than saying nested decimals can only be converted row by row. has_nested_decimal checks the logical type, not the actual child widths, so even already-strict nested decimal values intentionally lose both bulk selection and single-branch reuse. That is a reasonable correctness-first choice here, but the comment should make the scope explicit. A future layout-aware check or recursive pre-normalization could retain the fast paths; no need to add that complexity to this fix. The current benchmarks do not measure this nested-type case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9f9f54ef0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
SkyFan2002
added
hold-ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
SkyFan2002
removed
the
hold-ci
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
Follow-up to #20881, which added a typed row selection for a binary numeric/decimal
if. This PR applies three further improvements toifevaluation, suggested by sundy-li after #20232 and used by other engines.1. Skip branches that no enabled row reaches (
Evaluator::eval_if, all result types)Each branch already runs under
validity & flag, and an all-unset validity means the branch runs for nobody (errors are dropped, seeexpand_scalar_error). Before this PR such a branch was still evaluated over the whole block. Now:falsecondition already was. Before selection it is replaced by a reached branch of the same type, which is only read on rows that are not enabled; this also avoidsScalar::default_value, which has noOpaquevalue;ifnested inside a masked branch), that branch is returned directly without row selection.Row errors keep their existing behaviour: errors on rows that are not enabled were never reported, and the skipped branches have no enabled row. The result type, nullability and the values on enabled rows are unchanged; only the content of NULL slots in the output can differ (two golden files show this in
evaluation (internal)).Prior art: DuckDB skips a THEN with no selected rows and runs THEN/ELSE directly when a WHEN covers all rows (
src/execution/expression_executor/execute_case.cpp); DataFusion checkshas_true/has_falseinCaseExpr::expr_or_expr(datafusion/physical-expr/src/expressions/case.rs); ClickHouse's short-circuitmaskedExecutedoes not run a branch whose mask has no ones (src/Columns/MaskOperations.cpp).2. Word-adaptive selection for every result type (new
kernels::select_if_then_else)The mask is read one 64-row word at a time (
Bitmap::chunks::<u64>). Consecutive all-set or all-unset words are merged into runs and copied from one side in bulk (push_item_repeatfor a scalar,append_columnof a slice for a column); the rows of a mixed word are selected one by one. This replaces the #20881 selector and extends it from numbers/decimals to every type:Nullable(T)is split into inner values (selected asT) and validity (selected as a bitmap), so every nullable type is covered. When one side is a NULL scalar the other side provides the inner values, so the column is reused without a copy;Booleanand validity are combined with word-wide bit operations,(m & t) | (!m & f);String,BinaryandVariantgo through their concreteValueType; any other type usesAnyType.Decimal branches with a non-strict width (e.g.
Decimal(10, 2)carried byDecimal128) are converted to the strict width instead of falling back to the generic path, keeping the regression test from #20881 passing.Prior art: run-based copy in Arrow
zip(arrow-select/src/zip.rs,SlicesIterator+MutableArrayData::extend), Polarsif_then_else_extend(polars-compute/src/if_then_else/mod.rs), ClickHouse's per-64-rowprefixToCopyin filters (src/Columns/ColumnVector.cpp). Deciding per word avoids a block-level density heuristic: a block of a clustered table often has one or two long runs even when half of its rows are true.3. Branch-free select for fixed-width types on mixed words
Numbers, decimals (64/128/256),
DateandTimestampwrite into aVec<N>; on a mixed word both sides are loaded and one is picked without a branch, as in Polars'if_then_else_loopand ClickHouse'sfillVectorVector(src/Functions/if.cpp).Also:
ColumnBuilder::push_repeatgains theOpaquearm thatpushalready had; the bulk path needs it.Benchmark
cargo bench -p databend-common-functions --bench bench(bench profile, i.e. release with debug assertions), groupsif_selection(new, 65,536 rows) andnumeric_if(from #20881). The timed section isEvaluator::run: condition, branches and selection. The base (dc7efce824) and this PR were built as separate binaries, pinned to one core of an AMD Ryzen 9 9950X, and run three times interleaved; the table shows the median of the three Divan medians.Conditions:
alternating=x % 2 = 0(every mask word mixed),clustered=x < 32768(two runs),all_true=x >= 0.if(<cond>, x, x * 3)alternatingif(<cond>, x, x * 3)clusteredif(<cond>, x, x * 3)all_trueif(<cond>, x, NULL)alternatingif(<cond>, x, NULL)clusteredif(<cond>, x, NULL)all_trueif(<cond>, s, 'none')alternatingif(<cond>, s, 'none')clusteredif(<cond>, s, 'none')all_trueif(<cond>, x > 10, x < 10)alternatingif(<cond>, x > 10, x < 10)clusteredif(<cond>, x > 10, x < 10)all_truenumeric_if/if_contains65,536numeric_if/if_modulo65,536numeric_if/if_boolean65,536For
int64alternating the conditionx % 2 = 0andx * 3dominate: a standalone loop with the same release settings (opt-level = "s") selects 65,536i64rows from two columns with an alternating mask in about 21 µs with the branch-free form, against 44 µs with a per-rowif+push.Tests
kernel::test_select_if_then_else(databend-common-expression): every type ofget_all_test_data_types(All)plus Decimal256, Tuple and 11 nullable types; lengths 0–700; random, run, all-set and all-unset masks with a random bit offset; column/column, scalar/column, column/scalar, scalar/scalar and NULL scalars; compared row by row with a per-row reference.kernel::test_select_if_then_else_non_strict_decimal: aDecimal128column of typeDecimal(10, 2)is read at theDecimal64width.scalars::control::test_if_with_opaque_result:OpaqueandNullable(Opaque)results with all-true, all-false and mixed column conditions and a NULL branch.scalars::control::test_control: golden cases for all-true and all-false/NULL column conditions with a division by zero in the other branch, a multi-branchifwhose first condition takes every row, a nestedifwhose inner else is reached by no enabled row, and String, nullable String and Boolean results.cargo test -p databend-common-expression --test it,cargo test -p databend-common-functions --test it,make lint.No setting or default change.
Tests
Type of change
AI assistance
if/CASE implementations of other engines, implemented the branch skipping ineval_if, the selection kernel, its tests and the benchmark, ran the benchmark and drafted this PR descriptionThis change is