Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/23922 )
Change subject: IMPALA-9059: Add UNPIVOT clause ...................................................................... Patch Set 11: (9 comments) > Patch Set 9: > > (10 comments) > > Thanks for applying the changes! Thanks for your reviews! http://gerrit.cloudera.org:8080/#/c/23922/9/be/src/exec/unpivot-node.h File be/src/exec/unpivot-node.h: http://gerrit.cloudera.org:8080/#/c/23922/9/be/src/exec/unpivot-node.h@51 PS9, Line 51: > nit: could be initialized to -1 / 0. Thanks! Added. http://gerrit.cloudera.org:8080/#/c/23922/9/be/src/exec/unpivot-node.cc File be/src/exec/unpivot-node.cc: http://gerrit.cloudera.org:8080/#/c/23922/9/be/src/exec/unpivot-node.cc@136 PS9, Line 136: // Unlike a UnionNode, for each input tuple, an UnpivotNode might produce multiple > You should add Thanks! Added. http://gerrit.cloudera.org:8080/#/c/23922/9/be/src/exec/unpivot-node.cc@187 PS9, Line 187: > Can child_row_batch_ be null? Thanks! Added a check. http://gerrit.cloudera.org:8080/#/c/23922/9/be/src/exec/unpivot-node.cc@196 PS9, Line 196: return ExecNode: > Can child_row_batch_ be null? Thanks! I think we can still call reset() even if it is null. What do you think? Thanks! http://gerrit.cloudera.org:8080/#/c/23922/9/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/23922/9/fe/src/main/cup/sql-parser.cup@3413 PS9, Line 3413: unpivot_column_item ::= > nit: could be 'unpivot_column_item' Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/23922/9/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/9/fe/src/main/java/org/apache/impala/analysis/UnpivotTableRef.java@225 PS9, Line 225: String sourceTableColumnName = entry.getKey(); : LiteralExpr headerExpr = entry.getValue(); : SlotRef sourceTableSlotRef = registerSourceTableSlotRef( : analyzer, Lists.newArrayList(sourceTableColumnName)); : unpivotExprMap_.put(sourceTableSlotRef, headerExpr); : } : } : if (columnName.equals(headerColumn_)) { : headerSlotDesc_ = slot; : } else { : > Seems like the same loop is being executed if slot is the dataColumn and wh Thanks! Changed to executing it only once. http://gerrit.cloudera.org:8080/#/c/23922/9/fe/src/main/java/org/apache/impala/planner/UnpivotNode.java File fe/src/main/java/org/apache/impala/planner/UnpivotNode.java: http://gerrit.cloudera.org:8080/#/c/23922/9/fe/src/main/java/org/apache/impala/planner/UnpivotNode.java@171 PS9, Line 171: ssingCost.basicCost( > We should multiply it by 'getNumUnpivotColumns()', or just use the output c Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/23922/9/testdata/workloads/functional-query/queries/QueryTest/unpivot-clause.test File testdata/workloads/functional-query/queries/QueryTest/unpivot-clause.test: http://gerrit.cloudera.org:8080/#/c/23922/9/testdata/workloads/functional-query/queries/QueryTest/unpivot-clause.test@323 PS9, Line 323: 7,'m',4 > Could you please add a few more tests? Thanks! Added. http://gerrit.cloudera.org:8080/#/c/23922/9/testdata/workloads/functional-query/queries/QueryTest/unpivot-clause.test@324 PS9, Line 324: 0,'y',2009 > Most tests are on small tables/CTEs, they could be executed with different Thanks! Added. -- 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: 11 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 11 May 2026 08:53:29 +0000 Gerrit-HasComments: Yes
