Repository navigation
[fix](function) Fix HLL downsample constant column access - #69799
Merged
hello-stephen merged 2 commits intoOct 29, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69257, apache#69679 Problem Summary: The constant-expression API migration missed two HLL downsample calls, breaking the BE build. Invoke ConstantExprResult::column_ptr() when reading the constant arguments. ### Release note None ### Check List (For Author) - Test: Local ASAN BE build passed (`./build.sh --be -j 48`). - Behavior changed: No - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No new PR-introduced defect found at c5a9b97. Reviewed the complete authoritative diff: both constant-column accesses in FunctionDataSketchesHllDownsample::open now call the existing ConstantExprResult::column_ptr() accessor. No existing inline findings require revalidation, and no additional user focus was supplied.
Critical checkpoint conclusions:
- Goal/proof: the two calls now match the declared API and pass const IColumn references to the parsers. Source inspection supports the intended compile fix; no build was performed for this review.
- Scope: two focused accessor corrections in one file.
- Concurrency: no new shared mutation, threads or locks. Fragment state remains read-only during execution and shared through context clones.
- Lifecycle/static initialization: the constant wrapper owns a non-null, normalized single-row column; parsing consumes it immediately and stores scalar values. No new ownership cycle or static initialization dependency.
- Configuration: no settings or dynamic-update behavior changed.
- Compatibility: SQL signatures, return types, sketch serialization, storage formats and dependency/toolchain inputs are unchanged; no new libc API dependency is introduced. No binary ABI audit was run.
- Thrift/protobuf: no definitions changed; cross-branch field-ID checks are inapplicable.
- Parallel paths: both lg_k and optional tgt_type calls are migrated. Default HLL_4, execute fallback and analogous accessor users remain consistent; FE/BE type and nullability contracts are unchanged.
- Conditions: no conditional branch changed. Existing constant-availability guards and parser errors remain effective.
- Test coverage: inspected existing regression cases for default/explicit types, folded/unfolded/BE-evaluated constants, nullable sketches, string/binary inputs and invalid parameters. Existing unit source also covers open/fallback and nullable invalid parameters, subject to the limitation below. No tests were run.
- Test results: no test or expected-output file changed in this PR; no runtime result is claimed.
- Observability/errors: no additional instrumentation needed for these accessor corrections; RETURN_IF_ERROR continues to propagate validation failures.
- Transactions/persistence: no EditLog, replay, failover or persistence changes.
- Writes/atomicity: no data-write or transaction changes; no new crash-cleanup obligation.
- FE/BE transport: no new transmitted variables or sending paths.
- Performance: the accessor returns a const reference; no added column copy, allocation, materialization or per-row work.
- Other invariants/BE obligations: nullable constant handling and fallback remain valid; committed-data visibility, version and delete-bitmap logic are unaffected. No header/include/PCH/template/unity/gate-table changes.
Validation limit: the existing BE unit helper at be/test/exprs/function/function_datasketches_hll_test.cpp:217,224 still refers to the removed ColumnPtrWrapper type. These exact lines and the replacement ConstantExprResult API already coexist at the merge base, so this is a pre-existing test migration gap, not a new inline finding against this two-line patch. Unit-test source coverage does not establish that the target compiles or passes. This review was static only: no builds, tests, benchmarks or runtime validation.
Converged after 1 round: the complete-review and separate constant-API risk reviewers both returned NO_NEW_VALUABLE_FINDINGS; main independently verified the evidence, dismissed the ownership/shape/lifecycle/fallback risk, and deduplicated the pre-existing test limitation. Final sweep covered the full changed-file list and all dispositions; no unresolved candidates or uncovered static-review scope remain.
Review converged after 1 round(s).
OIiveirra
added a commit
to OIiveirra/doris
that referenced
this pull request
Oct 29, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69032, apache#69683, apache#69799 Problem Summary: Upstream partition batching and the spatial SPI extension both used connector API 25.0. Merge official master while retaining both interface surfaces, advance the spatial extension to 26.0, and update its surface-test pin. Preserve both spatial constant-folding protection and upstream HLL exclusions. Apply the two approved HLL constant-result getter calls equivalent to apache#69799; do not change HLL algorithms or its tests. Spatial functionality and ordinary-key spatial-value support remain unchanged. ### Release note None ### Check List (For Author) - Test: Standard ASAN BE+FE build and Checkstyle; selected FE UT 219/219 (including SPI 153/153); GIS, Iceberg and VARBINARY native regressions; API26 resource and ten connector manifests; format16, hygiene and changed-line clang-tidy. BE UT compilation is blocked before execution by unchanged upstream HLL test references to the removed ColumnPtrWrapper; no passing BE UT is claimed. - Behavior changed: No. Preserve both parent features and distinguish the spatial SPI major version from upstream partition batching. - Does this need documentation: No. Internal merge/API reconciliation; public spatial contracts in apache/doris-website#4239 remain unchanged.
Contributor
TPC-H: Total hot run time: 28608 ms |
Contributor
TPC-DS: Total hot run time: 151688 ms |
Contributor
ClickBench: Total hot run time: 24.26 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#69799 Problem Summary: The HLL downsample unit test still constructs the removed ColumnPtrWrapper after the constant-expression API migration. Use ConstantExprResult to match FunctionContext::set_constant_cols and fix the two undeclared-identifier errors in BE UT CI build 1085144. ### Release note None ### Check List (For Author) - Test: clang-format 16 dry-run, git diff --check, and build hygiene passed. Local compilation and tests skipped as requested; CI compilation pending. - Behavior changed: No - Does this need documentation: No
Contributor
|
run compile |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
skip buildall |
hello-stephen
approved these changes
Oct 29, 2026
Contributor
|
Codex automated review failed and did not complete. Error: Review recovery stopped: PR base/head or open state changed; refusing to resume stale context Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
11 tasks done
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
OIiveirra
added a commit
to OIiveirra/doris
that referenced
this pull request
Oct 29, 2026
### What problem does this PR solve? Related PR: apache#69032, apache#69799 Problem Summary: The spatial branch inherited HLL unit tests using the removed ColumnPtrWrapper. Merge official master containing apache#69799 so focused spatial verification can use the standard BE UT entry point. The merge is conflict-free and matches the independently simulated tree; no manual product changes are added. ### Release note None ### Check List (For Author) - Test: Manual merge-tree and staged-tree comparison; standard builds and focused tests follow in the spatial fix. - Behavior changed: No additional task-specific behavior change - Does this need documentation: No
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?
Issue Number: None
Related PR: #69257, #69679
Problem Summary: The constant-expression API migration in #69679 missed two calls introduced by #69257, breaking the BE build. Call
ConstantExprResult::column_ptr()when reading the HLL downsample constant arguments.Release note
None
Check List (For Author)
./build.sh --be -j 48).