This is an automated email from the ASF dual-hosted git repository. stigahuang pushed a commit to branch branch-4.4.1 in repository https://gitbox.apache.org/repos/asf/impala.git
commit 65ee0ffeaa6d283fcb952d36104f5400e45786ce Author: Daniel Becker <[email protected]> AuthorDate: Fri Apr 26 15:33:24 2024 +0200 IMPALA-13035: Querying metadata tables from non-Iceberg tables throws IllegalArgumentException When attempting to query a metadata table of a non-Iceberg table the analyzer throws 'IllegalArgumentException'. The problem is that 'IcebergMetadataTable.isIcebergMetadataTable()' doesn't actually check whether the given path belongs to a valid metadata table, it only checks whether the path could syntactically refer to one. This is because it is called in 'Path.getCandidateTables()', at which point analysis has not been done yet. However, 'IcebergMetadataTable.isIcebergMetadataTable()' is also called in 'Analyzer.getTable()'. If 'isIcebergMetadataTable()' returns true, 'getTable()' tries to instantiate an 'IcebergMetadataTable' object with the table ref of the base table. If that table is not an Iceberg table, a precondition check fails. This change renames 'isIcebergMetadataTable()' to 'canBeIcebergMetadataTable()' and adds a new 'isIcebergMetadataTable()' function, which also takes an 'Analyzer' as a parameter. With the help of the 'Analyzer' it is possible to determine whether the base table is an Iceberg table. 'Analyzer.getTable()' then uses this new 'isIcebergMetadataTable()' function instead of canBeIcebergMetadataTable(). The constructor of 'IcebergMetadataTable' is also modified to take an 'FeIcebergTable' as the parameter for the base table instead of a general 'FeTable'. Testing: - Added a test query in iceberg-metadata-tables.test. Change-Id: Ia7c25ed85a8813011537c73f0aaf72db1501f9ef Reviewed-on: http://gerrit.cloudera.org:8080/21361 Tested-by: Impala Public Jenkins <[email protected]> Reviewed-by: Peter Rozsa <[email protected]> --- .../java/org/apache/impala/analysis/Analyzer.java | 9 ++++-- .../main/java/org/apache/impala/analysis/Path.java | 2 +- .../catalog/iceberg/IcebergMetadataTable.java | 32 +++++++++++++++++++--- .../impala/service/DescribeResultFactory.java | 6 ++-- .../java/org/apache/impala/service/Frontend.java | 4 +-- .../queries/QueryTest/iceberg-metadata-tables.test | 15 ++++++++-- 6 files changed, 52 insertions(+), 16 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java index 15be58a8b..9dd55bea6 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java +++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java @@ -3669,7 +3669,7 @@ public class Analyzer { */ public FeTable getTable(TableName tblName, boolean mustExist) throws AnalysisException, TableLoadingException { - if (IcebergMetadataTable.isIcebergMetadataTable(tblName.toPath())) { + if (IcebergMetadataTable.isIcebergMetadataTable(tblName.toPath(), this)) { return getMetadataVirtualTable(tblName.toPath()); } FeTable table = globalState_.stmtTableCache.tables.get(tblName); @@ -3721,7 +3721,8 @@ public class Analyzer { */ public FeTable getMetadataVirtualTable(List<String> tblRefPath) throws AnalysisException { - Preconditions.checkArgument(IcebergMetadataTable.isIcebergMetadataTable(tblRefPath)); + Preconditions.checkArgument( + IcebergMetadataTable.canBeIcebergMetadataTable(tblRefPath)); try { TableName catalogTableName = new TableName(tblRefPath.get(0), tblRefPath.get(1)); @@ -3733,8 +3734,10 @@ public class Analyzer { if (catalogTable instanceof IcebergMetadataTable || catalogTable == null) { return catalogTable; } + Preconditions.checkState(catalogTable instanceof FeIcebergTable); + FeIcebergTable catalogIceTable = (FeIcebergTable) catalogTable; IcebergMetadataTable virtualTable = - new IcebergMetadataTable(catalogTable, tblRefPath.get(2)); + new IcebergMetadataTable(catalogIceTable, tblRefPath.get(2)); getStmtTableCache().tables.put(catalogTableName, catalogTable); getStmtTableCache().tables.put(virtualTableName, virtualTable); return virtualTable; diff --git a/fe/src/main/java/org/apache/impala/analysis/Path.java b/fe/src/main/java/org/apache/impala/analysis/Path.java index 5083527c6..a78f6b1d4 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Path.java +++ b/fe/src/main/java/org/apache/impala/analysis/Path.java @@ -350,7 +350,7 @@ public class Path { String dbName = (tblNameIdx == 0) ? sessionDb : path.get(0); String tblName = path.get(tblNameIdx); String vTblName = null; - if (IcebergMetadataTable.isIcebergMetadataTable(path)) { + if (IcebergMetadataTable.canBeIcebergMetadataTable(path)) { vTblName = path.get(2); } result.add(new TableName(dbName, tblName, vTblName)); diff --git a/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java b/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java index 3d417eef9..ce3f333f3 100644 --- a/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java +++ b/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java @@ -25,6 +25,7 @@ import org.apache.iceberg.MetadataTableType; import org.apache.iceberg.MetadataTableUtils; import org.apache.iceberg.Schema; import org.apache.iceberg.Table; +import org.apache.impala.analysis.Analyzer; import org.apache.impala.analysis.TableName; import org.apache.impala.catalog.CatalogObject.ThriftObjectType; import org.apache.impala.catalog.Column; @@ -59,11 +60,10 @@ public class IcebergMetadataTable extends VirtualTable { // Name of the metadata table. private String metadataTableName_; - public IcebergMetadataTable(FeTable baseTable, String metadataTableTypeStr) + public IcebergMetadataTable(FeIcebergTable baseTable, String metadataTableTypeStr) throws ImpalaRuntimeException { super(null, baseTable.getDb(), baseTable.getName(), baseTable.getOwnerUser()); - Preconditions.checkArgument(baseTable instanceof FeIcebergTable); - baseTable_ = (FeIcebergTable) baseTable; + baseTable_ = baseTable; metadataTableName_ = metadataTableTypeStr.toUpperCase(); MetadataTableType type = MetadataTableType.from(metadataTableTypeStr.toUpperCase()); Preconditions.checkNotNull(type); @@ -136,7 +136,31 @@ public class IcebergMetadataTable extends VirtualTable { /** * Returns true if the table ref is referring to a valid metadata table. */ - public static boolean isIcebergMetadataTable(List<String> tblRefPath) { + public static boolean isIcebergMetadataTable(List<String> tblRefPath, + Analyzer analyzer) { + if (!canBeIcebergMetadataTable(tblRefPath)) return false; + + TableName virtualTableName = new TableName(tblRefPath.get(0), + tblRefPath.get(1), tblRefPath.get(2)); + // The catalog table (the base of the virtual table) has been loaded and cached + // under the name of the virtual table. + FeTable catalogTable = analyzer.getStmtTableCache().tables.get(virtualTableName); + // If the metadata table has already been analyzed in the query, the table cache will + // return the virtual table, not the base table. + return catalogTable instanceof FeIcebergTable || + catalogTable instanceof IcebergMetadataTable; + } + + /** + * Returns true if the path could refer to an Iceberg metadata table in a syntactically + * correct way (also checking that the name of the metadata table is valid). Does not + * check whether the base table is an Iceberg table, so the path is not guaranteed to + * actually refer to a valid Iceberg metadata table. + * + * This function can be called before analysis is done, when isIcebergMetadataTable() + * cannot be called. + */ + public static boolean canBeIcebergMetadataTable(List<String> tblRefPath) { if (tblRefPath == null) return false; if (tblRefPath.size() < 3) return false; String vTableName = tblRefPath.get(2).toUpperCase(); diff --git a/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java b/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java index 6ce58e5c1..8880da3d4 100644 --- a/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java +++ b/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java @@ -362,9 +362,9 @@ public class DescribeResultFactory { * it is simpler to re-create this object than to extract those from a new * org.apache.iceberg.Table object or to send it over. */ - public static TDescribeResult buildIcebergMetadataDescribeMinimalResult(FeTable table, - String vTableName) throws ImpalaRuntimeException { + public static TDescribeResult buildIcebergMetadataDescribeMinimalResult( + FeIcebergTable table, String vTableName) throws ImpalaRuntimeException { IcebergMetadataTable metadataTable = new IcebergMetadataTable(table, vTableName); return buildIcebergDescribeMinimalResult(metadataTable.getColumns()); } -} \ No newline at end of file +} diff --git a/fe/src/main/java/org/apache/impala/service/Frontend.java b/fe/src/main/java/org/apache/impala/service/Frontend.java index 90f8b233c..d8e56eecc 100644 --- a/fe/src/main/java/org/apache/impala/service/Frontend.java +++ b/fe/src/main/java/org/apache/impala/service/Frontend.java @@ -1834,8 +1834,8 @@ public class Frontend { return DescribeResultFactory.buildIcebergDescribeMinimalResult(filteredColumns); } else { Preconditions.checkArgument(vTableName != null); - return DescribeResultFactory.buildIcebergMetadataDescribeMinimalResult(table, - vTableName); + return DescribeResultFactory.buildIcebergMetadataDescribeMinimalResult( + (FeIcebergTable) table, vTableName); } } else { return DescribeResultFactory.buildDescribeMinimalResult( diff --git a/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test b/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test index 50c3fe2ea..cfd278228 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test +++ b/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test @@ -173,7 +173,7 @@ select * from $DATABASE.empty_ice_tbl.entries; INT,BIGINT,BIGINT,BIGINT,STRING,STRING #### -# Test 2 : Test select list +# Test select list #### ==== ---- QUERY @@ -401,7 +401,7 @@ AnalysisException: FOR SYSTEM_VERSION AS OF clause is only supported for Iceberg ==== #### -# Test 9 : Use-cases +# Use-cases #### ==== ---- QUERY @@ -430,11 +430,20 @@ row_regex:[1-9]\d*|0,'$NAMENODE/test-warehouse/iceberg_test/hadoop_catalog/ice/i ---- TYPES INT,STRING,BIGINT +#### +# Test querying a metadata table of a non-Iceberg table. +#### +==== +---- QUERY +select * from functional_parquet.alltypes.`files`; +---- CATCH +AnalysisException: Could not resolve table reference: 'functional_parquet.alltypes.files' +==== + #### # Invalid operations # In most cases the parser catches the table reference. #### -==== ---- QUERY describe formatted functional_parquet.iceberg_query_metadata.snapshots; ---- CATCH
