Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/23895 )
Change subject: [WIP] Introduce TableSchema for FeTable implementations ...................................................................... Patch Set 1: (62 comments) gerrit-auto-critic failed. You can reproduce it locally using command: python3 bin/jenkins/critique-gerrit-review.py --dryrun To run it, you might need a virtual env with Python3's venv installed. http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/analysis/AlterTableDropColStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableDropColStmt.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/analysis/AlterTableDropColStmt.java@75 PS1, Line 75: if (t.getSchema().getColumns().size() - t.getMetaStoreTable().getPartitionKeysSize() <= 1) { line too long (96 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@740 PS1, Line 740: table_.getSchema().getColumns().size(), partitionClause, totalColumnsMentioned)); line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java File fe/src/main/java/org/apache/impala/catalog/FeFsTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@462 PS1, Line 462: * TODO: this does schema related stuff too! getSchema().getColumns() and getNumClusteringCols are both schema methods line too long (120 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/FeTable.java File fe/src/main/java/org/apache/impala/catalog/FeTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/FeTable.java@121 PS1, Line 121: line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java File fe/src/main/java/org/apache/impala/catalog/HBaseTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java@172 PS1, Line 172: new TTableDescriptor(tableId, TTableType.HBASE_TABLE, getSchema().toTColumnDescriptors(), line too long (97 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1001 PS1, Line 1001: if (partition.getPartitionValues().size() != getSchema().getNumClusteringCols()) return; line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1927 PS1, Line 1927: nonPartFieldSchemas_.size() + getSchema().getNumClusteringCols(), getSchema().getColumns().size()); line too long (107 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2143 PS1, Line 2143: getSchema().toTColumnDescriptors(), getSchema().getNumClusteringCols(), name_, db_.getName()); line too long (102 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java File fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@37 PS1, Line 37: * metadata may need to be updated. If 'partitionsToUpdate' is not null, it specifies a line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@48 PS1, Line 48: * Metastore without reloading the file metadata(this is done in later steps) to ensure line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@57 PS1, Line 57: // public void load(Table table, HdfsTableLoadParams loadParams) throws TableLoadingException { line too long (98 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@64 PS1, Line 64: // String annotation = String.format("%s metadata for %s%s partition(s) of %s.%s (%s)", line too long (94 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@67 PS1, Line 67: // partitionsToUpdate == null ? "all" : String.valueOf(partitionsToUpdate.size()), line too long (97 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@90 PS1, Line 90: // LOG.info("Not skipping file metadata reload since writeId is changed in the " + line too long (101 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@100 PS1, Line 100: // loadParams.isLoadPartitionFileMetadata(), "Conflicts in " + line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@105 PS1, Line 105: // if (loadParams.isLoadPartitionFileMetadata() || prevWriteIdChanged) { line too long (95 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@109 PS1, Line 109: // } else { // Update the single partition stats in case table stats changes. line too long (101 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@114 PS1, Line 114: // partitionsToUpdate, loadParams.isLoadPartitionFileMetadata(), line too long (95 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@116 PS1, Line 116: // loadParams.getPartitionToEventId(), loadParams.getDebugAction(), line too long (98 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@119 PS1, Line 119: // LOG.info("Incrementally loaded table metadata for: " + getTableName()); line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@121 PS1, Line 121: // LOG.info("Fetching partition metadata from the Metastore: " + getTableName()); line too long (100 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@123 PS1, Line 123: // getMetrics().getTimer(HdfsTable.LOAD_DURATION_ALL_PARTITIONS).time(); line too long (99 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@128 PS1, Line 128: // LOG.info("Fetched partition metadata from the Metastore: " + getTableName()); line too long (99 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@150 PS1, Line 150: // LOG.warn("Time taken on loading table " + getTableName() + " exceeded " + line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@151 PS1, Line 151: // "warning threshold. Time: " + PrintUtils.printTimeNs(load_time_duration)); line too long (100 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@159 PS1, Line 159: // * Load Primary Key and Foreign Key information for table. Throws TableLoadingException line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@164 PS1, Line 164: // org.apache.hadoop.hive.metastore.api.Table msTbl) throws TableLoadingException{ line too long (120 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@171 PS1, Line 171: // throw new TableLoadingException("Failed to load primary keys/foreign keys for " line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@183 PS1, Line 183: // protected long updateMdFromHmsTable(org.apache.hadoop.hive.metastore.api.Table msTbl) line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@206 PS1, Line 206: // private long updateUnpartitionedTableFileMd(IMetaStoreClient client, String debugAction, line too long (94 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@207 PS1, Line 207: // EventSequence catalogTimeline) throws CatalogException { line too long (106 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@223 PS1, Line 223: // // Keep track of the previous partition id, so we can send invalidation on the old line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@227 PS1, Line 227: // ImmutableList.of(partBuilder), /*isRefresh=*/true, debugAction, catalogTimeline); line too long (99 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@261 PS1, Line 261: // Set<String> partitionsToUpdate, boolean loadPartitionFileMetadata, line too long (109 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@262 PS1, Line 262: // boolean refreshUpdatedPartitions, Map<String, Long> partitionToEventId, line too long (114 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@263 PS1, Line 263: // String debugAction, EventSequence catalogTimeline, boolean isPreLoadForInsert) line too long (121 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@270 PS1, Line 270: // Preconditions.checkState(loadPartitionFileMetadata || partitionsToUpdate == null); line too long (96 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@278 PS1, Line 278: // deltaUpdater = new HdfsTable.PartNameBasedDeltaUpdater(client, loadPartitionFileMetadata, line too long (103 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/HdfsTableOperations.java@279 PS1, Line 279: // partitionsToUpdate, partitionToEventId, debugAction, catalogTimeline); line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java@90 PS1, Line 90: desc.hdfsTable.setAvroSchema(AvroSchemaConverter.convertColumns(getSchema().getColumns(), line too long (97 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java@57 PS1, Line 57: tableSchema_ = new TableSchema(Arrays.asList(filePath, pos), Collections.emptyList(),0); line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@783 PS1, Line 783: getSchema().toTColumnDescriptors(), getSchema().getNumClusteringCols(), name_, db_.getName()); line too long (102 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergTimeTravelTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTimeTravelTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/IcebergTimeTravelTable.java@110 PS1, Line 110: schema_ = new TableSchema(IcebergSchemaConverter.convertToImpalaSchema(icebergSchema), line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@505 PS1, Line 505: getSchema().toTColumnDescriptors(), getSchema().getNumClusteringCols(), name_, db_.getName()); line too long (102 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/Table.java@783 PS1, Line 783: List<String> colList = selector.want_stats_for_all_columns ? schema.getColumnNames() : line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/Table.java@849 PS1, Line 849: public List<VirtualColumn> getVirtualColumns() { return getSchema().getVirtualColumns(); } line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java File fe/src/main/java/org/apache/impala/catalog/TableSchema.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@69 PS1, Line 69: org.apache.hadoop.hive.metastore.api.Table msTbl) throws TableLoadingException { line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@118 PS1, Line 118: public TableSchema(List<? extends Column> columns, List<VirtualColumn> virtualColumns, int numClusteringCols) { line too long (115 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@187 PS1, Line 187: * Sets the number of clustering columns. This method should only be used for tests and line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@229 PS1, Line 229: columnDesc.getIceberg_field_id(), columnDesc.getIceberg_field_map_key_id(), line too long (95 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@230 PS1, Line 230: columnDesc.getIceberg_field_map_value_id(), columnDesc.isIs_nullable()); line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@236 PS1, Line 236: // HBase table column. The HBase column qualifier (column name) is not be set for line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@237 PS1, Line 237: // the HBase row key, so it being set in the thrift struct is not a precondition. line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/TableSchema.java@240 PS1, Line 240: col = new HBaseColumn(columnDesc.getColumnName(), columnDesc.getColumn_family(), line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java@130 PS1, Line 130: getSchema().toTColumnDescriptors(), getSchema().getNumClusteringCols(), name_, db_.getName()); line too long (102 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/local/LocalPaimonTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalPaimonTable.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/catalog/local/LocalPaimonTable.java@79 PS1, Line 79: TTableType.PAIMON_TABLE, getSchema().toTColumnDescriptors(), 0, name_, db_.getName()); line too long (94 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/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/23895/1/fe/src/main/java/org/apache/impala/catalog/paimon/PaimonTable.java@79 PS1, Line 79: new TTableDescriptor(tableId, TTableType.PAIMON_TABLE, getSchema().toTColumnDescriptors(), line too long (98 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@525 PS1, Line 525: rangePartitioningKuduColNames.add(((KuduColumn)tbl.getSchema().getColumn(colName)).getKuduName()); line too long (104 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@3180 PS1, Line 3180: decimalTbl.getSchema().getColumns().size() + dateTbl.getSchema().getColumns().size()); line too long (94 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/test/java/org/apache/impala/catalog/TableSchemaTest.java File fe/src/test/java/org/apache/impala/catalog/TableSchemaTest.java: http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/test/java/org/apache/impala/catalog/TableSchemaTest.java@71 PS1, Line 71: TColumn virtualColumn = new TColumn(VirtualColumn.INPUT_FILE_NAME.getName(), Type.STRING.toThrift()); line too long (109 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/test/java/org/apache/impala/catalog/TableSchemaTest.java@131 PS1, Line 131: private void assertColumns(List<? extends Column> expected, List<? extends Column> actual) { line too long (96 > 90) http://gerrit.cloudera.org:8080/#/c/23895/1/fe/src/test/java/org/apache/impala/catalog/TableSchemaTest.java@143 PS1, Line 143: assertEquals(expectedVColumn.getVirtualColumnType(), actualVColumn.getVirtualColumnType()); line too long (107 > 90) -- To view, visit http://gerrit.cloudera.org:8080/23895 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief3ddab35f57f75091112288157710a2776a7122 Gerrit-Change-Number: 23895 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 23 Jan 2026 12:35:39 +0000 Gerrit-HasComments: Yes
