This is an automated email from the ASF dual-hosted git repository. stigahuang pushed a commit to branch branch-4.0.1 in repository https://gitbox.apache.org/repos/asf/impala.git
commit ce433ba42e8816217212de08ab5b6552b3cb9280 Author: stiga-huang <[email protected]> AuthorDate: Thu Sep 30 10:39:26 2021 +0800 IMPALA-10936: Fix NPE in collecting policy tables on FailedLoadLocalTable Ranger column-masking/row-filtering policies can have complex expressions that reference other tables (e.g. by using subqueries). So when loading metadata of a table, we also need to load all tables referenced by its related policies (i.e. "policy tables"). This requires getting all its columns so we can get the column-masking policies. However, for a failed loaded table in LocalCatalog mode, the table is represented by a FailedLoadLocalTable which has a null ColumnMap. This causes a NullPointerException when collecting policy tables for it. This patch skips collecting policy tables on failed loaded tables. Also add some null checks in LocalTable in case its ColumnMap is null. So FailedLoadLocalTable can have the same robustness as IncompleteTable which is the failed table representation in the legacy catalog mode. Tests: - Manually verified the NPE disappear in logs - Added FE unit test Change-Id: I7a6cd567685920211f05f2fdaac96bc98da41ac6 Reviewed-on: http://gerrit.cloudera.org:8080/17888 Reviewed-by: Joe McDonnell <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- .../apache/impala/analysis/StmtMetadataLoader.java | 9 ++++++-- .../apache/impala/catalog/local/LocalTable.java | 18 ++++++++-------- .../impala/analysis/StmtMetadataLoaderTest.java | 25 +++++++++++++++++++++- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java b/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java index fa8ecc4..5ed2f3b 100644 --- a/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java +++ b/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java @@ -18,6 +18,7 @@ package org.apache.impala.analysis; import java.util.ArrayList; +import java.util.Collections; import java.util.concurrent.TimeUnit; import java.util.HashMap; import java.util.HashSet; @@ -44,6 +45,7 @@ import org.apache.impala.util.TUniqueIdUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; /** @@ -315,7 +317,8 @@ public class StmtMetadataLoader { viewTbls.addAll(collectTableCandidates(((FeView) tbl).getQueryStmt())); } // Adds tables/views introduced by column-masking/row-filtering policies. - if (fe_.getAuthzFactory().getAuthorizationConfig().isEnabled() + if (!(tbl instanceof FeIncompleteTable) + && fe_.getAuthzFactory().getAuthorizationConfig().isEnabled() && fe_.getAuthzFactory().supportsTableMasking() && user_ != null) { try { viewTbls.addAll(collectPolicyTables(tbl)); @@ -346,8 +349,10 @@ public class StmtMetadataLoader { return tableNames; } - private Set<TableName> collectPolicyTables(FeTable tbl) + @VisibleForTesting + Set<TableName> collectPolicyTables(FeTable tbl) throws InternalException, AnalysisException { + if (tbl instanceof FeIncompleteTable) return Collections.emptySet(); Set<TableName> tableNames = new HashSet<>(); String dbName = tbl.getDb().getName(); String tblName = tbl.getName(); diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java index d89b960..44928a8 100644 --- a/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java +++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java @@ -18,6 +18,7 @@ package org.apache.impala.catalog.local; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import javax.annotation.Nullable; @@ -210,8 +211,7 @@ abstract class LocalTable implements FeTable { @Override public List<Column> getColumns() { - // TODO(todd) why does this return ArrayList instead of List? - return new ArrayList<>(cols_.colsByPos_); + return cols_ == null ? Collections.emptyList() : cols_.colsByPos_; } @Override @@ -228,37 +228,37 @@ abstract class LocalTable implements FeTable { @Override public List<String> getColumnNames() { - return cols_.getColumnNames(); + return cols_ == null ? Collections.emptyList() : cols_.getColumnNames(); } @Override public List<Column> getClusteringColumns() { - return cols_.getClusteringColumns(); + return cols_ == null ? Collections.emptyList() : cols_.getClusteringColumns(); } @Override public List<Column> getNonClusteringColumns() { - return cols_.getNonClusteringColumns(); + return cols_ == null ? Collections.emptyList() : cols_.getNonClusteringColumns(); } @Override public int getNumClusteringCols() { - return cols_.getNumClusteringCols(); + return cols_ == null ? 0 : cols_.getNumClusteringCols(); } @Override public boolean isClusteringColumn(Column c) { - return cols_.isClusteringColumn(c); + return cols_ != null && cols_.isClusteringColumn(c); } @Override public Column getColumn(String name) { - return cols_.getByName(name); + return cols_ == null ? null : cols_.getByName(name); } @Override public ArrayType getType() { - return cols_.getType(); + return cols_ == null ? null : cols_.getType(); } @Override diff --git a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java index 730a02e..8a50b8e 100644 --- a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java @@ -21,17 +21,24 @@ import java.util.Arrays; import org.apache.impala.analysis.StmtMetadataLoader.StmtTableCache; import org.apache.impala.authorization.NoopAuthorizationFactory; +import org.apache.impala.authorization.User; import org.apache.impala.catalog.Catalog; import org.apache.impala.catalog.FeTable; +import org.apache.impala.catalog.TableLoadingException; +import org.apache.impala.catalog.local.CatalogdMetaProvider; +import org.apache.impala.catalog.local.FailedLoadLocalTable; +import org.apache.impala.catalog.local.LocalCatalog; +import org.apache.impala.catalog.local.LocalDb; import org.apache.impala.common.ImpalaException; import org.apache.impala.common.InternalException; import org.apache.impala.compat.MetastoreShim; +import org.apache.impala.service.BackendConfig; +import org.apache.impala.service.FeSupport; import org.apache.impala.service.Frontend; import org.apache.impala.testutil.ImpaladTestCatalog; import org.apache.impala.util.EventSequence; import org.junit.Assert; import org.junit.Assume; -import org.junit.Ignore; import org.junit.Test; public class StmtMetadataLoaderTest { @@ -232,4 +239,20 @@ public class StmtMetadataLoaderTest { Assume.assumeTrue(MetastoreShim.getMajorVersion() >= 3); testLoadAcidTables("select * from functional.insert_only_transactional_table"); } + + @Test + public void testCollectPolicyTablesOnFailedTables() throws ImpalaException { + FeSupport.loadLibrary(); + CatalogdMetaProvider provider = new CatalogdMetaProvider( + BackendConfig.INSTANCE.getBackendCfg()); + LocalCatalog catalog = new LocalCatalog(provider, /*defaultKuduMasterHosts=*/null); + Frontend fe = new Frontend(new NoopAuthorizationFactory(), catalog); + EventSequence timeline = new EventSequence("Test Timeline"); + User user = new User("user"); + StmtMetadataLoader mdLoader = + new StmtMetadataLoader(fe, Catalog.DEFAULT_DB, timeline, user); + LocalDb db = new LocalDb(catalog, "default"); + mdLoader.collectPolicyTables(new FailedLoadLocalTable( + db, "tbl", new TableLoadingException("error", /*cause=*/new Exception()))); + } }
