Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24852 )
Change subject: IMPALA-15358: Cap Kudu DML writers to tablet count ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/24852/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24852/6//COMMIT_MSG@9 PS6, Line 9: UPSERT/UPDATE/DELETE Can we tests for these as well? http://gerrit.cloudera.org:8080/#/c/24852/6//COMMIT_MSG@25 PS6, Line 25: Could you please add e2e tests that check #Inst for KUDU WRITER from the exec summary? Currently we only have Planner tests. http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/analysis/KuduPartitionParam.java File fe/src/main/java/org/apache/impala/analysis/KuduPartitionParam.java: http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/analysis/KuduPartitionParam.java@226 PS6, Line 226: getNumHashPartitions Unused? http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java File fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java: http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@166 PS6, Line 166: getTabletsLocations Deprecated API, could use KuduPartitioner.numPartitions() http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@169 PS6, Line 169: Exception Do we hit this for CTAS statements? http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@170 PS6, Line 170: throw new ImpalaRuntimeException("Error accessing Kudu for tablet locations.", : e); Maybe we could just derive the result from partition params? Or return -1? http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@354 PS6, Line 354: IMPALA-5254 Should be IMPALA-15358? http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@359 PS6, Line 359: // Note: dmlStmt.getMaxTableSinks() is not used here since it is only : // populated from MAX_FS_WRITERS for HDFS/Iceberg targets, not Kudu. : int maxFsWriters = analyzer.getQueryOptions().getMax_fs_writers(); Can we document that MAX_FS_WRITERS now caps Kudu writers as well? http://gerrit.cloudera.org:8080/#/c/24852/6/testdata/workloads/functional-planner/queries/PlannerTest/kudu-insert-writer-limit.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu-insert-writer-limit.test: http://gerrit.cloudera.org:8080/#/c/24852/6/testdata/workloads/functional-planner/queries/PlannerTest/kudu-insert-writer-limit.test@1 PS6, Line 1: capped at the : # table's Kudu hash bucket count This isn't true anymore. -- To view, visit http://gerrit.cloudera.org:8080/24852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3644fb245ceac72e91ed70c8c0392f65df75fde Gerrit-Change-Number: 24852 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 29 Sep 2026 13:19:53 +0000 Gerrit-HasComments: Yes
