[feature](function) Support map arguments for inner_product - #67311
[feature](function) Support map arguments for inner_product#67311BiteTheDDDDt wants to merge 2 commits into
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Extend inner_product to compute sparse vector dot products represented as MAP<K, FLOAT>. Map keys are limited to integral and string types and use native typed dispatch with direct key access and hashing, without serialization or type erasure. Dense ARRAY<FLOAT> behavior remains unchanged.
### Release note
Support inner_product for MAP<K, FLOAT> arguments with integral or string keys.
### Check List (For Author)
- Test:
- Regression test: test_map_inner_product
- Unit Test: FunctionMapInnerProductTest.* (6 tests, ASAN)
- Build: ./build.sh --be and ./build.sh --fe
- Behavior changed: Yes. inner_product now accepts compatible MAP arguments.
- Does this need documentation: No.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes — the review is complete and converged in Round 2. Three blocking issues remain. No additional user-specific focus was supplied.
Critical checkpoint conclusions:
- Goal and scope: the focused six-file change adds sparse MAP support while preserving dense ARRAY behavior. Happy-path BE and regression coverage supports the intended arithmetic, but the implementation does not yet handle all accepted key/coercion paths.
- Correctness and branch coverage: const/nullable wrappers, empty maps, deduplicated keys, NULL-key matching, smaller-side build symmetry, unsupported keys, and dense delegation were traced. The two inline FE findings identify distinct reachable failures before and after coercion.
- Memory and performance: the algorithm is O(m+n) per row and builds the smaller input, but its user-sized temporary hash allocation bypasses Doris MemTracker accounting; see the BE inline finding. No other material performance issue was substantiated.
- Concurrency and lifecycle: execution state is invocation-local and no shared mutable state, locking, static initialization, ownership cycle, or exceptional teardown issue is introduced.
- Configuration and compatibility: no new setting, protocol, or storage format is added, and dense behavior remains compatible. However, the existing legacy type-coercion setting exposes an FE/BE key-type mismatch described inline.
- Tests and observability: the added expected results are deterministic and arithmetically consistent, but coverage misses the untyped-NULL and legacy mixed-family reproducers. This review runner intentionally did not build or execute tests. Scalar-function errors are surfaced explicitly; no new logging or metrics path is needed.
- Transactions, persistence, and distributed state: none are involved. FE/BE behavior was reviewed end to end for signature resolution, implicit casts, physical key dispatch, nullability, and error propagation.
- Style and maintainability: the change generally follows adjacent function/test patterns and reuses the dense base implementation; the three inline fixes are required before approval.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed the authoritative bundle at head 8f23259 (base 2689e0d). FE coercion/legality, BE registration/array compatibility, allocator tracking, and the added test wiring were rechecked. The existing NULL-key, mixed-family, and allocator threads are duplicate-fenced or fixed on this head. One distinct BE correctness issue is called out inline below.
| } | ||
| continue; | ||
| } | ||
| const auto it = values_by_key.find(KeyTraits::get_key(probe.keys, i)); |
There was a problem hiding this comment.
[P1] Preserve last-wins semantics for duplicate map keys on both sides. The build loop overwrites repeated keys, but the probe loop (including NULL keys) adds every occurrence; with a raw row {1:2, 1:3} against {1:4}, choosing the smaller side to build returns 20 instead of the map's last-wins result 12, and swapping row sizes changes the answer. Legacy VMapLiteral and native/old Parquet/ORC readers can expose duplicate entries without calling deduplicate_keys. Please normalize/track final occurrences on both rows before matching and add coverage for ordinary and NULL duplicates under both build-side choices.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
inner_productcurrently only accepts denseARRAY<FLOAT>vectors. This PR extends it to sparse vectors represented asMAP<K, FLOAT>, where matching map keys identify dimensions.Map keys are intentionally limited to integral and string types. The BE dispatches to concrete native key types and hashes keys directly: numeric keys use their native column data, while string keys use zero-copy
StringRefaccess. The implementation does not serialize keys or use runtime type erasure. For each row, it builds a flat hash map from the smaller input map and probes it with the larger map, using O(m + n) time and O(min(m, n)) temporary space. Existing dense array behavior remains unchanged.The implementation also validates unsupported key types in FE and BE, preserves NULL-key matching, and rejects NULL map values and NULL outer maps.
Release note
Support
inner_product(MAP<K, FLOAT>, MAP<K, FLOAT>)for integral and string key types.Check List (For Author)
test_map_inner_productFunctionMapInnerProductTest.*(6 tests under ASAN)Additional validation:
DISABLE_BE_CDC_CLIENT=ON ./build.sh --beDISABLE_BUILD_UI=ON ./build.sh --febuild-support/check-build-hygiene.shbuild-support/check-format.shBehavior changed:
inner_productnow accepts compatible MAP arguments in addition to ARRAY arguments.Does this need documentation?
Check List (For Reviewer who merge this PR)