[impala] branch master updated: IMPALA-10936: Fix NPE in collecting policy tables on FailedLoadLocalTable

[jo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git


The following commit(s) were added to refs/heads/master by this push:
     new 4b8bb04  IMPALA-10936: Fix NPE in collecting policy tables on FailedLoadLocalTable
4b8bb04 is described below

commit 4b8bb041b95529a8147981e7ca2c392e07611b2f
Author: stiga-huang <hu...@gmail.com>
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 <jo...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../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())));
+  }
 }