[feature](function) Support Spark/Hive-compatible stack table function - #66734
Merged
Conversation
HappenLee
requested review from
924060929,
englefly,
gavinchou,
morrySnow,
starocean999 and
yiguolei
as code owners
August 13, 2026 10:03
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
HappenLee
commented
Aug 13, 2026
HappenLee
left a comment
HappenLee
left a comment
Contributor
Author
There was a problem hiding this comment.
Review requested: implementation and the reported FE/BE/regression validation are complete. Please review the stack table-function compatibility changes.
### What problem does this PR solve? Issue Number: close apache#66687 Related PR: None Problem Summary: Doris does not provide the Spark/Hive-compatible stack table-generating function. Add FE validation, recursive constant folding, signature inference, and per-output-column type checks, together with BE execution that arranges values in row-major order and pads incomplete rows with nulls. ### Release note Support the Spark/Hive-compatible `stack(num_rows, expr1, ..., exprN)` table-generating function. ### Check List (For Author) - Test: Regression test and Unit Test - `TableFunctionTest.vstack` - `StackTest` - `query_p0/sql_functions/table_function/stack` - Behavior changed: Yes (adds the `stack` table-generating function) - Does this need documentation: No
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: three blocking issues remain in stack's analysis and execution paths.
Review completion: converged after three full FE, BE, and risk-focused rounds. Every final-round reviewer returned NO_NEW_VALUABLE_FINDINGS, and every candidate was independently adjudicated against the live inline-thread set.
- Goal and test plan: The implementation covers the intended row-major layout, NULL padding, scalar/struct outputs, and same-column type checks, but the test plan misses the three blocking boundaries called out inline.
- Focusedness: The change is focused on adding
stack; no unrelated production behavior was found. - Concurrency: Table-function state is operator-local, and no new shared mutable state or race was found.
- Lifecycle: Normal row/null-map/cursor handling and close/reset behavior are coherent; the inline BE issue is the retained block-wide constant expansion.
- Configuration: No configuration or session-variable contract is added.
- Compatibility: FE registration, BE fake-function registration, and the table-function factory are symmetric. No storage, protocol, or persisted-state format changes were found.
- Parallel and special paths: Scalar versus struct output, typed/all-NULL values, multiple child rows, batch splits, and Cartesian products with other table functions were traced; no additional defect survived.
- Conditions and error handling: Alias cardinality and row-count legality/evaluation do not consistently produce stable analysis errors; see the two FE comments.
- Tests: Existing FE/BE/regression tests cover ordinary results and several invalid inputs, but not short/long aliases, context-dependent or unsupported deterministic constant row counts, or multi-row literal amplification.
- Expected outputs: The checked-in ordered results match the intended row-major and padding semantics for the covered cases.
- Observability: No new logging or metrics are required, but invalid SQL should surface
AnalysisExceptionrather than internal exceptions. - Transactions and persistence: Not applicable; no transactional or persisted-state path is changed.
- Data writes: Not applicable; the function only generates query rows.
- FE/BE handoff: The function name and argument-0 constness metadata line up, and ordinary accepted inputs reach BE with coherent physical types; the remaining FE evaluator failures are inline.
- Performance: Constant value children are expanded by input-block cardinality and retained; see the BE comment.
- Other issues: No additional correctness, security-boundary, compatibility, or observability issue survived the final sweep.
- User focus:
review_focus.txtcontained no additional focus; the complete PR was reviewed.
Validation performed: static full-diff and upstream/downstream control-flow review only, as required by this review runner; I did not run local builds or tests. At review time, CheckStyle and Clang Formatter were passing, while the macOS BE unit-test job was still pending.
| for (int i = 0; i < columnTypes.size(); i++) { | ||
| fields.add(new StructField("col" + i, columnTypes.get(i), true, "")); | ||
| } | ||
| return FunctionSignature.of(new StructType(fields.build()), argumentTypes); |
Contributor
There was a problem hiding this comment.
[P1] Validate the dynamic stack alias count before expansion
stack can derive a different number of struct fields for each call, but the generic binder later iterates every field and indexes the supplied alias list without checking its size. For example, lateral view stack(2, 1, 2, 3, 4, 5) s as a, b derives three fields and reaches aliases.get(2), producing an internal IndexOutOfBoundsException; supplying four aliases silently drops the fourth. Please validate the alias count against the derived stack schema (while preserving the intended one-alias whole-result form if required) and return an AnalysisException; add short/long cardinality tests.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2351b0f. Multi-column Stack now always enters STRUCT expansion and exact alias-count validation, including the previously bypassed one-alias case. A 3-column stack with 1, 2, or 4 aliases returns AnalysisException; exactly 3 aliases still analyzes successfully. Single-column Stack and the existing one-alias behavior of other struct-returning generators are unchanged. Added a direct Nereids analyzer unit test plus the one-alias regression case.
| for (size_t i = 1; i < children.size(); ++i) { | ||
| RETURN_IF_ERROR(children[i]->execute(_expr_context.get(), block, &column_index)); | ||
| _value_columns.emplace_back( | ||
| block->get_by_position(column_index).column->convert_to_full_column_if_const()); |
Contributor
There was a problem hiding this comment.
[P1] Keep constant stack arguments compact per input block
convert_to_full_column_if_const() expands each literal to block->rows() physical values and _value_columns retains all of them until the child block is exhausted. Literal-heavy stack calls therefore multiply live memory/CPU by input_rows * constant_arguments * value_size (for example, 100 constant 1 KiB strings over 4096 rows is about 400 MiB of duplicated source payload). Preserve/unpack the const column and read row 0 for constants versus _row_idx for ordinary columns; add a multi-row constant-value case so the common constant form does not amplify per block.
### What problem does this PR solve? Issue Number: close apache#66687 Related PR: None Problem Summary: The stack implementation expanded constant value columns across each input block, did not reliably fold supported cardinality expressions for the row count, could invoke constant folding on runtime-only expressions, and did not validate dynamic output aliases before expansion. Preserve compact constant columns and read row zero, fold cardinality literals, reject nondeterministic and otherwise non-foldable row counts with analysis errors, validate alias cardinality, and cover these boundaries in unit and regression tests. ### Release note Support Spark/Hive-compatible stack table-generating function semantics. ### Check List (For Author) - Test: Regression test and Unit Test - BE TableFunctionTest.vstack - FE StackTest (7 tests) - query_p0/sql_functions/table_function/stack generated and rerun - BE and FE builds, BE clang-tidy and format checks, FE checkstyle - Behavior changed: Yes (fixes stack constant-column handling and analysis validation) - Does this need documentation: No
Contributor
Author
|
/review |
### What problem does this PR solve? Issue Number: close apache#66687 Related PR: None Problem Summary: Multi-column stack calls with a single explicit alias bypassed alias cardinality validation and exposed an internal STRUCT instead of following Spark and Hive exact alias-count semantics. Stack also invoked null-context constant folding for deterministic runtime expressions because it rejected only nondeterministic inputs. Require the row-count expression to satisfy Expression.isConstant() before folding, and make multi-column stack validate every non-empty alias list against its derived output schema. ### Release note Make stack reject mismatched output aliases and nonconstant row-count expressions with analysis errors. ### Check List (For Author) - Test: Unit Test - FE StackTest: 8 tests passed - Regression cases updated; not run locally because no repository output cluster was running with this change - Behavior changed: Yes (multi-column stack calls with one alias now fail like Spark and Hive) - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Request changes: two distinct P1 correctness gaps remain in stack analysis. A logical one-column result is confused with the multi-column STRUCT carrier when the value itself is STRUCT, and compatible ordinary literal types are rejected before the custom signature can widen them.
Review status: the full 14-file change and its parser/analyzer/translator/operator handoffs were reviewed through three cyclic rounds. Round 3 returned no new valuable findings, every candidate has been accepted, duplicate-fenced, or dismissed with concrete reachability evidence, and the review converged. No additional user focus was provided, so the full PR was reviewed without extra narrowing.
Critical checkpoint conclusions:
- Goal and proof: the PR wires a Spark/Hive-style
stacktable-generating function through FE and BE and includes FE unit, BE unit, and regression coverage. The ordinary scalar, NULL-padding, const, and true multi-column cases are implemented coherently, but the two inline cases show that the compatibility goal is not yet complete. - Scope: the implementation is focused on function registration, dynamic schema analysis, translation, execution, and tests. No unrelated production behavior was found in the changed files.
- Concurrency: no new threads, shared mutable state, locks, or lock ordering are introduced. Generator instances and retained columns are operator/query local.
- Lifecycle/static initialization: FE registration and BE factory registration follow existing patterns; retained evaluated columns are scoped to table-function processing and released on close/block transitions. No cross-translation-unit static-initialization dependency or lifecycle leak was found.
- Configuration: no configuration item or dynamic-reload behavior is added.
- Compatibility: no storage/persistence format changes are introduced. FE lookup, legacy translation, BE fake-function lookup, and runtime factory lookup are present; an older BE fails as unsupported rather than decoding mismatched state. The remaining compatible-value rejection is called out inline.
- Parallel paths: parser return-many handling, Nereids binding, legacy expression translation, BE fake preparation, and table-function creation were traced. No missing parallel registration path was found.
- Conditions and errors: positive constant row-count, output-width, alias-count, and NULL-padding checks were inspected. Existing live threads already cover the residual null-context/physical-const concern; the two new, distinct condition defects are inline.
- Test coverage: coverage is substantial across FE, BE, and regression layers, including negative cases, but it lacks width-one STRUCT values and compatible mixed-width integer/VARCHAR literals. Both missing regressions are requested inline.
- Test results: no tests or builds were run in this bundle-only review environment. The committed test code and expected outputs were inspected; no incorrect expected row was found.
- Observability: this is a synchronous expression-analysis/execution feature with existing analysis errors and operator status propagation; no new logs or metrics are required.
- Transactions/persistence: not applicable; no transaction state, EditLog, replay, or persisted metadata changes.
- Data writes/atomicity: not applicable; the change produces query rows and does not mutate stored data.
- FE-BE variables: no new session/config variable or scattered thrift variable is introduced. Function metadata and dynamic return type use the existing expression serialization path.
- Performance: const input materialization is compact at the current head. The suspected row-wise Cartesian repeat cost was dismissed because the mandatory Stack alias Project prevents Stack from occupying the repeated generator position in a production plan; no reachable blocking performance issue remains.
- Other: FE/BE width and row-major indexing agree for ordinary arities, trailing padding and nullable/nested values are aligned, and no additional correctness, memory-safety, security-boundary, or style blocker was substantiated.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: close apache#66687 Related PR: apache#66734 Problem Summary: Stack uses a STRUCT return type both as the carrier for a logical multi-column result and as a legitimate single output value. The binder previously treated every STRUCT returned by Stack as the multi-column carrier, so a one-column Stack containing STRUCT values either flattened a one-field STRUCT or rejected a multi-field STRUCT because its fields did not match the single alias. Derive the logical Stack output width from the row count and value arguments, validate aliases against that width, and expand the STRUCT carrier only when the logical width is greater than one. A logical one-column result now preserves the complete STRUCT value. ### Release note Stack now preserves STRUCT values when the function has one logical output column. ### Check List (For Author) - Test: Regression test and Unit Test - StackTest: 9 tests passed - query_p0/sql_functions/table_function/stack: passed - FE build with UI disabled: passed - Behavior changed: Yes. A one-column Stack over STRUCT values now returns each complete STRUCT instead of flattening or rejecting it. - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: no new inline thread is warranted, but the existing Stack row-count folding blocker remains unresolved on the current head.
Existing blocker: #66734 (comment). The new isConstant() guard does not fully make the null-context schema fold safe. KEY db.key parses as a deterministic EncryptKeyRef over two string literals, so it passes isConstant(); FoldConstantRuleOnFE.evaluate(..., null) then dispatches to visitEncryptKeyRef(), which dereferences context.cascadesContext and raises an internal NPE instead of Stack's documented AnalysisException. CAST(KEY db.key AS INT) reaches the same visitor, so a top-level type check alone is insufficient. This is the same changed call and mechanism already covered by the unresolved thread, so I did not open a duplicate inline comment. Please make the schema-time fold context-safe (or pass the real rewrite context) and add a negative analyzer regression for this case.
Review status: complete and converged after two full FE/test, BE/cross-layer, and risk-focused rounds over all 14 changed files. Every Round 2 reviewer returned NO_NEW_VALUABLE_FINDINGS; the residual reproducer above was independently validated and deduplicated against the live thread set. No other distinct issue survived the final sweep.
Critical checkpoint conclusions:
- Goal and proof: the PR implements row-major Spark/Hive-style
stack, NULL padding, dynamic output width, and scalar/STRUCT results across FE and BE, with FE unit, BE unit, and regression coverage. Ordinary covered behavior is coherent, but the unresolved analysis-error boundary above prevents approval. - Scope and reuse: the production change is focused on registration, schema analysis, translation, execution, and tests, and follows existing table-function and expression-folding infrastructure. No unrelated behavior was found.
- Concurrency: no new threads, shared mutable state, locks, or lock-ordering concerns are introduced. Table-function execution state is local to the operator instance.
- Lifecycle and memory safety: VStack retains owning
ColumnPtrs only for the active child block, releases them throughprocess_close(), reads compact constants from physical row 0, and appends nullable scalar/complex values with consistent payload/null-map structure. No cross-translation-unit initialization dependency, ownership leak, or distinct memory-accounting issue was found. - Configuration: no configuration item or dynamic-reload behavior is added.
- Compatibility and FE-BE contract: FE and BE agree on the lowercase symbol, INT constant row count, row-major width, casts, dynamic return type, and nullable fields. An older BE lacking the new symbol fails explicitly; no storage, persistence, or wire-format change is introduced. The resolved exact-type discussion is consistent with Spark's exact non-NULL type check and NullType-only coercion.
- Parallel and special paths: mixed-case names, short/long aliases, sequential/merged generators, width-one genuine STRUCT values, width-many synthetic STRUCT carriers, all-NULL and nested complex values, const/nonconst sources, cross products, repeated output, batch splits, and final-row padding were traced. No distinct issue survived.
- Conditions and error handling: alias count, positive/range row counts, output width, NULL padding, and BE invariants were inspected. The existing row-count thread remains the one blocking error-path defect because invalid SQL can still surface an internal NPE.
- Tests and expected results: coverage is substantial and checked-in ordered results match the row-major/padding inputs. The missing
KEYnegative analyzer case belongs to the existing unresolved blocker. No local build or test was run because this review runner explicitly forbids it. - Observability: existing synchronous analysis errors and operator status propagation are sufficient; no new logs or metrics are required.
- Transactions, persistence, and writes: not applicable; the change neither persists metadata nor mutates stored data.
- FE-BE variables: no new configuration/session variable or scattered thrift field is introduced; existing function-expression metadata carries the required contract on all traced paths.
- Performance: the current head keeps constant arguments compact per input block, and no other reachable blocking CPU, allocation, or redundant-copy issue was found.
- Other risks: no additional correctness, lifecycle, security-boundary, compatibility, performance, style, or observability issue was substantiated.
User focus: review_focus.txt contained no additional focus, so the complete PR was reviewed without extra narrowing.
Validation performed: static full-diff and upstream/downstream control-flow review only. At submission preparation time, CheckStyle, Clang Formatter, dependency review, license, and secret checks were passing; the macOS BE unit-test job was still in progress.
### What problem does this PR solve? Issue Number: close apache#66687 Related PR: apache#66734 Problem Summary: Stack derives its output schema before an expression rewrite context is available. Its row-count validation called the regular FE constant folder with a null context, so deterministic context-dependent expressions such as an encrypt-key reference, including one nested under CAST, dereferenced the missing context and raised an internal NullPointerException. Add an explicit context-free FE folding mode that leaves connection-, session-, and catalog-dependent expressions unresolved while preserving pure constant folding. Stack now rejects unresolved row-count expressions with its documented AnalysisException. Add analyzer regressions for direct and cast encrypt-key references. ### Release note Stack now reports a stable analysis error when its row-count argument depends on connection, session, or catalog context. ### Check List (For Author) - Test: Unit Test and FE build - StackTest: 10 tests passed - DISABLE_BUILD_UI=ON ./build.sh --fe -j 48: passed - Behavior changed: Yes. Invalid context-dependent stack row-count expressions now return an analysis error instead of an internal exception. - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: one P1 analysis-error boundary remains in Stack's new context-free row-count folding. A deterministic password(KEY db.key) expression reaches the new evaluator, but visitPassword() assumes its child is already a string literal and leaks an internal IllegalArgumentException instead of Stack's documented AnalysisException; see the inline comment.
Review status: complete and converged after two full FE, BE/tests, and risk-focused rounds over all 15 changed paths. Every round-2 reviewer returned NO_NEW_VALUABLE_FINDINGS; the sole candidate was independently verified, checked against the refreshed live thread set, and all other risks were dismissed with concrete evidence or duplicate-fenced.
Critical checkpoint conclusions:
- Goal and proof: the PR implements Spark/Hive-style row-major
stack, NULL padding, dynamic output width, and scalar/STRUCT results through FE and BE. The ordinary paths are coherent, but the inline internal-exception case prevents approval. - Scope and reuse: the changes remain focused on function registration, schema analysis, translation, execution, and tests, using existing generator and expression-folding infrastructure. No unrelated production behavior was found.
- Concurrency: no threads, shared mutable state, locks, or lock-ordering concerns are introduced. Table-function state and retained columns are operator-local.
- Lifecycle and memory safety: child columns are owned for the active input block and released on close; const columns remain compact, nullable/STRUCT insertion is aligned, and batch/cartesian roll-reset paths were traced without another issue.
- Configuration: no configuration item, session variable, or dynamic-reload contract is added.
- Compatibility and FE-BE contract: FE registration, expression serialization, BE fake-function registration, and table-function lookup agree on the symbol, constant first argument, dynamic return shape, and nullable fields. Older components reject the unknown function explicitly; no storage or persisted-state format changes are introduced. Spark's exact non-NULL per-column type rule and NullType-only coercion were also checked, so the existing widening discussion is not repeated.
- Parallel and special paths: mixed-case names, alias cardinality, width-one genuine STRUCT values, width-many synthetic STRUCT carriers, all-NULL and nested values, const/nonconst sources, multiple input rows, Cartesian products, batch splits, and final-row padding were inspected. No distinct issue survived.
- Conditions and error handling: positive/range row counts, foldability, output width, alias count, NULL padding, and BE invariants were reviewed. The inline nested
password(KEY ...)path is the remaining condition that surfaces an internal exception. - Tests and expected results: FE unit, BE unit, and ordered regression coverage exercise primary positive/negative, row/column, constant, NULL, STRUCT, and alias behavior; the checked-in expected rows match those inputs. The missing nested Password/EncryptKeyRef negative regression is requested inline.
- Observability: existing synchronous analysis errors and operator status propagation are sufficient; no new log or metric is needed, but invalid SQL must not leak an unchecked internal exception.
- Transactions, persistence, and writes: not applicable; the change does not persist metadata, change transaction state, or mutate stored data.
- Performance: the current implementation keeps literal inputs compact per block and bounds output production by the requested batch size; no additional blocking CPU, allocation, or retained-memory issue was substantiated.
- Other risks: no further correctness, lifecycle, compatibility, security-boundary, performance, style, or observability blocker was found.
User focus: review_focus.txt contained no additional focus, so the entire PR was reviewed without extra narrowing.
Validation performed: static authoritative-diff and upstream/downstream control-flow review only, as required by this review runner; no local build or test was run.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28946 ms |
Contributor
TPC-DS: Total hot run time: 158439 ms |
Contributor
ClickBench: Total hot run time: 23.89 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
924060929
reviewed
Aug 17, 2026
Comment on lines
+98
to
+115
| private int getNumRows() { | ||
| Expression numRowsArgument = getArgument(0); | ||
| if (!numRowsArgument.isConstant()) { | ||
| throw new AnalysisException("The first argument of stack must be a positive constant integer, but got: " | ||
| + numRowsArgument.toSql()); | ||
| } | ||
| Expression evaluated = FoldConstantRuleOnFE.evaluateWithoutContext(numRowsArgument); | ||
| if (!(evaluated instanceof IntegerLikeLiteral)) { | ||
| throw new AnalysisException("The first argument of stack must be a positive constant integer, but got: " | ||
| + numRowsArgument.toSql()); | ||
| } | ||
| long numRows = ((IntegerLikeLiteral) evaluated).getLongValue(); | ||
| if (numRows <= 0 || numRows > Integer.MAX_VALUE) { | ||
| throw new AnalysisException("The first argument of stack must be in (0, " + Integer.MAX_VALUE | ||
| + "], but got: " + numRows); | ||
| } | ||
| return (int) numRows; | ||
| } |
Contributor
There was a problem hiding this comment.
Use LazyCompute to reuse the result of numRows
### What problem does this PR solve? Issue Number: close apache#66687 Related PR: apache#66734 Problem Summary: Stack derives its output schema in multiple analysis phases. Each call recomputed and validated the constant row-count expression even though a Stack expression is immutable. Cache the validated row count with LazyCompute so it is evaluated only once per Stack instance while preserving lazy evaluation and all existing validation behavior. Add a unit test that verifies construction does not evaluate the row count and repeated schema access reuses the first result. ### Release note None ### Check List (For Author) - Test: Unit Test and FE build - StackTest: 11 tests passed - DISABLE_BUILD_UI=ON ./build.sh --fe -j 48: passed - Behavior changed: No. The row-count result and validation behavior are unchanged; only repeated computation is eliminated. - Does this need documentation: No
Contributor
Author
|
run buildall |
924060929
approved these changes
Aug 17, 2026
924060929
approved these changes
Aug 17, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-DS: Total hot run time: 80284 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17003 ms |
Contributor
TPC-DS: Total hot run time: 80666 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
3 participants
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: close #66687
Related PR: apache/doris-website#4059
Problem Summary: Add Spark/Hive-compatible stack(num_rows, ...) support with row-major output, NULL padding, column type validation, and recursive constant folding.
Release note
Support Spark/Hive-compatible stack table-generating function semantics.
Check List (For Author)