Repository navigation
[improvement](be) Reduce transitive dependencies in shared headers - #69717
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
There was a problem hiding this comment.
No actionable introduced defects found at head 3ee2057df441d6e165e290fd11d4b4fe34157dc2. Reviewed all 71 changed files and relevant callers, implementations, ownership paths and tests. Two scope reviewers and a separate exchange/runtime-filter risk reviewer each returned NO_NEW_VALUABLE_FINDINGS; the main review independently verified their conclusions. The review converged in one round, with no unresolved candidates or unreviewed changed paths.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and supporting evidence | The changes reduce transitive dependencies through extracted contracts, forward declarations and relocated definitions. The text-only build-hygiene gates pass; compile-time savings were not independently measured. |
| Scope and clarity | The changes stay focused on header dependencies, equivalent definition/initializer moves and direct use-site includes. |
| Concurrency and locks | Exchange queue/dependency signaling, runtime-filter RPC coordination, cancellation and lock order retain their existing behavior. No new critical section or blocking operation is introduced. |
| Lifecycle and static initialization | BlockItem explicitly retains its move operations and passive destruction. Weak callback owners, incomplete-type ownership and TabletManager cache initialization remain valid. No new cross-TU static initialization dependency was found. |
| Configuration | No configuration item or dynamic-update behavior changes. |
| Compatibility and core correctness | Numeric protobuf bodies, UID bit representations, compression choices and ColumnIteratorOptions layout are preserved. Visibility/version, delete-bitmap and error-propagation logic is unchanged. Relocated definitions remain in existing linked targets. |
| Thrift/protobuf definitions | No gensrc field IDs, enum values or requiredness changes. |
| Parallel paths | Checked local/remote exchange, callback success/error/cancellation, specialized numeric/IP/UUID SerDes, cloud/local consumers, scanner v1/v2 and standalone target dependencies. No missing parallel update found. |
| Conditional checks | No new behavioral branch is introduced; existing checks and their explanations are retained. Qualified std::abs handles the same bounded signed subsecond counts. |
| Test coverage | Inspected existing protobuf round-trip, UID, callback, exchange and affected module tests, including error/cancellation cases. Test changes add direct includes or qualify the same RowRanges type. No new functional behavior requires a new expected result. |
| Test results and fixtures | No result fixtures or behavioral test expectations change. No test was executed in this review. |
| Observability | Existing logs, counters and profiling are preserved; no new execution path requires additional instrumentation. |
| Transactions and persistence | No EditLog, persistence, version-publication or failover protocol changes. |
| Writes, atomicity and crashes | No transaction/data-write algorithm changes; inspected ownership and cleanup relocations preserve their prior behavior. |
| FE-to-BE variables | No transmitted variable or request-field changes. |
| Performance and memory | No additional per-value decoding operation, copy, allocation or scan is introduced. Memory ownership/accounting is preserved. Some definitions become out of line; runtime performance was not measured, and no concrete runtime regression was established. |
| Other dependency obligations | All 18 numeric SerDe class instantiations remain paired with extern declarations. Header layering/budgets/PCH and unity-skip checks pass. No additional actionable issue found. |
There were no existing inline review threads or confirmed existing P0/P1 findings to carry forward. No additional user focus was supplied.
Validation: ran only bash build-support/check-build-hygiene.sh and static source/diff/include-graph analysis. No builds, test execution, benchmarks, source edits or submodule initialization. Actual compilation/linking, runtime behavior, release ELF compatibility and the PR's reported performance measurements were not independently verified under this static-only contract.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28905 ms |
TPC-DS: Total hot run time: 151634 ms |
ClickBench: Total hot run time: 24.58 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
3ee2057 to
4d2d1e4
Compare
|
run buildall |
TPC-H: Total hot run time: 28710 ms |
TPC-DS: Total hot run time: 151921 ms |
ClickBench: Total hot run time: 24.26 s |
…leanup ### What problem does this PR solve? Related PR: apache#69717 Problem Summary: The include cleanup reduced transitive header dependencies, leaving several budgets above the measured reach. Update the forward closure budget and reverse reach baselines to preserve these reductions, with room for subsequent changes. Retain the existing 10% reverse reach allowance and gate behavior. Set the FrontendService_types.h, data.pb.h, storage/options.h and PaloInternalService_types.h baselines to 350, 120, 350 and 1150 respectively. ### Release note None ### Check List (For Author) - Test: build-support/check-build-hygiene.sh; build-support/tests/run.sh; git diff --check - Behavior changed: Yes, header dependency budget values are updated. - Does this need documentation: No
### What problem does this PR solve? Problem Summary: Shared BE headers propagate protobuf, Thrift, cache, thread-pool and storage-reader definitions into unrelated translation units, increasing repeated parsing and header rebuild scope. Introduce dedicated ColumnIteratorOptions and SyncSizeCallback headers, use forward declarations where complete types are unnecessary, and move protobuf conversions, number SerDe definitions and lifecycle operations into their implementation files. Add direct includes at actual use sites and repair independent compilation of affected production and test sources. Preserve object layouts, move semantics, explicit template instantiations and per-value decoding implementations. Across the 16 monitored heavy headers, measured header-to-TU dependency pairs decrease from 8,108 to 6,617 (18.39%), affecting 742 distinct TUs. RLE reach decreases from 537 to 154 TUs and threadpool reach from 607 to 361. The dependency.h project-header closure decreases from 325 to 246. Header budgets and exception lists remain unchanged. Release builds with Clang 21.1.8, -j50, PCH/Unity/ccache disabled measured 636.4 seconds before, 569.0 seconds after and 636.2 seconds on repetition. Concurrent machine load prevents claiming a stable full-build speedup. Median production compiler peak RSS changes from 563.5 MiB to 551.5 MiB and 551.6 MiB; production compilation remains at 1,531 translation units. ### Release note None ### Check List (For Author) - Test: Three complete Release builds; standard ASAN BE/FE build and final incremental build; all BE UT sources compiled and linked; 29 selected unit tests passed with -j50 --coverage; 1,024-row service read, aggregate, shuffle join, sort and cast checks; header hygiene and changed-line format checks passed. Clang-tidy covered all 71 files and retained two existing warnings in relocated protobuf methods (function size and signedness). - Behavior changed: No - Does this need documentation: No
…leanup ### What problem does this PR solve? Related PR: apache#69717 Problem Summary: The include cleanup reduced transitive header dependencies, leaving several budgets above the measured reach. Update the forward closure budget and reverse reach baselines to preserve these reductions, with room for subsequent changes. Retain the existing 10% reverse reach allowance and gate behavior. Set the FrontendService_types.h, data.pb.h, storage/options.h and PaloInternalService_types.h baselines to 350, 120, 350 and 1150 respectively. ### Release note None ### Check List (For Author) - Test: build-support/check-build-hygiene.sh; build-support/tests/run.sh; git diff --check - Behavior changed: Yes, header dependency budget values are updated. - Does this need documentation: No
e930737 to
dc2a6a3
Compare
|
/review |
There was a problem hiding this comment.
Found two P1 build regressions at dc2a6a3dd587865ae137d5cc5a9e85bb2359b7b0: the S3 reader test loses the complete ObjClientHolder definition, and cache reload callers lose CachePath and parse_conf_cache_paths. Both need direct includes at their actual use sites. These are separate dependency cuts and are reported inline. No existing P0/P1 findings apply; the supplied threads and live inline-comment readback were empty.
Reviewed all 69 changed paths in the authoritative aggregate diff, including changed tests, related callers, and local/remote and cloud/shared-nothing paths. No additional user focus was supplied.
| Required checkpoint | Conclusion |
|---|---|
| Goal and evidence | Header dependencies decrease and the tightened static budgets pass. The two missed consumers prevent the claimed complete independent-compilation cleanup. Existing behavioral tests were inspected, not executed. |
| Scope and clarity | The changes remain focused on dependency removal, declaration extraction, moved definitions, and matching use-site includes. |
| Concurrency and locks | Queue ingress, query dequeue, cancellation/close, and RPC completion retain their existing locks and ordering. No new thread entry, shared-state mutation, or blocking-under-lock operation was introduced. |
| Lifecycle and static initialization | BlockItem retains explicit defaulted moves, member initializers, and deferred callback cleanup; callback/query weak ownership is preserved. Tablet stat-cache construction remains before the sole constructor body. No new cross-TU static initialization dependency was found. |
| Configuration | No runtime configuration added. The changed build-gate thresholds remain static tables with the existing enforcement logic. |
| Symbols, layout, and rolling compatibility | Internal definitions move into already-linked source files; all 18 numeric SerDe extern/explicit families remain paired. Extracted options/callback bodies and protobuf implementations are identical. No wire or storage layout change was found; binary compatibility was not validated from built artifacts. |
| Thrift/protobuf field IDs | No gensrc/thrift or gensrc/proto definitions changed. |
| Parallel paths | Local/remote exchange, cloud/local storage, numeric/date/IP/UUID SerDes, and direct header consumers were traced. The two reported omissions are in dependent consumers. |
| Conditions and error handling | Compression selection and callback/error branches are unchanged relocations. std::abs qualification retains bounded signed subsecond handling. No new silent-failure branch was found. |
| Functional and negative coverage | Inspected protobuf round trips, UID conversions, exchange delayed-callback/decode-error/cancellation tests, runtime-filter failure/expired-owner tests, and changed reader tests. No tests were run. Missing includes must be fixed before affected test targets compile. |
| Test results | No regression output or expected-value files changed; test edits add includes or qualify the same RowRanges type. |
| Observability | Existing counters, timers, logs, and error propagation remain unchanged; no new runtime path needs instrumentation. |
| Transactions and persistence | No commit/replay, rowset-version, delete-bitmap, or persisted-metadata algorithm changed. |
| Data writes and atomicity | No new write algorithm or transaction boundary; cache admission and publication behavior are unchanged by the header cuts. |
| FE/BE variables | No new cross-process variable or sending path. |
| Performance | No added per-value decoding work, copy, or allocation was found. Moving internal methods out of line changes inlining opportunities; runtime performance and PR benchmark claims were not independently validated. |
| Other risks | The header gate's own static graph and seeded self-test source were checked; no additional gate defect was found. |
Read-only static analysis passed: 50 header-layering rules, 10 third-party bans, all 16 forward/16 reverse budgets and PCH whitelist; 298 extern declarations across 50 template families; and all four test-included source files' unity exclusions. dependency.h has 248 project headers in its closure, matching the new limit. Gate injection self-tests were inspected only because they mutate source.
This review is static-only: no builds, unit/regression tests, benchmarks, cluster runs, clang-tidy runs, or release-artifact/glibc ABI audit were performed. No repository source files were changed.
All assigned reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2. All candidates and initial risk items have explicit dispositions, with no unresolved static question or uncovered changed file.
Review converged after 2 round(s).
### What problem does this PR solve? Related PR: apache#69717 Problem Summary: Header dependency reductions left the S3 reader test and file-cache consumers without complete type definitions needed for object construction and member access. Include s3_file_system.h directly in the S3 reader test, storage/options.h in the file-cache HTTP action and microbench, and the rate limiter and FDCache definitions in the microbench. This restores independent compilation of the BE_TEST and enabled microbench paths. ### Release note None ### Check List (For Author) - Test: Standard ASAN BE/FE build; BE UT target build and 10 S3FileReaderTest/FileCacheActionTest cases with -j50 --coverage; independent compilation without PCH or Unity; build-hygiene; changed-line clang-format; clang-tidy; git diff --check. Coverage rendering reported 20 functions with mismatched profile data. - Behavior changed: No - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
No new actionable findings in the 72 changed paths at 09c0f1a. The two previous P1 findings are fixed: 4442228286 by the S3 test's direct holder-definition include, and 4442228578 by direct storage-options includes in both cache callers. Neither remains blocking; neither is reposted.
Critical checkpoint conclusions:
- Goal and evidence: dependency isolation is implemented through declaration-only interfaces and moved definitions. Static comparison confirms unchanged numeric protobuf methods, callback, iterator-options structure and compression selector; existing relevant tests were inspected.
- Scope and reuse: the changes remain focused on dependency reduction, direct consumer includes and budget updates, reusing existing implementations.
- Concurrency: exchange task/receiver threads and RPC callbacks retain their existing queue lock, dependency signaling and callback-release paths. Tablet-stat snapshot/publication still uses
_tablet_stat_cache_mutex; no new lock work or lock ordering is introduced. - Lifecycle and memory: BlockItem retains defaulted noexcept moves and out-of-line destruction; runtime-filter callback/query/wrapper ownership remains weak where required. Cache initialization preserves member order. No new allocation lifetime, accounting, COW/null-handling or cross-TU static-initialization change was found.
- Configuration: no configuration item is added or changed; dynamic behavior is unaffected.
- Compatibility: protobuf bodies, layouts and conversions are preserved; all 18 numeric SerDe extern/explicit instantiation families pair. Moved functions remain in their owning build targets. No new platform API or library dependency is introduced. Compile/link and final-artifact ABI validation were outside this static review.
- Thrift/protobuf field IDs: no definitions under gensrc are changed; generated-header dependencies and forward declarations were inspected.
- Parallel paths: production, unit-test, cloud/local and optional tool consumers were inspected, including the previously failing BE_TEST and microbench callers.
- Conditional behavior: no new runtime condition is introduced. Relocated failure/status branches retain the existing error propagation; std::abs uses the standard integer overload for bounded subsecond values.
- Test coverage: existing UID, SerDe, exchange cleanup, runtime-filter callback and reader tests were inspected. This refactor adds dependency fixes and namespace qualifications to tests; no new functional test behavior is claimed.
- Expected results: no regression result file or expected output changes; native decoder ranges/assertions are preserved.
- Observability: logging, metrics, profile counters and error context retain their existing behavior; no new operational path requires instrumentation.
- Transactions and persistence: no transaction, replay, visible-version, delete-bitmap or persisted-format behavior changes.
- Writes and crash handling: data-write algorithms, atomicity boundaries, cleanup and recovery are unchanged by these definition/include moves.
- FE/BE variables: no new transmitted variable or sending path is introduced.
- Performance: dependency isolation supports the compile-cost goal, and budget changes are explicitly explained in commit dc2a6a3. No added per-value work or allocation was found; runtime effects of out-of-line calls were not measured here.
- Other risks: downstream complete-type requirements, generated namespaces, constructor/destructor visibility, PCH/unity independence and budget/self-test assumptions were reviewed.
The read-only header-dependency checker passed all 50 layering rules, 10 third-party bans, 16 forward and 16 reverse budgets, and the PCH whitelist. No builds, unit/regression tests, benchmarks or mutating hygiene self-tests were run. No additional user focus was supplied. The review converged in round 1: all three reviewers returned NO_NEW_VALUABLE_FINDINGS, all changed paths and risk items are covered, and no unresolved candidate remains within this static scope.
Review converged after 1 round(s).
|
run buildall |
TPC-H: Total hot run time: 28813 ms |
TPC-DS: Total hot run time: 152070 ms |
ClickBench: Total hot run time: 24.37 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? Issue Number: close apache#69884 Related PR: apache#69904 Problem Summary: Merging master 7acfd9b left two Delta gaps. First, apache#69717 replaced table_reader.h's full ShardedKVCache definition with a forward declaration, but deltalake_reader_test.cpp constructs a ShardedKVCache and relied on the transitive include. Include format/format_common.h directly, as hudi_reader_test.cpp does. Second, apache#69300 made Parquet readers in one scan share their profile counters through TableReader's parquet profile cache. Hudi, Iceberg and the base reader pass that cache, but DeltaReader still built its ParquetReader without it, so every Delta file created its own set of counters. Pass the cache, and keep binary-UUID preservation disabled as before. ### Release note None ### Check List (For Author) - Test: No local build. The merge was conflict-free. Both changes follow the matching master code. COMPILE and BE UT are left to CI. - Behavior changed: No. Delta scan profiles reuse Parquet counters across files, as other formats do. - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#69884 Related PR: apache#69904 Problem Summary: Merging master 7acfd9b left two Delta gaps. First, apache#69717 replaced table_reader.h's full ShardedKVCache definition with a forward declaration, but deltalake_reader_test.cpp constructs a ShardedKVCache and relied on the transitive include. Include format/format_common.h directly, as hudi_reader_test.cpp does. Second, apache#69300 made Parquet readers in one scan share their profile counters through TableReader's parquet profile cache. Hudi, Iceberg and the base reader pass that cache, but DeltaReader still built its ParquetReader without it, so every Delta file created its own set of counters. Pass the cache, and keep binary-UUID preservation disabled as before. ### Release note None ### Check List (For Author) - Test: No local build. The merge was conflict-free. Both changes follow the matching master code. COMPILE and BE UT are left to CI. - Behavior changed: No. Delta scan profiles reuse Parquet counters across files, as other formats do. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: #65730 #66608 #66805 #66841 #66999 #67005 #67011 #67110 #67268 #67289 #67325 #67650 #67921 #68028 #68335 #68375 #68615 #68690 #68813 #68815 #68707 #68649 Problem Summary: **In short.** This ports the Lance BE read path of branch-4.1 / branch-4.2 to master, in its branch-4.2 form as of `cabcf45c1af`, plus the BE side of three 4.x changes merged after that snapshot: the search filter `prefilter` flag and profile layout (#68813), distance bounds for `vector_search` (#68815), and the split-level index rule of #68707 that #68815 relies on. It gathers the BE parts of the PRs listed above. Master links `liblance_c` but has no Lance reader, so every `FORMAT_LANCE` scan fails today. With this PR the BE reads Lance datasets through FileScannerV2: plain scans, vector search and full-text search, predicate and runtime-filter pushdown, the schema fetch of a local Lance TVF, and the second phase of TopN lazy materialization. The PR is BE only. The FE connector plugin and the regression suites come in separate PRs, so until then nothing on master plans a Lance scan, and the BE unit tests carry the proof. Original authors: @zhangstar333 @Gabriel39 @wangzhaobo957-cloud @Jay-ju @FANNG1 **Background.** Lance shipped in 4.1.4: the Lance catalog, `local()` / `s3()` with `format=lance`, and the `vector_search` / `full_text_search` TVFs. On master the FE side is being rebuilt as a connector plugin (`fe-connector-lance`), and the BE needs the reader from 4.x. The thrift ids are the branch-4.1 / 4.2 ones, including `TExternalSearchRequest.5` (`prefilter`) and `TVectorSearchParams.6/7` (distance bounds) added after the snapshot, so a BE built from master also reads the Lance scan ranges and search requests that a 4.x FE sends during a rolling upgrade. The one exception is `TVectorSearchOptions` 6, branch-4.2's `approx_mode` (#69852, merged into branch-4.2 after the snapshot and not ported): it is reserved in a comment, and a master BE ignores it, so during a rolling upgrade a 4.2 FE that asks for `fast` or `accurate` gets Lance's default (`normal`). **The problem, and what it cost.** | | master today | with this PR | |---|---|---| | A `FORMAT_LANCE` scan | FileScannerV2 has no Lance reader, so the scan fails with "FileScannerV2 does not support file format" | read by `LanceTableReader` in FileScannerV2 | | `fetch_table_schema` for a local Lance TVF | not handled | returns the Arrow schema mapped to Doris types; Null leaves keep `NULL_TYPE` | | The row location of a Lance row in TopN lazy materialization | `GlobalRowLoacationV2` only has a 32-bit row id, but a Lance row id is a 64-bit id within the snapshot | a version 1 layout carries the 64-bit row id; phase two reads the rows with the dataset's `take_rows` | **How this PR fixes it.** - Reader: `be/src/format_v2/lance/*` and `be/src/format_v2/table/lance_reader.*`, as on branch-4.2 except one local renamed for master's `-Wshadow`. The unit tests construct `RuntimeState(TQueryOptions(), globals)`, because master removed the globals-only constructor. - FileScannerV2: the reader dispatch and `_to_file_format` handle `FORMAT_LANCE` / `"lance"`; `FileFormat::LANCE` is added to its two exhaustive switches. Since #69723 every file scan uses FileScannerV2, so nothing else routes Lance scans (on branch-4.2 a Lance query fails with NotSupported when `enable_file_scanner_v2` is off). - Row locations: `ROW_VERSION {FILE_LOCAL_ROW_ID = 0, LANCE_DATASET_ROW_ID = 1}` and a union layout for `GlobalRowLoacationV2`. Version 0 keeps its byte layout, which static asserts pin. `PRequestBlockDesc.row_id` becomes `uint64` (a proto2 varint, so the wire format is compatible). The materialization operator decodes row locations by version, rejects an unknown version or a wrong size, and stamps `row_location_version`. `RowIdStorageReader` validates the counts and the version, range-checks internal row ids against `uint32`, and groups Lance rows by plan node, snapshot version and dataset URI, so each dataset is read with one `take_rows`. Master's segment batching, scan-slot dedup and fetch concurrency stay as they are. - Thrift (`PlanNodes.thrift`): the external search request body (`TVectorElementType`, `TVectorMetric`, `TSearchVector`, `TVectorSearchParams` 1-7, `TFtsCoverageMode`, `TFtsQueryType`, `TFtsMatchOperator`, `TFullTextSearchParams`, `TSearchFilterFormat`, `TSearchFilter`, `TVectorSearchOptions`, `TExternalSearchQuery`), `TExternalSearchRequest` 2-5 and `TLanceFileDesc` 4-6, all with the branch-4.1 / 4.2 ids. The enums spell out their 4.x values. No id is reused. On master, #70075 reserved `TExternalSearchRequest` 2-5 in comments and added 6 (`paimon_options`); this PR replaces those comments with the fields and leaves 6 as it is. `TVectorSearchOptions` 6 is reserved in a comment for branch-4.2's `approx_mode` (#69852). - After the snapshot (one commit each): - #68813: `prefilter` decides whether the TVF search filter runs before (true) or after (unset / false) candidate selection, for vector and full-text search. The profile groups the Lance counters under `LanceTiming`, `LanceIO`, `LanceIndex`, `LanceVectorIndex`, `LanceScalarIndex`, `LanceExecutionPlan`, `LanceScan`, `LanceSearchPlan` and `LanceDataCache`, uses the 4.x counter names, and registers every scan metric before the statistics callback is installed, so the callback, which Lance invokes from its execution streams, never adds counters. #68813's FE tweak that hides `TUnit.NONE` labels in `AggCounter` belongs to the FE port. - #68707 (part): a vector split searches exactly the index segments FE planned for it, and a split without segments is searched flat whatever `use_index` says; a split with segments under `use_index = false` is rejected. 4.x FEs plan segments only when `use_index` allows them. - #68815: `distance_lower_bound` (inclusive) and `distance_upper_bound` (exclusive) are validated (finite FLOAT values, lower < upper at FLOAT precision, single-vector only) and installed with `lance_scanner_set_distance_range()`, which master's pinned lance-c provides. - Review fixes: - A nested array goes to the Doris SerDes without a copy only when it and every descendant start at offset 0 and reference exactly their child ranges. Lance's legacy (v1) decoder can return a struct or list that is tight at its own level over a Boolean child with a bit offset (a batch that starts mid-byte in the bitmap), and the SerDes reject that under Arrow input validation. - A zero-field `STRUCT<>`, which Lance decodes, is reported as an unsupported column at any nesting level, like the other unsupported Lance types. It used to be typed as a STRUCT that `ColumnStruct` cannot hold (its row count comes from its first child). - A Lance Decimal256 field with precision <= 38 is narrowed to Decimal128 by a safe Arrow cast before decoding. Doris types it DECIMAL32/64/128I by precision, and those SerDes read only Decimal128 arrays; the BE used to crash on the first value, also for nested leaves. Only values a reader can see are cast: before a nested leaf is narrowed, every child slot that no visible row references (under a NULL list, fixed-size-list, map or struct row, or outside a sliced parent) is marked NULL, so a hidden value does not fail the cast and a visible value outside the declared precision still does; map keys, which cannot be NULL, stay checked. - FLOAT / DOUBLE `>=` runtime filters stay residual-only. Lance compares floats by totalOrder, so a pushed `x >= c` dropped negative-NaN rows that Doris keeps. - Runtime filters that mention INT64_MIN stay residual-only. The pinned Lance SQL parser reads `-9223372036854775808` as the negation of a magnitude that it parses as i64 first; the overflow turns the literal into a Float64, which the planner cannot coerce to Int64 or Decimal, so a pushed IN, `>=` or `<=` filter on an INT64 or scale-zero decimal column failed planning and the query failed. Every other i64 is still pushed. - The shared Arrow DATE32 decoding (`DataTypeDateV2SerDe::read_column_from_arrow`) rejects days outside 0000-01-01..9999-12-31 instead of returning a non-null `0000-00-00`, skips null slots and adds the epoch in 64 bits: the DATE32 range part of branch-4.1 / 4.2 #68649. It also changes ADBC, Arrow stream load and paimon-rust reads. - Child values hidden by a NULL list, fixed-size-list, map or struct row are not decoded: the array, map and struct SerDes give the element SerDe a validity bitmap that also clears those slots (`mask_arrow_children_hidden_by_null_parents()`). Arrow leaves them unspecified, and Lance's v2.1 miniblock encoding keeps them, so a hidden 10000-01-01 under a NULL fixed-size list used to fail the DATE32 range check above. Visible values are unchanged; this applies to ADBC, Arrow stream load and paimon-rust as well. The masks come from ExecEnv's tracked Arrow pool. A nested Lance array that is compacted before decoding is masked the same way, at every level, before its `ValidateFull()`, so a hidden invalid UTF-8 string, out-of-precision decimal, Date64 or time no longer fails the batch while a visible one still does. Since #69697 the same masks keep hidden DATETIMEV2 / TIMESTAMPTZ values out of its range checks. A mask costs at most one pass per batch: a NULL row that owns no child slots, or only NULL ones, allocates nothing, and Arrow stream load masks each retained IPC batch once (`mask_arrow_descendants_not_visible()`) before it hands row ranges to the SerDes, so its per-block reads allocate nothing. The one per-block mask left is for map keys under a NULL map row that still owns entries, since a MapArray cannot hold NULL keys; Arrow's map builder writes NULL map rows without entries. - Core: `to_protobuf` keeps `NULL_TYPE` for a NULL literal type, so a Null leaf in ARRAY / MAP / STRUCT no longer comes back as BOOLEAN. The nullable serde reads an Arrow Null array as NULLs without touching buffers that the array does not have. - Configs, caches and metrics, with the 4.x names and defaults: | Config | Default | |---|---| | `lance_index_cache_size_bytes` | 10 GiB | | `lance_metadata_cache_size_bytes` | 1 GiB | | `enable_lance_data_cache` | true | | `lance_data_cache_path` | `${DORIS_HOME}/lance_data_cache` | | `lance_data_cache_disk_capacity_bytes` | 100 GiB | | `lance_data_cache_read_block_size_bytes` | 1 MiB | | `lance_io_buffer_size_bytes` (mutable) | 2 GiB | | `lance_batch_readahead` (mutable) | 5 | | `lance_fragment_readahead` (mutable) | 5 | The BE-wide Lance session (index cache, metadata cache and the optional Foyer disk cache) is created by the first Lance scan, so a BE that never reads Lance creates no cache directory. The first six configs take effect after a restart. A negative cache size (and, while the data cache is enabled, a negative disk capacity or read block size) fails the first Lance scan with a message that names the setting, as does any data-cache value lance-c rejects; the rest of the BE keeps serving. Ten `lance_session_*` metrics report the two in-memory caches. With the defaults, the disk tier keeps 51,200 sparse 2 MiB block files open from the first Lance scan on (100 GiB in blocks of twice the read block size), the same as on branch-4.1 / 4.2. Size `lance_data_cache_disk_capacity_bytes` and `lance_data_cache_read_block_size_bytes` against the BE's open-file limit, or set `enable_lance_data_cache = false`. The index and metadata caches live in lance-c memory: they are not charged to a MemTracker or registered with CacheManager, so memory GC, the cache capacity adjustment under memory pressure and `/api/clear_cache/all` do not shrink them, and `enable_lance_data_cache = false` does not turn them off. lance-c has no call to clear, evict or resize them, so size them with the two configs. The disk tier keys blocks by Lance's store prefix (`scheme$bucket` for S3, GCS and OSS, without the endpoint) and the object path (`thirdparty/patches/lance-c-foyer.patch`). Several endpoints that serve the same bucket name and data-file path with different bytes (data files registered under chosen names or replaced in place; Lance's writers use random file names) share entries, also across a BE restart; such a deployment should set `enable_lance_data_cache = false`. Both are as on branch-4.1 / 4.2 and tracked as follow-ups. A Lance scan's native working set, that is the I/O buffered up to `lance_io_buffer_size_bytes` per scanner, the decode readahead and the operators of a search plan such as the top-k of a flat vector search, is allocated inside lance-c and is charged to neither the query nor its workload group. It counts toward the BE process memory only, so memory GC, which ranks queries by tracked memory, does not attribute it to the Lance query. lance-c accepts no memory budget, so on BEs with little memory headroom size `lance_io_buffer_size_bytes`, `lance_batch_readahead` and `lance_fragment_readahead` for the number of concurrent Lance scanners. As on branch-4.1 / 4.2; tracked as a follow-up. - Build: macOS links `-framework IOKit`. This is the first time objects of `liblance_c.a` are pulled into `doris_be`, so the binary and the link time grow. With this PR `util/rle_encoding.h` reaches 231 TUs and `util/threadpool.h` 589, the same as master `315c2e00331` alone (limits 275 and 660 since #69717), and `gen_cpp/internal_service.pb.h` 562 (master alone 559, limit 660); the reach budgets are unchanged. Not included: the Lance index metadata scanner (`LANCE_INDEX_ENTRIES`); the FE side, including the rolling-upgrade guard that keeps Lance lazy materialization off while an old BE could decode a version 1 row location as version 0; the regression suites; the rest of #68707 (searching historical versions, tags and branches, rejecting fragment ids missing from the snapshot, naming the snapshot in errors and the profile). The `InitReaderAvgTime` / `GetBlockAvgTime` profile fix that #67005 carried goes in a separate PR. **Results.** | | master | with this PR | |---|---|---| | A `FORMAT_LANCE` query | fails before reading anything | read by `LanceTableReader` in FileScannerV2 | | A Lance row in the second phase of TopN lazy materialization | cannot be represented | a version 1 row location, read back with `take_rows` | | A row location of an unknown version or size in phase two | decoded as version 0 | rejected with an error | | An Arrow DATE32 day outside 0000-01-01..9999-12-31 (ADBC, Arrow stream load, paimon-rust, Lance) | read as a non-null `0000-00-00` | the read fails with `InvalidArgument` | BE unit tests (ASAN, macOS arm64, against master's pinned lance-c), on the head rebased onto master `378404eeb12`: 1029 cases in 54 suites run, 1027 pass and 2 Parquet microbenchmarks skip by design. They cover the Lance suites (107: `LanceTableReader*` 83, `LanceNestedNullTest` 10, `LanceRuntimeFilterHelperTest` 10, `LanceSessionManagerTest` 4); the row-location protocol (`MaterializationSharedStateTest`, `RowIdStorageReaderTest`, and the new `RowIdFetchRequestValidationTest`: count mismatch, unknown version, a Lance version on an internal mapping, an internal row id of 2^32); `FileScannerV2Test` (28); the ORC and Parquet reader suites (`NewOrcReaderTest`, `NewParquetReaderTest`, `ParquetScanTest` and the other `Parquet*` suites), whose row ids now go through `file_local`; and the Arrow / protobuf / DATE serde suites with the new `ArrowParentNullsTest`, plus the Iceberg and Parquet reader suites that decode Arrow dates. Reverting any of the review fixes, the #68813 flag, the #68707 split rule or the #68815 bounds makes its new tests fail. The sixth review round adds 13 cases (`ArrowParentNullsTest` 10, `LanceTableReaderTypeTest` 3) and extends one; with them the Arrow SerDe, Lance, ADBC and `IcebergV2ReaderTest` suites run 317 cases: 310 pass and the 7 ADBC SQLite cases skip without a local SQLite driver. The ninth review round adds `LanceRuntimeFilterHelperTest.KeepsInt64MinFiltersResidualOnly` and `LanceTableReaderFilterTest.KeepsInt64MinRuntimeFiltersOutOfLanceScanner`: both pass with the 106 Lance cases (`Lance*`) on the branch rebased onto `339e087b11f`, and restoring `parse_int64()` in either check makes exactly these two fail. On the head rebased onto master `315c2e00331`, with the per-block mask fix (`42cf4e3e5d1`), 328 cases in 24 suites pass: the Arrow SerDe and validation suites, `ArrowParentNullsTest` (15), `LoadArrowPipeTest` (47), the DATETIMEV2 / TIMESTAMPTZ SerDe suites, `ArrowColumnToDorisColumnTest`, `ArrowBlockConvertorTest`, `AdbcReaderTest`, `RemoteDorisV2ReaderTest`, `PaimonRustTableReaderTest` and the Lance suites. Allocating at the first NULL row of each call again fails the new `ArrowParentNullsTest` case and the four new `LoadArrowPipeTest` block cases, dropping the per-batch mask fails the LIST and STRUCT block cases, and returning the child unmasked fails the new timestamp case, four `ArrowParentNullsTest` cases and `LanceTableReaderTypeTest.IgnoresDate32PayloadHiddenByNullParents`. ### Release note Add nine `lance_*` BE configs and `lance_session_*` BE metrics. They take effect once the FE can plan Lance scans. Reading an Arrow DATE32 value outside 0000-01-01..9999-12-31 through ADBC, Arrow stream load or paimon-rust now fails instead of returning 0000-00-00. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - `run-be-ut.sh --run --filter=...` with the suites listed under Results: 420 of 420 pass on macOS arm64 (ASAN) on the content before the rebases, and the Lance and Arrow SerDe suites pass again with the nested-offset, zero-field struct, hidden-child, hidden-Decimal256 and compaction-mask fixes (the ADBC SQLite cases skip without a local SQLite driver). Rebased onto master `43f9ceb48d7`: #69668 conflicted in the scanner selection and in `fetch_table_schema`, which now hands every file format to `format::fetch_file_schema()`, so the Lance branch runs ahead of that call; #69646 added a TopN row-group test that reads the version 0 row location fields, which now go through `file_local` (the earlier rebase onto `5caaeb3e3d4` resolved #69614 and #69601 in the scanner selection, the `FileFormat` enum, its test and the reach budget table). The 95 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. Rebased again onto master `8d88f1cc6f7`: the only conflict was the budget table, where #65858 moved the `util/rle_encoding.h` baseline to 828, and the 163 TUs that this PR changes or that master changed since `43f9ceb48d7` pass the same syntax check. Rebased again onto master `339e087b11f`: the only conflict was the budget table again, where #67292 moved the `util/rle_encoding.h` baseline to 834; #69872 added Arrow type checks ahead of the NULL-parent masking in the map and struct readers and merged cleanly. Of the 114 sources that this PR changes or that master changed since `8d88f1cc6f7`, 111 pass the same syntax check. The other three are master's: two new #67292 files under `storage/segment` fail the same way on master alone, since they compile only inside their unity batch, and this PR adds no source to that target; the #69818 benchmark is not in the UT build and includes only `util/simd` headers that this PR does not touch. Reverting any of the review fixes, the #68813 flag, the #68707 split rule or the #68815 bounds makes its new tests fail. Rebased again onto master `378404eeb12`, which removed the legacy file scanner (#69723): the conflicts were in the scanner-selection code and tests that #69723 deleted, so this PR's Lance lines there went with them (including its `LanceQueriesAlwaysUseScannerV2` test), in the profile keys of `materialization_opertor.cpp`, and in the row fetch of `rowid_fetcher.cpp`, whose non-Lance path is now only `FileScannerV2::read_by_rows`; the deleted V1 ORC / Parquet row id tests took this PR's `file_local` rename with them, and the `util/rle_encoding.h` budget commit is dropped because master is no longer near that limit. Of the 100 sources that this PR changes or that master changed since `339e087b11f`, 99 pass the same syntax check; the other, `format_v2/orc/orc_reader.cpp`, fails the same way on master alone. On the rebased head the suites listed under Results run 1029 cases: 1027 pass and 2 microbenchmarks skip. The test-utility fix that `DataTypeDateTimeV2SerDeTest` needed on macOS (#69695) is now on master. Rebased again onto master `315c2e00331`: the only conflict was `TExternalSearchRequest` in `PlanNodes.thrift`, where #70075 had reserved fields 2-5 in comments; the two commits that add the fields replace those comments, and field 6 stays #70075's `paimon_options`. #69717 cut transitive includes from shared headers: of the 160 sources that this PR changes or that master changed since `378404eeb12`, 156 pass the same syntax check; the two `jemalloc_hook` tests have no compile command on macOS, `runtime/memory/jemalloc_hook.cpp` uses glibc aliases that macOS cannot build (#69819), and `format_v2/table_reader.cpp` alone fails the same way on master alone while its unity batch compiles. The full `doris_be_test` builds and links on the rebased head; `ArrowParentNulls*` and `Lance*` (121 cases) pass before the mask fix, and the 328 cases above with it. - `build-support/check-build-hygiene.sh` (on this branch rebased onto master `315c2e00331`), clang-format 16 and `build-support/run-clang-tidy.sh` on the changed files: all pass; on the changed lines clang-tidy (llvm 22) reports only `readability-inconsistent-ifelse-braces` on the `DORIS_CHECK` lines of the masking functions, the macro's expansion that master's existing `DORIS_CHECK`s report too. - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - Behavior changed: - [ ] No. - [x] Yes. `PRequestBlockDesc.row_id` is `uint64`; phase two of TopN lazy materialization rejects row locations of an unknown version or size; `FORMAT_LANCE` scans run on FileScannerV2; an Arrow DATE32 day outside the Doris DATE range fails the read instead of becoming `0000-00-00`. - Does this need documentation? - [x] No. Lance is documented with the FE connector. - [ ] 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: zhangstar333 <[email protected]> Co-authored-by: Gabriel <[email protected]> Co-authored-by: zhaobo wang <[email protected]> Co-authored-by: jay <[email protected]> Co-authored-by: FANNG <[email protected]> Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]> Co-authored-by: zy-kkk <[email protected]>
Related PR: #66901
What problem does this PR solve?
Several shared BE headers transitively include protobuf, Thrift, thread-pool, and storage-reader implementations. Translation units that only need declarations must still parse these definitions, increasing compilation work and the rebuild scope when those headers change.
Recent CI builds failed during CMake configuration because the reverse-reach counts for threadpool.h and rle_encoding.h exceeded the build-hygiene limits. This PR audits all 32 header-budget entries and reduces unnecessary dependencies without changing the budgets or exception lists.
The changes:
ColumnIteratorOptionsinto a dedicated header sovexpr_context.hdoes not require the fullcolumn_reader.hdependency chain.thread-pool,protobuf,cache, andWorkloadGrouptypes where complete definitions are unnecessary.UniqueIdconversions,protobufmethods in numericSerDe, andselected construction/destructionoperations into.cppfiles.SyncSizeCallbackinto a dedicated header to reduce RPC dependencies propagated through the runtime-filter interface.Object layouts, move semantics, explicit template instantiations, and per-value decoding implementations are preserved. Moving some definitions into .cpp files changes their visibility for inlining in ordinary separate compilation. Query runtime performance has not been measured in this PR.
Dependency reductions
The following counts come from Ninja dependencies after completed builds, covering 1,531 production translation units and 52 generated translation units:
Across the 16 monitored heavy headers, measured header-to-TU dependency pairs decrease from 8,108 to 6,617, an 18.39% reduction. These removals affect 742 distinct translation units. The number of production compilation units remains unchanged.
The transitive project-header closure of dependency.h decreases from 325 to 246 headers, a 24.31% reduction. This uses the hygiene script’s static include graph and is reported separately from the measured Ninja dependency counts.
Query runtime performance
TPC-H SF1000
SSB SF1000
ClickBench
Configuration: cluster_type=single, ecs.c9i.4xlarge_16c32g, one 500 GB disk, full analyze, PR #69717 release package.
TPC-DS SF1000 (1 TB)
Comparisons use the PR result from 2026-10-29. The 2026-10-28 rerun is listed separately.
Release note
None
Check List (For Author)