Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/23922 )
Change subject: IMPALA-9059: Add UNPIVOT clause ...................................................................... Patch Set 2: (4 comments) This will be a very useful addition to Impala! Just went through the code quickly, but planning to do deeper iterations later this week! http://gerrit.cloudera.org:8080/#/c/23922/2/be/src/exec/unpivot-node.h File be/src/exec/unpivot-node.h: http://gerrit.cloudera.org:8080/#/c/23922/2/be/src/exec/unpivot-node.h@60 PS2, Line 60: virtual nit: virtual keyword is redundant http://gerrit.cloudera.org:8080/#/c/23922/2/be/src/exec/unpivot-node.cc File be/src/exec/unpivot-node.cc: http://gerrit.cloudera.org:8080/#/c/23922/2/be/src/exec/unpivot-node.cc@93 PS2, Line 93: for (const auto expr : plan_node.source_exprs_) { : ScalarExprEvaluator* eval = nullptr; : RETURN_IF_ERROR(ScalarExprEvaluator::Create( : *expr, state, pool_, expr_perm_pool(), expr_results_pool(), &eval)); : source_evals_.push_back(eval); : } : for (const auto expr : plan_node.data_exprs_) { : ScalarExprEvaluator* eval = nullptr; : RETURN_IF_ERROR(ScalarExprEvaluator::Create( : *expr, state, pool_, expr_perm_pool(), expr_results_pool(), &eval)); : data_evals_.push_back(eval); : } : for (const auto expr : plan_node.header_exprs_) { : ScalarExprEvaluator* eval = nullptr; : RETURN_IF_ERROR(ScalarExprEvaluator::Create( : *expr, state, pool_, expr_perm_pool(), expr_results_pool(), &eval)); : header_evals_.push_back(eval); : } It's a bit repetitive, how about: auto init_evals = [&](const auto& exprs, auto& evals) -> Status { for (const auto expr : exprs) { ScalarExprEvaluator* eval = nullptr; RETURN_IF_ERROR(ScalarExprEvaluator::Create( *expr, state, pool_, expr_perm_pool(), expr_results_pool(), &eval)); evals.push_back(eval); } return Status::OK(); }; RETURN_IF_ERROR(init_evals(plan.source_exprs_, source_evals_)); RETURN_IF_ERROR(init_evals(plan.data_exprs_, data_evals_)); RETURN_IF_ERROR(init_evals(plan.header_exprs_, header_evals_)); http://gerrit.cloudera.org:8080/#/c/23922/2/be/src/exec/unpivot-node.cc@161 PS2, Line 161: RETURN_IF_CANCELLED(state); : RETURN_IF_ERROR(QueryMaintenance(state)); nit: I think it's enough to do per invocation of GetNex(), no need to do these after each row. http://gerrit.cloudera.org:8080/#/c/23922/2/fe/src/main/java/org/apache/impala/analysis/UnpivotTableRef.java File fe/src/main/java/org/apache/impala/analysis/UnpivotTableRef.java: http://gerrit.cloudera.org:8080/#/c/23922/2/fe/src/main/java/org/apache/impala/analysis/UnpivotTableRef.java@42 PS2, Line 42: AS Other engines don't require the AS-clause: https://learn.microsoft.com/en-us/sql/t-sql/queries/from-using-pivot-and-unpivot?view=sql-server-ver17 https://docs.snowflake.com/en/sql-reference/constructs/unpivot https://docs.databricks.com/aws/en/sql/language-manual/sql-ref-syntax-qry-select-unpivot Would it be possible to make the AS-clause optional? -- To view, visit http://gerrit.cloudera.org:8080/23922 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10e9d0a1bd70de02c7998afca274b895055d9b6a Gerrit-Change-Number: 23922 Gerrit-PatchSet: 2 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 03 Feb 2026 17:22:25 +0000 Gerrit-HasComments: Yes
