Pranav Lodha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22914 )
Change subject: IMPALA-14081: Support create/drop paimon table for impala ...................................................................... Patch Set 11: (14 comments) > Patch Set 11: > > (11 comments) > > Thank you for working on this. Left a few small comments otherwise LGTM. > > Also, please reply to every comment or just mark them as "done", thanks. Thanks for the patch. Left some minor comments. http://gerrit.cloudera.org:8080/#/c/22914/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22914/11//COMMIT_MSG@12 PS11, Line 12: paimon It'd be good to mention all the datatypes supported. http://gerrit.cloudera.org:8080/#/c/22914/11//COMMIT_MSG@46 PS11, Line 46: Would also recommend writing a TODO section for all things that are WIP or still uncertain. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/ImpalaTypeUtils.java File fe/src/main/java/org/apache/impala/catalog/paimon/ImpalaTypeUtils.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/ImpalaTypeUtils.java@156 PS11, Line 156: // @Override : // public Type visit(TimeType timeType) { : // return Type.DATETIME; : // } Can you add a comment or explain why these and the following functions are commented? http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/ImpalaTypeUtils.java@258 PS11, Line 258: case DATETIME: If this is a TODO, please add it to the TODO list. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/catalog/paimon/PaimonCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonCatalogOpExecutor.java@122 PS11, Line 122: , e); Do you mean 'e'? Also can you be a bit more descriptive with these exceptions. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonCatalogOpExecutor.java@177 PS11, Line 177: n(e.getMessage()) Please be more descriptive. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java File fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java@108 PS11, Line 108: // TODO: do verification paimon table Would you be adding it on the same patch or in a separate patch? Would recommend to mention it in commit message if you plan to do the latter. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java@191 PS11, Line 191: addVirtualColumn(VirtualColumn.ICEBERG_PARTITION_SERIALIZED); Yes, would suggest to make the mapping configurable. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java@226 PS11, Line 226: catalogTimeline.markEvent("Loaded stats from Paimon"); Maybe add a marker when schema sync to HMS completes as well. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonUtil.java File fe/src/main/java/org/apache/impala/catalog/paimon/PaimonUtil.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonUtil.java@230 PS11, Line 230: Returns the corresponding paimon catalog implementation, Not used for now. I'd suggest to put it as a TODO, and do it in a separate patch if it's not used. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonUtil.java@522 PS11, Line 522: final int[] SNAPSHOT_TABLE_PROJECTION = {5, 0, 1}; Can you add a comment about these numbers? http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonUtil.java@567 PS11, Line 567: } Maybe change it to: throw new DatabaseNotFoundException("Failed to get snapshot: " + ex.getMessage(), ex); to be more descriptive. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4407 PS11, Line 4407: TPaimonCatalog underlyingCatalog = PaimonUtil.getTPaimonCatalog(newTable); Maybe add a meaningful exception if catalog type is unsupported. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4430 PS11, Line 4430: ICEBERG Please add a comment about this. -- To view, visit http://gerrit.cloudera.org:8080/22914 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I57e77f28151e4a91353ef77050f9f0cd7d9d05ef Gerrit-Change-Number: 22914 Gerrit-PatchSet: 11 Gerrit-Owner: ji chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 19 Aug 2025 22:28:21 +0000 Gerrit-HasComments: Yes
