Arnab Karmakar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24504 )
Change subject: IMPALA-15101: Add Iceberg UUID primitive type ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/24504/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24504/5//COMMIT_MSG@11 PS5, Line 11: > nit: please keep the 72-char limit Done http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.h File be/src/runtime/types.h: http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.h@158 PS5, Line 158: T > This should be a static constant at class level. Done http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.h@199 PS5, Line 199: > UUID always has len 16, so we don't need this. Done http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.cc File be/src/runtime/types.cc: http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.cc@56 PS5, Line 56: f > We should use a constant here, related to my other comment in the header. Done http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.cc@322 PS5, Line 322: > Nice catch, thanks for updating this! Done http://gerrit.cloudera.org:8080/#/c/24504/5/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java: http://gerrit.cloudera.org:8080/#/c/24504/5/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@109 PS5, Line 109: if (!(t instanceof FeIcebergTable) && c.getType().containsUuid()) { : throw new AnalysisException( : "UUID type is only supported for Iceberg tables."); : } > Please add similar checks to AlterTableAlterColStmt and AlterTableReplaceCo Done http://gerrit.cloudera.org:8080/#/c/24504/5/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: http://gerrit.cloudera.org:8080/#/c/24504/5/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java@326 PS5, Line 326: (type_.isUuid() && defaultValLiteral.getType().isString()) { > It is not true anymore. And I'm not sure if we want implicit casts in the f Done http://gerrit.cloudera.org:8080/#/c/24504/5/fe/src/main/java/org/apache/impala/catalog/ScalarType.java File fe/src/main/java/org/apache/impala/catalog/ScalarType.java: http://gerrit.cloudera.org:8080/#/c/24504/5/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@312 PS5, Line 312: || type_ == PrimitiveType.FIXED_UDA_INTERMEDIATE || type_ == PrimitiveType.UUID; > Add UUID here. Done -- To view, visit http://gerrit.cloudera.org:8080/24504 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefc73aefe73b6144c929ec37b0cb333007cf8bfe Gerrit-Change-Number: 24504 Gerrit-PatchSet: 6 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[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: Fri, 24 Jul 2026 06:26:00 +0000 Gerrit-HasComments: Yes
