Repository navigation
[refine](expr) Bind CAST executors for direct column execution - #69639
Merged
Merged
Conversation
### What problem does this PR solve? Problem Summary: CAST expressions construct temporary Blocks and execute through the generic function framework. Bind typed column executors during preparation, capture conversion settings in CastOptions, and pass RuntimeState during execution. Keep a FunctionCast compatibility adapter for direct function callers and apply schema evolution options before binding. ### Release note None ### Check List (For Author) - Test: BUILD_TYPE_UT=ASAN ./run-be-ut.sh --run --filter='*Cast*' -j48; 395 passed, 1 skipped, 4 disabled. Regression tests were not run. - Behavior changed: No; CAST execution refactor. - Does this need documentation: No.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No actionable regression was verified in PR #69639 at head 06cc333. The review covered every hunk in the authoritative diff across all 59 changed files, with two complete-scope reviewers and an additional settings/column-shape risk reviewer. All three returned NO_NEW_VALUABLE_FINDINGS; the review converged in one round. No existing inline finding remains to carry forward because both the supplied context and live PR contained no inline comments. There was no additional user-provided focus.
Critical checkpoint conclusions:
- Goal and evidence: CAST now binds type/option executors during preparation and executes on source/target columns. Scalar, complex, JSONB, Variant, SQL CAST/TRY_CAST and format-v2 consumers were traced. New tests cover binding reuse, nullable constants, source/conversion NULL merging, empty unsupported casts and execution after original-context closure; existing TRY_CAST and EvolutionCast tests cover conversion errors, clones and overflow. These tests were inspected, not executed.
- Scope and clarity: the broad signature migration is consistently focused on removing temporary Blocks and repeated binding. FunctionCast remains the compatibility adapter for direct function callers.
- Concurrency and locks: no new threads, locks or mutable shared execution scratch were introduced. Bound closures own immutable options/types and stateless shared kernels; execution uses local output/mask storage. No lock-order or heavy-under-lock change was found.
- Lifecycle and initialization: executors retain no FunctionContext pointer. Runtime context clones share query RuntimeState; deep expression clones rebuild unprepared nodes. Closing the original expression context does not invalidate a clone's captures. No new cross-translation-unit static initialization dependency was introduced.
- Configuration: no configuration item was added. Existing query strict/decimal flags are read before binding; explicit strict and evolution overrides are applied first. Direct function callers set JSON flags before per-call preparation.
- Compatibility: internal C++ signatures and their in-tree callers are migrated together; the FunctionCast adapter remains. No storage, serialization or FE/BE wire-format change occurs. Final ELF/glibc compatibility was not exercised in this static review.
- Thrift/protobuf: no gensrc/thrift or gensrc/proto change; field-ID/enum upgrade checks are not applicable.
- Parallel paths: ordinary CAST, TRY_CAST retries, the generic function adapter, scalar/complex/JSONB/Variant conversions, format-v2 Cast/EvolutionCast, mapper/TableReader consumers and unchanged Variant explode callers were checked for corresponding behavior.
- Special conditions and NULLs: zero rows, constant one-row execution, parent-masked constant expansion, source/conversion NULL-map merging and nested ARRAY/MAP/STRUCT payload masks retain aligned storage and owners. EvolutionCast's DATE-to-TIMESTAMP_NS strict override binds before execution; the separate Paimon truncation path is preserved. Branch reasons and adjacent invariants were checked.
- Test coverage: focused unit coverage exercises positive, negative, NULL, empty, nested, overflow and clone paths. The broader existing scalar/JSONB/Variant tests are migrated to column arguments. End-to-end regression execution, external-system interoperability and performance measurements remain outside this static invocation.
- Test expectations/results: added expected Boolean, NULL, integer and conversion-error outcomes agree with the inspected kernels. No regression .out file is added or edited. Author-reported test counts were not independently reproduced.
- Observability: existing conversion diagnostics and status propagation remain; no new distributed operation or operational control surface requires additional metrics. No runtime profile or benchmark claim is made.
- Transactions and persistence: no transaction, visible-version, rowset/delete-bitmap, EditLog, replay or failover implementation is changed; those mechanisms' checkpoints are not applicable.
- Writes and atomicity: no write/commit protocol is introduced. Column outputs and masks retain COW ownership; map deduplication detaches shared children. No source-owner loss or new crash-cleanup path was verified.
- FE/BE variables: no new transmitted variable or send path is added; constant-folding and direct function calls retain the existing adapter.
- Performance and memory: binding is reused and temporary execution Blocks are removed; row-dependent columns/masks stay local and tracked through existing column ownership. No additional actionable allocation, complexity or redundant-work regression was verified. Read-only build-support/check-build-hygiene.sh passed header layering/bans/budgets/PCH, extern-template pairing and unity-skip checks. No measured speedup is claimed.
- Other risks and final sweep: settings inheritance, constant/nullable shape, clone lifetime, TRY_CAST error classification, timezone access, aliasing and schema-evolution behavior were rechecked. Variant's default non-strict options do not discard observable JSON/decimal behavior on the traced dispatch paths: JSONB uses its dedicated route, and decimal range checks are independent of the sanitation flag. All initial risks and candidates have evidence-backed dismissals; no unresolved code-level question or changed-file coverage gap remains.
Validation limits: this was a static review. No Doris build, unit test, regression test or runtime benchmark was run; no source file was modified. Convergence describes completed static review coverage and candidate disposition, not runtime validation.
Review converged after 1 round(s).
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28530 ms |
Contributor
TPC-DS: Total hot run time: 150720 ms |
Contributor
ClickBench: Total hot run time: 24.41 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
approved these changes
Oct 28, 2026
HappenLee
left a comment
HappenLee
left a comment
Contributor
There was a problem hiding this comment.
Statically reviewed the CAST/TRY_CAST and format-v2 execution paths, including NULL handling and executor lifetimes. No blocking correctness issues found. Current CI checks pass. Adapter simplification and focused before/after CAST benchmarks can be addressed as follow-up improvements.
yiguolei
approved these changes
Oct 28, 2026
Gabriel39
pushed a commit
that referenced
this pull request
Oct 29, 2026
…lete and data file types (#69711) ### What problem does this PR solve? Issue Number: None Related PR: #69536, #68267 Problem Summary: **In short.** FileScannerV2 now compares an Iceberg equality-delete key in the wider of the delete file's and the data file's own key types, instead of in the one table type the FE sends for the whole scan. For some table histories that single type is wrong: the reader then deletes rows it must keep, keeps rows it must delete, or fails the scan. This follows the review of #69536, where these cases came up. **Background.** An equality delete removes a data row whose key values equal a tombstone's. After a type promotion the two sides can be stored in different types: tombstones written before an INT -> LONG promotion hold INT values that must still match LONG data. #68267 made FileScannerV2 compare in the current schema's type, and for a key that the current schema no longer has, the FE sends a type recovered from the retained schemas (#69536). The reader casts both the tombstones and the data keys to that one type. **The problem, and what it cost.** One type per scan cannot describe every delete file: | History | Table type from the FE | Delete file | Data file | Before | After | |---|---|---|---|---|---| | `expireSnapshots().cleanExpiredMetadata(true)` removed the schema of a schema-only INT -> LONG promotion of a dropped key | INT | LONG 4294967296 | INT, one NULL key | the tombstone becomes NULL in INT and deletes the NULL-key row | row kept | | A read pinned before a later DATE -> TIMESTAMP_NS promotion of the key | TIMESTAMP_NS | DATE 1000-01-01 | DATE 1000-01-01 | scan fails: `TIMESTAMP_NS overflow when casting row ...` | row deleted | | No retained schema gives the key a concrete type, so the FE sends Iceberg `unknown` as STRING | STRING | TIMESTAMP_NS 2024-01-02 00:00 | DATE 2024-01-02 | `2024-01-02` != `2024-01-02 00:00:00.000000000`, the row stays visible | row deleted | | The delete was prepared under a TIMESTAMP_NS schema that the table later abandoned for TIMESTAMP | TIMESTAMP | TIMESTAMP_NS 12:00:00.000000500 | TIMESTAMP 12:00:00.000000 | the tombstone is rounded to microseconds and deletes the row | row kept | Three of the four return wrong rows without any error. The V1 reader is not changed by this PR. **How this PR fixes it.** - The delete block keeps the delete file's own key types. When the predicate for a data file is built, each key is compared in the wider of the delete file's type and the data file's type (`equality_delete_comparison_type`). The accepted widenings are the Iceberg promotions (INT -> LONG, FLOAT -> DOUBLE, decimal precision, DATE -> TIMESTAMP and DATE -> TIMESTAMP_NS), a wider fractional scale of the same date-time kind, wider integers, STRING -> VARBINARY (an Iceberg BINARY key decodes as either one, depending on `enable_mapping_varbinary`), and TIMESTAMP -> TIMESTAMP_NS. The last one is not an Iceberg promotion; it is accepted for the abandoned-schema case above, because scaling microseconds to nanoseconds is exact. When the table field is `timestamptz`, a `DATETIMEV2` key (Parquet INT96, which carries no instant annotation, as Spark and Hive write it) and a `TIMESTAMPTZ` key compare as `TIMESTAMPTZ` at the larger scale, the `DATETIMEV2` side converted with the session time zone as the projection converts that column. When the table field is the STRING placeholder the FE sends for an Iceberg TIME, which Doris cannot represent, an ORC TIME key (a LONG annotated `iceberg.long-type=TIME`, read as `BIGINT`) and a Parquet one (`TIME(MICROS)`, read as `TIMEV2(6)`) compare as `TIMEV2(6)`, the `BIGINT` side converted exactly as microseconds from midnight by a dedicated expression (`CAST(BIGINT AS TIME)` reads the integer's digits as HHMMSS), and a value outside 00:00:00..23:59:59.999999 fails the scan. Two types that convert in neither direction fail the scan instead of being compared in a third type. - The data keys are cast to that type. When it is the data file's wider type, the tombstones are widened once per scanner, and the converted delete block and its hash index are cached next to the original (`_convert_equality_delete_filter`). A table without type changes is compared as before, with no cast and no copy. - A key that the data file lacks is compared through its initial default, in the wider of the delete file's type and the table type. When that default is NULL (no initial default, or a struct default `{}` whose leaf has none) the key is NULL in that file whatever its table type, so it is compared in the delete file's type (an `unknown` key arrives as STRING). A non-NULL default behind the STRING placeholder of an Iceberg TIME is Iceberg's text of the value (`00:00`), with at most six fractional digits, so it is parsed exactly in the tombstones' type, `TIMEV2` (`TIMEV2(6)` against an ORC TIME tombstone), by the strict comparison cast. A non-NULL default of a `timestamptz_ns` key fails the scan with `NotSupported`: V2 reads its tombstones as `TIMESTAMPTZ(6)` (Parquet floors the nanoseconds, ORC rounds them), so no parse of the nanosecond default compares exactly; the default epoch would match a tombstone 100 ns later and delete every row of the file. - `AncestorNullDefaultExpr`, which gives a missing child the NULL of its NULL parent and its default otherwise, now opens its children. Its `open()` returned before opening them, because `prepare()` had already set the flag its guard checks, so a default cast to the comparison type below it was never opened and aborted the BE with `Check failed: _open_finished` in `Cast`. - The comparison casts are strict whatever the session's cast mode, through a new `strict` flag on `EvolutionCast`: a non-strict overflow would turn a key into NULL and let a NULL tombstone delete it. DATE -> TIMESTAMP_NS was already strict; other `EvolutionCast` users are unchanged. - A delete file without rows is skipped before its keys are resolved: it deletes nothing, but the predicate evaluates its keys first, so its strict casts could fail the scan (a DATE key 1000-01-01 against an empty TIMESTAMP_NS delete file). - The table types from the FE still resolve table fields and the defaults of missing keys; the delete-file cache key is unchanged. **Results.** - 14 new BE unit tests in `iceberg_reader_test.cpp`: the four histories above; INT tombstones against LONG, INT and LONG data files of one scan (the third split reuses the cached widened copy); a missing key's INT default against a LONG tombstone; a missing key without a default under an `unknown` table type; a microsecond key outside the nanosecond range, which must fail; and incomparable key types. With the reader before this PR, 6 of them fail; the other three pin behavior the old code shares. Removing the TIMESTAMP -> TIMESTAMP_NS widening fails 2, and making the casts non-strict fails 1. Three more came from review: an INT96 data key against an adjusted-UTC tombstone in Asia/Shanghai, a NULL leaf under a struct default `{}`, and an empty TIMESTAMP_NS delete file against a DATE key; removing the INT96 rule, the NULL-leaf walk or the empty-file skip fails its test. Two more came from the third round: Iceberg TIME keys across all four ORC / Parquet data and delete file pairs, with NULL and non-NULL tombstones, and a nested Iceberg TIME key added with a text default, against Parquet TIME and ORC TIME tombstones, under NULL and non-NULL parents. Removing the TIME pair rule or the default parsing, or converting the ORC TIME with a SQL cast, fails them; leaving `AncestorNullDefaultExpr`'s children unopened aborts. One more came from the fourth round: a nested `timestamptz_ns` key with the defaults `...0000001`, the epoch and `...0000005` fails against an epoch, a 100 ns and a 500 ns tombstone, while a key without a default still matches NULL tombstones only; parsing those defaults again fails it. - End to end on a local cluster: a v2 table in a `file://` Hadoop catalog, built with the Iceberg 1.12.0 Java API: `k INT` with rows (1, NULL), (2, 0), (3, 1); `k` promoted to LONG without a snapshot; a delete of `k = 4294967296` written under that schema; `k` dropped; the delete committed; then `expireSnapshots().retainLast(2).cleanExpiredMetadata(true)`, which leaves schema 0 (`k INT`) and schema 2 (no `k`). `SELECT id` through FileScannerV2 returned 2, 3 before this PR and returns 1, 2, 3 with it. branch-4.1 and branch-4.2 have the same FileScannerV2 code. ### Release note Fix FileScannerV2 comparing Iceberg equality-delete keys in a single scan-wide type, which could delete or keep the wrong rows, or fail the scan, after a key's type changed (for example a schema-only promotion removed by `cleanExpiredMetadata`, or a key whose retained schemas only have it as `unknown`). ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - `run-be-ut.sh --run --filter='IcebergV2ReaderTest.*:*EqualityDelete*:*ColumnMapper*:*EvolutionCast*:*TableReader*'`: 526 of 526 pass on macOS arm64 (ASAN), including the 14 new cases. - Rebased onto master `43f9ceb48d7`: #69639 moved the strict-mode override of `EvolutionCast` from `prepare()` into `configure_cast_options()`, so the new `strict` flag now sets `CastOptions::strict_mode` there, next to the existing DATE -> TIMESTAMP_NS rule. The run above is on the content before the rebase (the third-round commit applies to both unchanged); the 67 TUs that this PR changes or that master changed since `5caaeb3e3d4` were syntax-checked against the rebased tree with the UT build flags, and CI runs the suites again. - [x] Manual test (add detailed scripts or steps below) - The local end-to-end check described under Results, with an ASAN BE built from this change. - [ ] No need to test or manual test. Explain why: - Behavior changed: - [ ] No. - [x] Yes. FileScannerV2 compares equality-delete keys per delete file and data file; two key types that convert in neither direction now fail the scan; the comparison casts are strict. - Does this need documentation? - [x] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
CAST expressions currently construct temporary Blocks and execute through the generic function framework. Bind column executors during expression preparation, capture conversion settings in
CastOptions, and passRuntimeStateduring execution. Migrate scalar, complex and Variant conversions to this interface while retaining aFunctionCastcompatibility adapter for direct function callers.Release note
None
Check List (For Author)
BUILD_TYPE_UT=ASAN ./run-be-ut.sh --run --filter='*Cast*' -j48: 395 passed, 1 skipped, 4 disabled; includes EvolutionCast overflow/clone and nested MAP deduplication coverage.