Zoltan Borok-Nagy 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: (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. http://gerrit.cloudera.org:8080/#/c/22914/11/docs/topics/impala_paimon.xml File docs/topics/impala_paimon.xml: http://gerrit.cloudera.org:8080/#/c/22914/11/docs/topics/impala_paimon.xml@237 PS11, Line 237: 1.0 nit: 1.1? http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@83 PS11, Line 83: to a HDFS table It is quite an obsolete statement. I think "SHOW FILES is applicable only to file-based tables" would be more correct. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@91 PS11, Line 91: "SHOW FILES with partition filter is applicable only to a HDFS table" Can we say "SHOW FILES with partition filter is not applicable to table type: PAIMON"? http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/analysis/paimon/PaimonAnalyzer.java File fe/src/main/java/org/apache/impala/analysis/paimon/PaimonAnalyzer.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/analysis/paimon/PaimonAnalyzer.java@257 PS11, Line 257: !colSets.contains(col) 'colSets' contains lower cased column names, but PaimonUtil.extractColumnNames() return column names as is. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java File fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java@55 PS11, Line 55: nit: too much indent http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/FePaimonTable.java File fe/src/main/java/org/apache/impala/catalog/paimon/FePaimonTable.java: http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/FePaimonTable.java@48 PS11, Line 48: public static JobConf jobConf = new JobConf(); Is jobConf needed? 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@95 PS11, Line 95: RuntimeException Here we could wrap 'e' into a TableLoadingException. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java@191 PS11, Line 191: ICEBERG_PARTITION_SERIALIZED I think it'd make sense to create a new PAIMON-specific virtual column for this. http://gerrit.cloudera.org:8080/#/c/22914/11/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java@212 PS11, Line 212: // Copy the table to check later if anything has changed. : msTable_ = msTbl.deepCopy(); No need for this deepCopy() as we don't check changes later. 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@4454 PS11, Line 4454: msClient.getHiveClient().createTable(newTable); So PaimonCatalogOpExecutor.createTable() at L4400 did not create the table even if it is in the HiveCatalog? http://gerrit.cloudera.org:8080/#/c/22914/11/tests/custom_cluster/test_paimon.py File tests/custom_cluster/test_paimon.py: http://gerrit.cloudera.org:8080/#/c/22914/11/tests/custom_cluster/test_paimon.py@25 PS11, Line 25: CustomClusterTestSuite I don't think we need a custom cluster for these tests, i.e. it could inherit from ImpalaTestSuite and the test file could be placed under tests/query_test. -- 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: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 18 Aug 2025 16:46:21 +0000 Gerrit-HasComments: Yes
