[Impala-ASF-CR] IMPALA-15101: Add Iceberg UUID primitive type

[Zoltan Borok-Nagy (Code Review)]

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/24504 )

Change subject: IMPALA-15101: Add Iceberg UUID primitive type
......................................................................


Patch Set 5:

(8 comments)

Looks great! Please also add the following to SlotRef.analyze() so the backend 
doesn't crash on UUID types:

 if (type_.isUuid()) {
     throw new AnalysisException("Reading UUID columns is not yet supported.");
 }

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: D
nit: please keep the 72-char limit


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: 16
This should be a static constant at class level.


http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.h@199
PS5, Line 199: || type == TYPE_UUID
UUID always has len 16, so we don't need this.


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: 16
We should use a constant here, related to my other comment in the header.


http://gerrit.cloudera.org:8080/#/c/24504/5/be/src/runtime/types.cc@322
PS5, Line 322: , i8, <padding>
Nice catch, thanks for updating this!


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 
AlterTableReplaceColsStmt.


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: even though string literals are implicitly castable to UUID in 
predicates
It is not true anymore. And I'm not sure if we want implicit casts in the 
future.


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;
Add UUID here.



--
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: 5
Gerrit-Owner: Arnab Karmakar <arnabk1...@gmail.com>
Gerrit-Reviewer: Arnab Karmakar <arnabk1...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Jul 2026 15:36:07 +0000
Gerrit-HasComments: Yes