Repository navigation
[feature](function) Add H3 cell and coordinate functions - #69018
Merged
Merged
Conversation
### What problem does this PR solve? Problem Summary: Add h3_lat_lng_to_cell, h3_cell_to_lat_lng, and h3_cell_to_boundary using official H3 4.5.0. Register the functions in Nereids and build H3 as a static dependency with sanitizer support. Coordinates use degrees in latitude/longitude order; invalid inputs return NULL. Center coordinates contain two elements, and boundaries preserve H3 vertex order without repeating the first vertex. Use the default NULL execution framework and allow constant folding through BE evaluation. ### Release note Add H3 functions for mapping coordinates to cells and retrieving cell centers and boundaries. ### Check List (For Author) - Test: ASAN BE build, clean FE build, three FunctionH3Test unit tests, generated and rerun test_h3 regression, and clang-tidy checks passed. Compared 10,000 inputs per function against H3 4.5.0 and ClickHouse 26.3.9.8; cell IDs matched and coordinate differences met tolerance. - Behavior changed: Yes, add three SQL functions. - Does this need documentation: Yes, SQL function documentation is needed.
### What problem does this PR solve? Problem Summary: Include the upstream H3 4.5.0 attribution and NOTICE for the newly linked static library in Doris distribution notices. ### Release note None ### Check List (For Author) - Test: Compared the attribution and NOTICE against contrib/h3; git diff --check passed. - Behavior changed: No. - Does this need documentation: No.
Mryange
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
October 13, 2026 09:34
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.
Request changes for one confirmed P1: the new mandatory H3 target is not initialized by the standalone BE unit-test entry point. On a fresh checkout/worktree, run-be-ut.sh reaches the H3 wrapper with contrib/h3 still empty and cannot configure any BE unit tests. The inline comment identifies the fix.
Reviewed commit c9a406b. Review converged after 2 rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS in the latest round, all 18 changed paths were covered, and every candidate/risk was independently accepted, deduplicated or dismissed with evidence. There are no unresolved static candidates or uncovered changed paths. Existing inline threads were absent; no existing blocking comment IDs apply. No additional user focus was supplied.
Invocation chain reviewed:
FE signatures/registration -> generic translation or BE constant folding
-> geo/H3 factory registration -> default constant/NULL dispatch
-> ColumnView -> official H3 C API -> nullable results -> array consumers/storage
Critical checkpoint conclusions:
- Goal and evidence: the three H3 functions use degree-based latitude/longitude, NULL for invalid typed inputs, two-coordinate centers and unclosed variable-length boundaries. Implementation and inspected tests agree; standalone UT setup is incomplete as reported inline.
- Scope and reuse: focused changes reuse existing FE scalar/signature/visitor conventions, IFunction null/constant handling and ColumnView; no unrelated implementation change identified.
- Concurrency and locks: stateless wrappers, immutable H3 tables and caller-owned results; existing factory call_once handles registration. No new shared writes, locks or lock-order issue.
- Lifecycle and initialization: constexpr names, compile-time C tables and RAII/COW column ownership introduce no dynamic cross-TU initialization, ownership cycle or cleanup obligation.
- Configuration: no new configuration item or dynamic propagation requirement.
- Compatibility: additive builtins and standard BIGINT/ARRAY types; both FE and BE must support the new functions. Existing storage/wire layouts are unchanged. Static/PIC linkage and existing glibc policy were inspected; completed artifacts were not audited.
- Thrift/protobuf IDs: no gensrc definition changes, so cross-release field-ID checks do not apply.
- Parallel paths: ordinary vectors, mixed/all constants, nullable/only-NULL/empty inputs, FE/BE folding and nested array consumers were traced. Normal build.sh initializes H3; standalone run-be-ut.sh is the reported omission. Cloud/shared-nothing share the scalar path.
- Conditions and errors: finite/domain/cell checks handle concrete invalid inputs, and all H3 statuses affect NULL results. Ordinary cast overflow checks prevent wide integer wraparound. Intrinsic and input NULL maps merge correctly.
- Tests: inspected BE and regression coverage for all 16 resolutions, geographic bounds, NaN/Inf, invalid cells/modes, constants, NULLs, empty/multiple batches, pentagons, storage, array access/map/filter/explode and grouping. No tests were executed.
- Expected results: read all 36 golden blocks/269 rows; independently checked resolution bits, row/NULL/group counts, repeated folding outputs and array consistency. No static discrepancy found. Exact floating-point results and output-generation provenance were not independently executed.
- Observability: existing scalar/query/configuration error paths suffice; no new distributed operation or state calls for extra logs/metrics.
- Transactions and persistence: no visible-version, MoW bitmap, transaction, EditLog or replay path changes.
- Writes and crash recovery: production changes compute result columns and add no write/atomicity/crash-recovery protocol; tests exercise existing array storage.
- FE-BE values: generic builtin names/types cover runtime and folding. FE AlwaysNullable and nullable array elements match BE declarations; no new context variable or wire field.
- Performance, memory and failures: bounded per-row geometry with at most 10 vertices, tracked column storage and fixed stack outputs; constants evaluate once, input columns remain shared safely, and allocation exceptions use existing Status conversion. No additional concrete regression found.
- Other integration checks: pinned v4.5.0 agrees with the gitlink; upstream source list/header generation, include/link propagation, sanitizer flags and license/NOTICE were inspected. Documentation remains an acknowledged author follow-up without a documentation PR.
Validation limits: static review only, as required. No builds, tests, source modifications or submodule initialization were performed. Author-reported execution/CI comparisons are not this review's test evidence; runtime sanitizer behavior, other architectures and final ELF/shipped-library glibc versions remain unverified.
Review converged after 2 round(s).
### What problem does this PR solve? Problem Summary: The standalone BE unit-test script did not initialize the newly required H3 submodule. A fresh checkout could fail during CMake configuration when the H3 wrapper reads VERSION. Add the same H3 initialization used by build.sh before configuring unit tests. ### Release note None ### Check List (For Author) - Test: bash -n run-be-ut.sh and git diff --check passed; initialization matches build.sh. Fresh-checkout build validation was not run. - Behavior changed: No SQL behavior change; fix standalone unit-test setup. - Does this need documentation: No.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed PR #69018 at head 35724e3 using the supplied aggregate diff and all 19 changed paths. No new verified code defect was found. The existing P1 comment 4257310464 is fixed on this head: run-be-ut.sh:263 now initializes contrib/h3 before CMake, matching build.sh:961. It is neither reposted nor retained as an existing blocker.
Critical checkpoint conclusions:
- Goal and evidence: the three degree-based H3 functions are registered through FE binding, existing scalar translation and BE geo registration. Cell conversion returns nullable BIGINT; center and boundary return nullable arrays with the expected nullable element layers. Added unit/regression tests cover the intended behavior, but this review did not execute them.
- Scope and reuse: changes are focused on H3 dependency integration, three scalar expressions, one stateless BE implementation and tests. Default IFunction constants/NULL handling and ColumnView are reused.
- Concurrency and locks: no new threads, locks or shared execution state. Registration remains under the existing factory call_once; output columns and H3 conversion temporaries are local to execution.
- Lifecycle/static initialization: constexpr names, ordinary function/context lifetime and local owned columns introduce no cross-translation-unit initializer dependency, owner cycle or custom cleanup.
- Configuration: no new runtime configuration.
- Compatibility: existing scalar/array types and serialization are reused; no existing function semantics or persisted format changes. These new function names require BEs that implement them during deployment.
- Thrift/protobuf IDs: no gensrc changes.
- Parallel paths: ordinary/selected block execution and BE constant folding use the same implementation; constant, nullable, only-NULL/Nothing and empty-column behavior follows the existing framework. Both BE build entry points initialize H3.
- Conditional/error handling: finite coordinate/geographic/resolution checks and official cell validation handle actual invalid inputs. H3 return codes are checked; invalid input produces the advertised SQL NULL. Repeated offsets preserve invalid rows, and boundaries preserve upstream vertex order without closing the ring.
- Test coverage: new BE tests exercise non-finite/out-of-domain inputs, constant combinations, Nothing, interleaved valid/invalid/NULL rows, empty blocks and 5-/10-vertex pentagons. Regressions cover both folding settings, all 16 resolutions, poles/dateline, nullable/non-nullable columns, nested storage/access/explode/composition, grouping/filtering and mixed batches. These are inspected coverage, not passing-test claims.
- Changed results: all 342 golden-file lines were reviewed. Static structure/arithmetic checks found 36 query blocks, 128 all-resolution rows with consistent encoded resolutions, matching stored/direct array results and consistent aggregate counts. Numerical coordinate equivalence and result generation were not independently executed.
- Observability: existing scalar error/NULL behavior is suitable; no new distributed operation or state transition requires logs/metrics.
- Transactions/persistence: no EditLog, transaction, visible-version or delete-bitmap mechanism changes.
- Writes/atomicity: production changes only compute scalar results; existing array columns/storage receive valid nested layouts. No new write protocol or crash-recovery mechanism.
- FE-BE parameters: names, fixed argument families, AlwaysNullable semantics and both array element-nullability levels agree. No new transmitted variable; generic translation and recursive folding serialization apply.
- Performance/memory: work is bounded per row by resolution 15 and at most 10 boundary vertices; all-constant input executes once. Column buffers use existing tracked allocators; reachable H3 APIs use stack temporaries/read-only tables. No repeated shared-input mutation or substantiated material performance regression found.
- Other integration checks: the wrapper lists all 18 pinned upstream core C sources, generates the matching versioned header, enables PIC and inherits sanitizer flags with warning relaxation limited to H3. GitHub tag v4.5.0 resolves to the gitlink revision. Licensing/NOTICE attribution matches the pinned upstream. BE header/gate-table/PCH/extern-template/unity-inclusion obligations introduce no additional issue. The author states SQL documentation is needed and has not linked a documentation PR.
User focus: no additional focus was supplied. The existing inline concern was independently checked and is addressed as described above.
Convergence: true after round 1. All three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS; full assigned scope and both initial risk focuses were covered, independently verified and disposed with evidence. No unresolved candidate or static coverage gap remains.
Validation limits: static review only. No source edits, builds, Doris tests, benchmark, sanitizer runtime checks, ARM/macOS execution or final-ELF glibc 2.17 audit were performed. Source inspection found no specific new ABI incompatibility; it cannot certify a completed binary. Author-reported validation and supplied goldens were treated as context, not executed evidence.
Review converged after 1 round(s).
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29388 ms |
Contributor
TPC-DS: Total hot run time: 152237 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
HappenLee
reviewed
Oct 14, 2026
### What problem does this PR solve? Problem Summary: H3 coordinate functions grow output buffers repeatedly for large batches. Reserve the row offsets and the center point value capacity before processing rows. ### Release note None ### Check List (For Author) - Test: ASAN BE build and targeted clang-tidy passed; clang-format 16 and git diff --check passed. - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29606 ms |
Contributor
TPC-DS: Total hot run time: 153223 ms |
Contributor
ClickBench: Total hot run time: 24.59 s |
Contributor
TPC-H: Total hot run time: 29319 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 151962 ms |
Contributor
ClickBench: Total hot run time: 24.59 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE Regression Coverage ReportIncrement line coverage |
HappenLee
approved these changes
Oct 19, 2026
HappenLee
left a comment
HappenLee
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the current head. The earlier standalone UT initialization and output-capacity reservation concerns are addressed. No blocking implementation issue found in the reviewed changes.
HappenLee
dismissed
github-actions[bot]’s stale review
October 19, 2026 09:35
The requested H3 initialization fix is implemented in run-be-ut.sh. The same reviewer confirmed this in the subsequent review 5502341526; the current head d720235 has been independently rechecked and approved in review 5569536561.
starocean999
approved these changes
Oct 19, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
3 of 16 tasks
morningman
added a commit
that referenced
this pull request
Oct 19, 2026
…enum class (#69297) ### What problem does this PR solve? Issue Number: None Related PR: #69018, #69174 Problem Summary: **In short.** BE UT fails with `BE BUILD FAIL` for every PR against master: `be/test/exprs/function/function_h3_test.cpp`, added by #69018, no longer compiles since #69174 made `FunctionContext::FunctionStateScope` an `enum class`. This PR spells the 8 affected calls with the scoped enumerators, so the test compiles again. Nothing else changes. **Background.** #69174 (merged 2026-10-18) turned `FunctionContext::FunctionStateScope` into an `enum class` and rewrote every caller from `FunctionContext::FRAGMENT_LOCAL` to `FunctionContext::FunctionStateScope::FRAGMENT_LOCAL`, and the same for `THREAD_LOCAL`. #69018 (merged 2026-10-19) added an H3 function test written against the old, unscoped names. The two PRs were merged independently; together they leave a test on master that does not compile. The BE UT pipeline merges each PR into the latest master before it builds `doris_be_test`, so the break hits every PR, whatever it changes. **The problem, and what it cost.** Since #69261 fixed the other test break of the day (left by #68447), every BE UT build for a PR against master stops at this file, for example [build 1073803](http://43.132.222.7:8111/viewLog.html?buildId=1073803) (#69257, merged into master `41a58afba2e`): ``` FAILED: [code=1] test/CMakeFiles/doris_be_test.dir/exprs/function/function_h3_test.cpp.o ../test/exprs/function/function_h3_test.cpp:66:62: error: no member named 'FRAGMENT_LOCAL' in 'doris::FunctionContext' ../test/exprs/function/function_h3_test.cpp:67:62: error: no member named 'THREAD_LOCAL' in 'doris::FunctionContext' ... 8 errors generated. ninja: build stopped: subcommand failed. ``` No BE unit test runs for those PRs, so their BE UT result says nothing about the PR until this is fixed. **How this PR fixes it.** The 8 `open` / `close` calls in `function_h3_test.cpp` now use `FunctionContext::FunctionStateScope::FRAGMENT_LOCAL` / `THREAD_LOCAL`, as #69174 did for the other function tests. The test logic is unchanged. **Results.** `-fsyntax-only` with the flags of each file's own entry in `be/ut_build_ASAN/compile_commands.json` (clang 22, `-DBE_TEST`): | File | master `41a58afba2e` | this PR | |---|---|---| | `function_h3_test.cpp` | the 8 errors CI reports | compiles | | the 4 test files #69261 fixed (`data_type_serde_arrow_test.cpp`, `data_type_variant_v2_serde_jsonb_test.cpp`, `variant_v2_execution_test.cpp`, `vparquet_writer_test.cpp`) | compile | compile (not touched) | | `function_array_zip_test.cpp` (baseline, not touched) | compiles | compiles |
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: N/A
Problem Summary: Add
h3_lat_lng_to_cell,h3_cell_to_lat_lng, andh3_cell_to_boundaryusing official H3 4.5.0. Coordinates use degrees in latitude/longitude order. Invalid inputs return NULL; centers contain two coordinates and boundaries preserve H3 vertex order without closing the ring. Input NULL propagation uses the default execution framework. FE has no separate Java H3 evaluator; constant folding through BE remains supported.Build H3 as a static C library from its 18 upstream core sources. Generate its versioned API header in the build directory, export include paths through the target, enable PIC, and inherit Doris sanitizer flags. Only the H3 target relaxes third-party warning errors. The wrapper avoids upstream standalone tools and global build options. Include the upstream license attribution and NOTICE.
Release note
Add three H3 SQL functions for mapping coordinates to cells and retrieving cell centers and boundaries.
Check List (For Author)
Validation on Linux x86_64:
run-be-ut.sh --run --filter=FunctionH3Test.* -j48: all three tests passed.run-regression-test.sh --run -d query_p0/sql_functions/spatial_functions -s test_h3: generated results with-forceGenOut, inspected them, and reran successfully. Includes constant-folding modes, nullable/non-nullable table columns, all 16 resolutions, stored nested arrays, array expansion, filtering/grouping, and 35,000 mixed rows.Check List (For Reviewer who merge this PR)