[Feature](lambda) Support some map lambda functions - #66968
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
5c42734 to
2130e12
Compare
|
/review |
There was a problem hiding this comment.
Automated review status: complete and converged after three rounds. Requesting changes for three P1 correctness issues.
Findings
- The FE materialization rule moves sensitive Map inputs from Join candidate-pair scope into a child-row Project, changing outer-join error behavior and volatile evaluation frequency.
- The same rule moves computed Maps out of selector-controlled IF/CASE branches, defeating supported short-circuit execution.
- The BE eight-batch direct path bypasses the variable-width lambda safety ceiling and can turn a bounded query into a multi-gigabyte ColumnString overflow.
Checkpoint conclusions
- Goal and scope: the advertised Map lambda wrappers, constructors, tuple syntax, recursive NULL-type handling, and FE-to-BE lowerings are present. The scalar wrappers and registrations are localized; the 786-line late materialization rule and the unrelated ArrayMap batching expansion are the principal risk surfaces.
- Planner semantics and parallel paths: Project, OneRowRelation, Filter, Having, normalized Aggregate, Generate, Join metadata, nested lambda ExprIds, marker translation, and second-pass stability were reviewed. Outside the two inline evaluation-domain failures, no additional owner or parallel path defect survived.
- Types, nullability, compatibility, and physical symbols: lambda arity, Map-key legality, nullable Maps/predicates, nested NULL_TYPE merging, last-win semantics, and the names
map_from_arrays,%map_from_arrays_unique%,map_filter, andmap_from_entriesalign across FE and BE. No serialized format or existing function signature changes. - Runtime correctness and ownership: constant/nullable wrappers, offsets, selected rows, invalid entries, duplicate keys, COW detachment, and Status propagation were checked. Those paths remain sound outside the accepted lambda budget issue.
- Performance and memory: the direct-path multiplier is a correctness failure as well as an allocation regression; no other distinct performance defect survived review.
- Concurrency, lifecycle, configuration, persistence, transactions, writes, and observability: no new applicable mechanism is introduced.
- Tests: the changed unit and regression sources broadly cover ordinary, null, constant, duplicate, empty, nested, aggregate, Join, Generate, and selected-row behavior, but omit the three accepted boundaries. Per the automated-review contract, I did not run builds or tests; current style, license, formatting, and secret checks pass, while build/test jobs are skipped.
- User focus: no additional focus was supplied.
No pre-existing live review thread or comment duplicated these findings.
|
run buildall |
TPC-H: Total hot run time: 17554 ms |
TPC-DS: Total hot run time: 84739 ms |
ClickBench: Total hot run time: 14.67 s |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
1872ace to
9651dc4
Compare
|
/review |
There was a problem hiding this comment.
Automated review status: complete and converged after three rounds. Requesting changes for one new P1 correctness issue.
Finding
- Native-table access-path pruning can lose Map values used only inside a nested Lambda and substitute default/null values; one inline comment is attached.
- The existing P1 variable-width Lambda budget thread remains applicable and was not duplicated.
Critical checkpoint conclusions
- Goal and proof: the PR adds Map Lambda functions, Map constructors/filtering, tuple Lambda syntax, and FE-to-BE lowering. Registrations and ordinary semantics align, and unit/regression sources cover null, empty, duplicate, constant, computed, nondeterministic, nested, aggregate, Join, and Generate cases; the accepted native stored-column boundary is not covered.
- Scope and focus: the implementation is cohesive for the advertised feature and reuses ArrayMap/MapEntries/Map primitives. No additional user focus was supplied.
- Concurrency: execution uses query-local expression/column state; no new shared mutable state, locks, lock ordering, or thread-safety mechanism is introduced.
- Lifecycle/static initialization: Lambda frames use scoped guards and ColumnMap sharing uses COW; no non-intuitive release path, circular ownership, or cross-TU static initialization dependency was found.
- Configuration: no configuration item or dynamic-update path is added.
- Compatibility: FE wrapper names, arities, return/nullability types, and internal physical symbols match BE registration. No storage format, persisted metadata, or protocol variable changes are introduced; the new function symbols are additive.
- Parallel paths: public Map functions, internal unique/filtered constructors, legacy two-argument
map_filter, constant folding, computed inputs, nested Lambdas, and native/external scan paths were checked. The native scan divergence is the attached finding. - Conditional checks: const broadcasting, top-level null Maps/arrays, hidden null-row payloads, mismatched offsets, nullable predicates, and last-win deduplication checks have explicit failure/skip behavior and targeted tests. No separate conditional-check defect survived.
- Test coverage: BE unit tests and FE/regression tests are broad, including negative cases, but need a stored native-table test where the outer Map Lambda retains the key and reads the value only inside an inner Lambda.
- Test results: the committed
.outresults match the asserted ordinary semantics. Per the review-run contract, I did not run builds or tests. - Observability: these are scalar analysis/execution paths; existing AnalysisException/Status failures are sufficient, and no new log or metric is required.
- Transactions/persistence: no transaction, EditLog, master-failover, or persistent-state path is changed.
- Data writes/crashes: the feature is query-time only and does not modify stored data. COW detachment and Status propagation are sound outside the accepted read-pruning correctness issue.
- FE-BE variables: no new session/protocol variable is transmitted. FE and BE independently computed types/names were checked and agree.
- Performance: zero-copy/COW paths are reasonable, but the existing live byte-budget P1 remains unresolved; no distinct additional CPU/memory issue survived duplicate fencing.
- Other issues: the scalar-subquery tuple-parser candidate was rejected because the semantic predicate only gates, rather than forces, its alternative. No other unresolved candidate remains.
|
run buildall |
TPC-H: Total hot run time: 17112 ms |
TPC-DS: Total hot run time: 82668 ms |
ClickBench: Total hot run time: 14.61 s |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
7b2a020 to
4a6f18e
Compare
|
/review |
There was a problem hiding this comment.
Automated review status: complete and converged after two rounds. Requesting changes for two new P1 issues.
Findings
- The new key-preserving/removing Map paths assume a carrier-wide unique-key invariant that external and serialized Map readers do not enforce, so duplicate-key Maps can bypass Doris's last-win normalization.
- The regression expects
map_from_arrays([1], [[]])to fail even though this PR's signature normalization and FE unit test explicitly accept it asMAP<TINYINT,ARRAY<TINYINT>>.
The existing variable-width Lambda batching and nested-Lambda Map access-path P1 threads remain applicable and were not duplicated.
Critical checkpoint conclusions
- Goal and proof: the PR adds the advertised Map Lambda functions, constructors/filtering, tuple Lambda syntax, and FE-to-BE lowering. Ordinary semantics are broadly tested, but the two attached boundaries keep the implementation from meeting the goal end to end.
- Scope and focus: the implementation is generally cohesive and reuses ArrayMap, MapEntries, and existing Map constructors; the unrelated ArrayMap fast-path expansion remains the principal out-of-scope risk already covered by a live thread.
- Concurrency: execution and analysis use query-local expression/column state; no new shared mutable state, locks, lock ordering, atomics, or thread entry is introduced.
- Lifecycle/static initialization: scoped Lambda frames and COW columns were traced; no non-intuitive release path, circular ownership, or cross-TU static initialization dependency was found.
- Configuration: no configuration item or dynamic-update path is added.
- Compatibility: public/internal function names, arities, return types, nullable shapes, and FE/BE registrations agree. The symbols are additive and no storage format or persisted protocol changes; cross-carrier Map semantics fail only at the first attached finding.
- Parallel paths: direct and Lambda
map_filter,map_apply,transform_keys,transform_values, public/internal constructors, native/external readers, constant folding, nested Lambdas, and access-path pruning were compared. M1 is the surviving divergence. - Conditional checks, errors, nullability, and memory: const/nullable materialization, empty/null Maps, null predicates, offset checks, selector ordering, COW ownership, and Status propagation are sound outside M1. The existing 8x variable-width budget thread remains the memory blocker.
- Tests: BE unit, FE unit, parser, access-path, and regression sources cover normal, null, empty, constant, duplicate-producing, nested, volatile, aggregate, Join, Generate, and invalid-input paths. They omit external duplicate-key provenance, and M2 is an internally contradictory expected error.
- Test results: committed
.outlabels and ordinary results were checked, but the M2 error block is incorrect. Per the review contract, no builds or tests were run. - Observability: these are scalar analysis/execution paths with existing AnalysisException/Status reporting; no new log or metric is required.
- Transactions/persistence and data writes: no EditLog, transaction, failover, stored-data mutation, or crash-recovery path is changed.
- FE-BE variables: no session/protocol variable is added; all new physical function names and types match across FE and BE.
- Performance: entry rewrites are linear and zero-copy/COW use is reasonable, but the existing variable-width fast-path issue remains unresolved; the unsafe uniqueness optimization is M1.
- Other issues: the tuple-parser scalar-subquery candidate was rejected because its semantic predicate enables but does not force the nonviable tuple alternative. No other unresolved nonduplicate candidate remains.
- User focus: no additional focus was supplied.
All 26 changed files were swept explicitly; Round 2 normal FE, normal BE, and separate cross-layer risk reviews each returned NO_NEW_VALUABLE_FINDINGS.
|
run buildall |
TPC-H: Total hot run time: 17348 ms |
TPC-DS: Total hot run time: 85102 ms |
ClickBench: Total hot run time: 14.97 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Automated review status: complete and converged after three rounds. Requesting changes for five new issues.
New inline findings
- Residual nested
NullTypefields can reach wrapper validation before the established recursiveTINYINTfallback, rejecting valid Map-Lambda expressions. - Lambda
map_filterintroduces its internal constructor after coercion, allowing its physical entry fields to disagree with the normalized FE result type. - The shared
MapEntries/ArrayMaplowering evaluates physical entries hidden beneath semantically null Map rows, exposing lambda errors or side effects. - Both new Map filtering paths narrow legal 64-bit nested positions into a 32-bit selector and can copy the wrong entry above 2^32.
MapFilter,TransformValues,TransformKeys, andMapApplymerge independent Map/Struct decimal or time precisions before execution or lowering.
Previously raised blockers still present (not duplicated inline)
- The existing variable-width Lambda budget thread remains applicable: the eight-batch fast path can exceed the byte budget and
ColumnStringoffset ceiling. - The existing external duplicate-key Map thread remains applicable: external/serialized Maps can reach the new unique/filter paths without last-win normalization.
Critical checkpoint conclusions
- Goal and proof: the PR adds the six advertised Nereids Map-Lambda functions, tuple-Lambda syntax, FE lowering, BE constructors/filtering, and broad ordinary-path tests. The five new findings and two existing blockers prevent the feature from meeting that goal end to end.
- Scope and focus: all 26 changed files and the complete 3,060-line authoritative diff were reviewed. The implementation is mostly feature-focused; the ArrayMap fast-path expansion is the principal adjacent change. No additional user review focus was supplied.
- Concurrency: analysis and execution use query-local expression/column state; no new shared mutable state, thread entry, locks, lock ordering, or atomic protocol is introduced.
- Lifecycle and static initialization: scoped Lambda bindings, rewritten expression ownership, and COW columns were traced. No non-intuitive release path, cycle, or cross-TU static initialization dependency was found.
- Configuration: no configuration item or dynamic-update path is added.
- Compatibility: public/internal names and registrations match. The three new serialized BE symbols are additive and compatible with the repository's BE-before-FE rolling order; no storage format, persisted metadata, or protocol variable changes are introduced.
- Parallel paths: direct and Lambda
map_filter, all six one-driver lowerings, public/internal constructors, native/external Map producers, nested Lambdas, access-path pruning, and old/new BE symbol paths were compared. The surviving divergences are represented by the five attached findings and two existing threads. - Conditional checks: null/empty/constant/computed/volatile Maps, nullable predicates, mask/offset mismatches, hidden null-row payload, nested NullTypes, selector bounds, and error propagation were checked. Ordinary-sized direct filtering is sound outside the listed issues.
- Test coverage: changed BE, FE, parser, access-path, and regression tests cover broad normal, null, empty, constant, duplicate-producing, nested, volatile, aggregate, Join, and Generate cases, but omit the accepted adversarial cases and the two existing blocker boundaries.
- Test results: all 44 changed regression query/result names correspond one-for-one and committed ordinary results were checked. Per the review-run contract, no build or test was executed; current completed style, formatting, license, secret, and large-file checks pass.
- Observability: these are local scalar analysis/execution paths with existing
AnalysisException/Statuspropagation; no additional distributed log, identifier, or metric is required. - Transactions and persistence: no transaction, EditLog, master-failover, or persistent-state path is changed.
- Data writes and crashes: the feature is query-time only and does not mutate stored data. Outside the physical type mismatch, selector corruption, and existing memory blocker already reported, no additional crash-recovery, leak, or atomicity issue survived.
- FE/BE variables: no session or protocol variable is added. Function names align, but the second inline finding identifies a concrete FE descriptor versus BE physical-column disagreement.
- Performance: ordinary entry processing is linear and COW use is reasonable. The existing eight-batch variable-width issue remains a correctness/allocation blocker; no other distinct CPU or memory issue survived.
- Other issues: tuple-parser ambiguity was rechecked and dismissed because the predicate only enables a branch that adaptive prediction can reject; the nested captured-entry access-path issue is fixed in the current base. Every other candidate was either disproved or duplicate-fenced.
- User focus: no additional focus was supplied, so the review covered the full PR without a separate focus constraint.
### What problem does this PR solve? Issue Number: None Related PR: apache#66968 Problem Summary: Map offsets use 64-bit positions while IColumn::Selector stores 32-bit indexes. map_filter and the filtered-entry map constructor could therefore truncate a position at 2^32 and silently copy the wrong key and value. Validate the nested entry count before building a selector and return INVALID_ARGUMENT when an index cannot be represented. Add an O(1)-memory boundary unit test for the overflow case. ### Release note Reject map operations whose nested positions exceed the selector range instead of returning corrupted data. ### Check List (For Author) - Test: Unit Test (FunctionMapTest.* under ASAN) - Behavior changed: Yes (oversized selector inputs now return INVALID_ARGUMENT) - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review status: complete and converged after two rounds. Requesting changes without new inline comments because every surviving blocker is already covered by an existing review thread.
Current-head blockers (not duplicated inline)
- The existing variable-width Lambda budget thread remains applicable:
_calculate_lambda_batch_size()can return the variable-width safety ceiling and the new direct path then admits8 * max_rows, defeating the byte bound and risking multi-gigabyteColumnStringexpansion. - The existing external duplicate-key Map thread remains applicable: external and serialized Map readers can feed the new unique/filter paths without Doris's last-win normalization.
- The existing internal-constructor coercion thread remains applicable: Lambda
map_filtercreatesMapFromFilteredEntriesUniqueafter wrapper coercion, so the normalized FE result descriptor can disagree with the physical entry fields sent to BE. - The existing hidden null-Map payload thread remains applicable:
MapEntriesandArrayMapstill evaluate nested entries retained beneath semantically null outer Map rows. - The existing independent Map precision thread remains applicable:
MapFilter,TransformValues,TransformKeys, andMapApplystill omit the no-op precision contract that prevents independent key/value decimal or time scales from being merged.
Current-head repairs and dismissals
- The 64-to-32-bit selector issue is fixed by explicit range checks and a boundary test.
- The nested-Lambda Map access-path dependency is present in the exact base/head and the captured outer entry now resolves through every active Lambda frame.
- The contradictory regression expected-error block was replaced with a deterministic result.
- The mixed-version BE-symbol candidate is dismissed under Doris's documented BE-first, Master-FE-last upgrade sequence.
- The tuple-Lambda scalar-subquery candidate is dismissed because its semantic predicate only enables the tuple alternative; adaptive prediction still eliminates that branch when its required expression cannot start with
SELECT,WITH, orVALUES.
Critical checkpoint conclusions
- Goal and proof: the PR adds the six advertised Nereids Map-Lambda functions, tuple syntax, FE lowerings, BE constructors/filtering, and broad ordinary-path tests. The five existing blocker mechanisms above prevent end-to-end correctness.
- Scope and focus: all 26 changed files and the complete 2,925-line authoritative diff were reviewed. The implementation is mostly feature-focused; the ArrayMap eight-batch fast path is the principal adjacent change. No additional user focus was supplied.
- Concurrency: analysis and execution state is query-local; no shared mutable state, thread entry, lock ordering, or atomic protocol is introduced.
- Lifecycle and initialization: Lambda binding frames, nullable nested payloads, rewritten expression ownership, and COW columns were traced. The hidden-payload blocker is listed above; no separate release, cycle, or static-initialization defect survived.
- Configuration: no configuration item or dynamic-update path is added.
- Compatibility: public/internal names and FE/BE registrations match, and the new symbols are compatible with the documented upgrade order. No storage format, persisted metadata, or protocol-variable change is introduced.
- Parallel paths and special conditions: all six wrappers, direct
map_filter, both internal constructors, native/external Map producers, const/null/empty inputs, nullable predicates, nested Lambdas, access-path pruning, selector/offset bounds, and join/branch/aggregate lowering were checked. The surviving divergences are exactly the five duplicate-fenced threads above. - Tests and results: changed BE, FE, parser, access-path, and regression sources cover broad normal, null, empty, constant, duplicate-producing, nested, volatile, aggregate, Join, and Generate cases. All 37 changed regression query labels match committed result blocks, but the five blocker boundaries are not proved. Per the review contract, no local build or test was run.
- Observability: these are local scalar analysis/execution paths with existing
AnalysisException/Statuspropagation; no additional distributed log, identifier, or metric is required. - Persistence, transactions, and writes: no EditLog, failover, transaction, persistent-state, or stored-data mutation path changes.
- FE/BE variables: no session or protocol variable is added. Physical function names align; the internal-constructor thread identifies the surviving descriptor/column mismatch.
- Performance: ordinary entry processing is linear and COW use is reasonable, but the existing eight-batch variable-width issue remains a correctness and allocation blocker; no other distinct performance issue survived.
- Other issues: the parser and mixed-version candidates were independently challenged and dismissed. Every other candidate was either fixed at head, disproved, or duplicate-fenced.
- User focus: no additional focus was supplied.
Review completeness: complete and converged. Round 2 normal FE, normal BE/tests, and separate parser/resolved-thread risk reviews all returned NO_NEW_VALUABLE_FINDINGS.
TPC-H: Total hot run time: 16923 ms |
FE UT Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 81934 ms |
ClickBench: Total hot run time: 14.62 s |
What problem does this PR solve?
Related PR: #67045, #67047
Doc: apache/doris-website#4085
Problem Summary:
This PR adds MAP lambda support under the Nereids planner.
Supported higher-order functions:
map_filter((k, v) -> predicate, map)map_exists((k, v) -> predicate, map)map_all((k, v) -> predicate, map)map_apply((k, v) -> struct(new_key, new_value), map)transform_keys((k, v) -> new_key, map)transform_values((k, v) -> new_value, map)Example:
Implementation
Use
map_entries(m)to expand the map parameter and reuse the originalarray lambdafor execution.Internal helper function
This PR adds two internal Map construction functions used by the rewritten expressions:
transform_valuemap_from_entries. Used bymap_filter