You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/06/04 23:15:00 UTC

[GitHub] [phoenix] abhishek-chouhan opened a new pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

abhishek-chouhan opened a new pull request #797:
URL: https://github.com/apache/phoenix/pull/797


   …ew indexes


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] gjacoby126 commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435622046



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,7 +1083,11 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
             rawScan.setRaw(true);
             rawScan.setMaxVersions();
             rawScan.getFamilyMap().clear();
-            rawScan.setFilter(null);
+            if (scan.getFilter() instanceof FirstKeyOnlyFilter) {

Review comment:
       Please add a comment to explain why FirstKeyOnlyFilter is a special case. If the rebuild index scan is explicitly asking for only the first keyvalue, why do we avoid using the AllVersions filter which also only gives the first keyvalue? 
   
   And if there is a reason not to use the FirstKeyOnlyFilter, are we still OK with using the AllVersionsIndexRebuildFilter if the Scan's filter it will delegate to is a composite filter which contains a FirstKeyOnlyFilter? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] abhishek-chouhan closed pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
abhishek-chouhan closed pull request #797:
URL: https://github.com/apache/phoenix/pull/797


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] swaroopak commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435627003



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {

Review comment:
       Please move this and other test to IndexToolForNonTxGlobalIndexIT




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] gokceni commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r436104741



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/filter/AllVersionsIndexRebuildFilter.java
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.filter;
+
+import java.io.IOException;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.filter.Filter;
+
+/**
+ * This filter overrides the behavior of delegate so that we do not jump to the next
+ * column as soon as we find a value for a column but rather include all versions which is
+ * needed for rebuilds.
+ */
+public class AllVersionsIndexRebuildFilter extends DelegateFilter {
+
+    public AllVersionsIndexRebuildFilter(Filter originalFilter) {
+        super(originalFilter);
+    }
+
+    @Override
+    public ReturnCode filterKeyValue(Cell v) throws IOException {
+        ReturnCode delegateCode = super.filterKeyValue(v);
+        if (delegateCode == ReturnCode.INCLUDE_AND_NEXT_COL) {

Review comment:
       Could you add a comment here why we convert it to INCLUDE? Why are we not happy with NEXT_COL?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] abhishek-chouhan commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
abhishek-chouhan commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435701953



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,7 +1083,11 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
             rawScan.setRaw(true);
             rawScan.setMaxVersions();
             rawScan.getFamilyMap().clear();
-            rawScan.setFilter(null);
+            if (scan.getFilter() instanceof FirstKeyOnlyFilter) {

Review comment:
       Allversions filter does not only give the first key value, its purpose is to make sure all versions of a column are returned(when matched by underlying supplied filter), instead of just one. Usually the filters used in normal queries(which also end up being used for rebuild since we use select count(*)) returns only 1 version of a column, in rebuild we want to return all versions hence this.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] kadirozde commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r437000303



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,7 +1083,11 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
             rawScan.setRaw(true);
             rawScan.setMaxVersions();
             rawScan.getFamilyMap().clear();
-            rawScan.setFilter(null);
+            if (scan.getFilter() instanceof FirstKeyOnlyFilter) {
+                rawScan.setFilter(null);
+            } else if (scan.getFilter() != null) {
+                rawScan.setFilter(new AllVersionsIndexRebuildFilter(scan.getFilter()));
+            }

Review comment:
       Yes, we get raw scan only for the old design partial rebuilds (i.e., auto-rebuilds). 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] abhishek-chouhan commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
abhishek-chouhan commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435702291



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,7 +1083,11 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
             rawScan.setRaw(true);
             rawScan.setMaxVersions();
             rawScan.getFamilyMap().clear();
-            rawScan.setFilter(null);
+            if (scan.getFilter() instanceof FirstKeyOnlyFilter) {
+                rawScan.setFilter(null);
+            } else if (scan.getFilter() != null) {
+                rawScan.setFilter(new AllVersionsIndexRebuildFilter(scan.getFilter()));
+            }

Review comment:
       AFAIK we get raw scan here in case of old design and partial rebuild (Correct me if i'm wrong here @kadirozde ). I didn't want to mess with the old design and hence only made the changes for new.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] swaroopak commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435627003



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {

Review comment:
       Please move this to IndexToolForNonTxGlobalIndexIT




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] abhishek-chouhan commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
abhishek-chouhan commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435702344



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] abhishek-chouhan commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
abhishek-chouhan commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r436145385



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/filter/AllVersionsIndexRebuildFilter.java
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.filter;
+
+import java.io.IOException;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.filter.Filter;
+
+/**
+ * This filter overrides the behavior of delegate so that we do not jump to the next
+ * column as soon as we find a value for a column but rather include all versions which is
+ * needed for rebuilds.
+ */
+public class AllVersionsIndexRebuildFilter extends DelegateFilter {
+
+    public AllVersionsIndexRebuildFilter(Filter originalFilter) {
+        super(originalFilter);
+    }
+
+    @Override
+    public ReturnCode filterKeyValue(Cell v) throws IOException {
+        ReturnCode delegateCode = super.filterKeyValue(v);
+        if (delegateCode == ReturnCode.INCLUDE_AND_NEXT_COL) {

Review comment:
       @gokceni NEXT_COL skips this column and goes to the next one. What we want to do is when the underlying filter says yes to a column, we want to say yes too, but instead of jumping to the next col since we got a value, we want to get all versions.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] abhishek-chouhan commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
abhishek-chouhan commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435700547



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'aaa'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'ccc'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {
+                Table htable = hcon.getTable(TableName.valueOf("_IDX_" + dataTableFullName));
+                Scan scan = new Scan();
+                scan.setRaw(true);
+                ResultScanner scanner = htable.getScanner(scan);
+                int numPuts = 0;
+                int numDeletes = 0;
+                for (Result result = scanner.next(); result != null; result = scanner.next()) {
+                    for (Cell cell : result.rawCells()) {
+                        if (KeyValue.Type.codeToType(cell.getTypeByte()) == KeyValue.Type.Put) {
+                            numPuts++;
+                        } else if (KeyValue.Type
+                                .codeToType(cell.getTypeByte()) == KeyValue.Type.DeleteFamily) {
+                                    numDeletes++;
+                                }
+                    }
+                }
+                assertEquals(4, numPuts);
+                assertEquals(2, numDeletes);
+            }
+        }
+    }
+
+    @Test
+    public void testUpdatableViewIndex2() throws Exception {

Review comment:
       Tried a more descriptive name in the latest commit :)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] swaroopak commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r437077087



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/filter/AllVersionsIndexRebuildFilter.java
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.filter;
+
+import java.io.IOException;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.filter.Filter;
+
+/**
+ * This filter overrides the behavior of delegate so that we do not jump to the next
+ * column as soon as we find a value for a column but rather include all versions which is
+ * needed for rebuilds.
+ */
+public class AllVersionsIndexRebuildFilter extends DelegateFilter {
+
+    public AllVersionsIndexRebuildFilter(Filter originalFilter) {
+        super(originalFilter);
+    }
+
+    @Override
+    public ReturnCode filterKeyValue(Cell v) throws IOException {
+        ReturnCode delegateCode = super.filterKeyValue(v);
+        if (delegateCode == ReturnCode.INCLUDE_AND_NEXT_COL) {

Review comment:
       Yes, I agree, comment in the code would be helpful here. @abhishek-chouhan 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] abhishek-chouhan commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
abhishek-chouhan commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435701024



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'aaa'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'ccc'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {
+                Table htable = hcon.getTable(TableName.valueOf("_IDX_" + dataTableFullName));
+                Scan scan = new Scan();
+                scan.setRaw(true);
+                ResultScanner scanner = htable.getScanner(scan);
+                int numPuts = 0;
+                int numDeletes = 0;
+                for (Result result = scanner.next(); result != null; result = scanner.next()) {
+                    for (Cell cell : result.rawCells()) {
+                        if (KeyValue.Type.codeToType(cell.getTypeByte()) == KeyValue.Type.Put) {
+                            numPuts++;
+                        } else if (KeyValue.Type
+                                .codeToType(cell.getTypeByte()) == KeyValue.Type.DeleteFamily) {
+                                    numDeletes++;
+                                }
+                    }
+                }
+                assertEquals(4, numPuts);
+                assertEquals(2, numDeletes);
+            }
+        }
+    }
+
+    @Test
+    public void testUpdatableViewIndex2() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views

Review comment:
       Done. Yes, the point of 2 tests is to test out two different filters that end up being used. One test has view on a non-leading part of pk, other one has view on a non pk column.

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'aaa'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'ccc'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {
+                Table htable = hcon.getTable(TableName.valueOf("_IDX_" + dataTableFullName));
+                Scan scan = new Scan();
+                scan.setRaw(true);
+                ResultScanner scanner = htable.getScanner(scan);
+                int numPuts = 0;
+                int numDeletes = 0;
+                for (Result result = scanner.next(); result != null; result = scanner.next()) {
+                    for (Cell cell : result.rawCells()) {
+                        if (KeyValue.Type.codeToType(cell.getTypeByte()) == KeyValue.Type.Put) {
+                            numPuts++;
+                        } else if (KeyValue.Type
+                                .codeToType(cell.getTypeByte()) == KeyValue.Type.DeleteFamily) {
+                                    numDeletes++;
+                                }
+                    }
+                }
+                assertEquals(4, numPuts);
+                assertEquals(2, numDeletes);
+            }
+        }
+    }
+
+    @Test
+    public void testUpdatableViewIndex2() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE CREATED_BY = 'foo'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE CREATED_BY = 'bar'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'aaa', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'ccc', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'ddd', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'bbb', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'ddd', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [phoenix] gjacoby126 commented on a change in pull request #797: PHOENIX-5932 View Index rebuild results in surplus rows from other vi…

Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #797:
URL: https://github.com/apache/phoenix/pull/797#discussion_r435619039



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'aaa'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'ccc'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {
+                Table htable = hcon.getTable(TableName.valueOf("_IDX_" + dataTableFullName));
+                Scan scan = new Scan();
+                scan.setRaw(true);
+                ResultScanner scanner = htable.getScanner(scan);
+                int numPuts = 0;
+                int numDeletes = 0;
+                for (Result result = scanner.next(); result != null; result = scanner.next()) {
+                    for (Cell cell : result.rawCells()) {
+                        if (KeyValue.Type.codeToType(cell.getTypeByte()) == KeyValue.Type.Put) {

Review comment:
       Note for when you port to master: in HBase 2.x Cell has a getType() method that returns the Cell.Type enum. We should avoid using KeyValue wherever possible because it's IA.Private. 

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'aaa'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'ccc'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {
+                Table htable = hcon.getTable(TableName.valueOf("_IDX_" + dataTableFullName));
+                Scan scan = new Scan();
+                scan.setRaw(true);
+                ResultScanner scanner = htable.getScanner(scan);
+                int numPuts = 0;
+                int numDeletes = 0;
+                for (Result result = scanner.next(); result != null; result = scanner.next()) {
+                    for (Cell cell : result.rawCells()) {
+                        if (KeyValue.Type.codeToType(cell.getTypeByte()) == KeyValue.Type.Put) {
+                            numPuts++;
+                        } else if (KeyValue.Type
+                                .codeToType(cell.getTypeByte()) == KeyValue.Type.DeleteFamily) {
+                                    numDeletes++;
+                                }
+                    }
+                }
+                assertEquals(4, numPuts);
+                assertEquals(2, numDeletes);
+            }
+        }
+    }
+
+    @Test
+    public void testUpdatableViewIndex2() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views

Review comment:
       Good to have a comment explaining what about the schemas is important to the test (I assume that they're filtering on a non-PK, non-indexed column?

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,7 +1083,11 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
             rawScan.setRaw(true);
             rawScan.setMaxVersions();
             rawScan.getFamilyMap().clear();
-            rawScan.setFilter(null);
+            if (scan.getFilter() instanceof FirstKeyOnlyFilter) {
+                rawScan.setFilter(null);
+            } else if (scan.getFilter() != null) {
+                rawScan.setFilter(new AllVersionsIndexRebuildFilter(scan.getFilter()));
+            }

Review comment:
       Do we need to do any special filter logic down at line 1099 in the else block if the Scan was raw in the first place? 

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/filter/AllVersionsIndexRebuildFilter.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.filter;
+
+import java.io.IOException;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.filter.Filter;
+
+public class AllVersionsIndexRebuildFilter extends DelegateFilter {
+
+    public AllVersionsIndexRebuildFilter(Filter originalFilter) {
+        super(originalFilter);
+    }
+
+    @Override
+    public ReturnCode filterKeyValue(Cell v) throws IOException {
+        ReturnCode delegateCode = super.filterKeyValue(v);
+        if (delegateCode == ReturnCode.INCLUDE_AND_NEXT_COL) {
+            return ReturnCode.INCLUDE;

Review comment:
       This is simulating the effects of a FirstKeyOnlyFilter? Comment would be good

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,7 +1083,11 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
             rawScan.setRaw(true);
             rawScan.setMaxVersions();
             rawScan.getFamilyMap().clear();
-            rawScan.setFilter(null);
+            if (scan.getFilter() instanceof FirstKeyOnlyFilter) {

Review comment:
       Please add a comment to explain why FirstKeyOnlyFilter is a special case. If the rebuild index scan is explicitly asking for only the first key, why do we avoid using the AllVersions filter which also only gives the first key? 
   
   And if there is a reason not to use the FirstKeyOnlyFilter, are we still OK with using the AllVersionsIndexRebuildFilter if the Scan's filter it will delegate to is a composite filter which contains a FirstKeyOnlyFilter? 

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'aaa'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'ccc'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {
+                Table htable = hcon.getTable(TableName.valueOf("_IDX_" + dataTableFullName));
+                Scan scan = new Scan();
+                scan.setRaw(true);
+                ResultScanner scanner = htable.getScanner(scan);
+                int numPuts = 0;
+                int numDeletes = 0;
+                for (Result result = scanner.next(); result != null; result = scanner.next()) {
+                    for (Cell cell : result.rawCells()) {
+                        if (KeyValue.Type.codeToType(cell.getTypeByte()) == KeyValue.Type.Put) {
+                            numPuts++;
+                        } else if (KeyValue.Type
+                                .codeToType(cell.getTypeByte()) == KeyValue.Type.DeleteFamily) {
+                                    numDeletes++;
+                                }
+                    }
+                }
+                assertEquals(4, numPuts);
+                assertEquals(2, numDeletes);
+            }
+        }
+    }
+
+    @Test
+    public void testUpdatableViewIndex2() throws Exception {

Review comment:
       More descriptive name, please. 

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java
##########
@@ -208,6 +209,195 @@ protected static void setEveryNthRowWithNull(int nrows, int nthRowNull, Prepared
         }
     }
 
+    @Test
+    public void testUpdatableViewIndex() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'aaa'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE KEY_PREFIX = 'ccc'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {
+                Table htable = hcon.getTable(TableName.valueOf("_IDX_" + dataTableFullName));
+                Scan scan = new Scan();
+                scan.setRaw(true);
+                ResultScanner scanner = htable.getScanner(scan);
+                int numPuts = 0;
+                int numDeletes = 0;
+                for (Result result = scanner.next(); result != null; result = scanner.next()) {
+                    for (Cell cell : result.rawCells()) {
+                        if (KeyValue.Type.codeToType(cell.getTypeByte()) == KeyValue.Type.Put) {
+                            numPuts++;
+                        } else if (KeyValue.Type
+                                .codeToType(cell.getTypeByte()) == KeyValue.Type.DeleteFamily) {
+                                    numDeletes++;
+                                }
+                    }
+                }
+                assertEquals(4, numPuts);
+                assertEquals(2, numDeletes);
+            }
+        }
+    }
+
+    @Test
+    public void testUpdatableViewIndex2() throws Exception {
+        if (!mutable || transactional || localIndex || useTenantId) {
+            return;
+        }
+        String schemaName = generateUniqueName();
+        String dataTableName = generateUniqueName();
+        String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+        String view1Name = generateUniqueName();
+        String view1FullName = SchemaUtil.getTableName(schemaName, view1Name);
+        String view2Name = generateUniqueName();
+        String view2FullName = SchemaUtil.getTableName(schemaName, view2Name);
+        String indexTableName = generateUniqueName();
+        String indexTableFullName = SchemaUtil.getTableName(schemaName, indexTableName);
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+
+        try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+            // Create Table and Views
+            String createTable =
+                    "CREATE TABLE IF NOT EXISTS " + dataTableFullName + " (\n"
+                            + "    ORGANIZATION_ID VARCHAR NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n" + "    CREATED_BY VARCHAR,\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n" + "        ORGANIZATION_ID,\n"
+                            + "        KEY_PREFIX\n" + "    )\n"
+                            + ") VERSIONS=1, COLUMN_ENCODED_BYTES=0";
+            conn.createStatement().execute(createTable);
+            String createView1 =
+                    "CREATE VIEW IF NOT EXISTS " + view1FullName + " (\n"
+                            + " VIEW_COLA VARCHAR NOT NULL,\n"
+                            + " VIEW_COLB CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COLA\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE CREATED_BY = 'foo'";
+            conn.createStatement().execute(createView1);
+            String createView2 =
+                    "CREATE VIEW IF NOT EXISTS " + view2FullName + " (\n"
+                            + " VIEW_COL1 VARCHAR NOT NULL,\n"
+                            + " VIEW_COL2 CHAR(1) CONSTRAINT PKVIEW PRIMARY KEY (\n"
+                            + " VIEW_COL1\n" + " )) AS \n" + " SELECT * FROM " + dataTableFullName
+                            + " WHERE CREATED_BY = 'bar'";
+            conn.createStatement().execute(createView2);
+
+            // We want to verify if deletes and set null result in expected rebuild of view index
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'aaa', 'A', 'G')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'ccc', 'C', 'I')");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'ddd', 'D', 'J')");
+
+            conn.createStatement().execute("UPSERT INTO " + view2FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COL1, VIEW_COL2) VALUES('ORG2', 'bbb', 'B', 'H')");
+            conn.commit();
+            conn.createStatement().execute("DELETE FROM " + view1FullName
+                    + " WHERE ORGANIZATION_ID = 'ORG1' AND VIEW_COLA = 'C'");
+            conn.createStatement().execute("UPSERT INTO " + view1FullName
+                    + "(ORGANIZATION_ID, KEY_PREFIX, VIEW_COLA, VIEW_COLB) VALUES('ORG1', 'ddd', 'D', NULL)");
+            conn.commit();
+
+            String createViewIndex =
+                    "CREATE INDEX IF NOT EXISTS " + indexTableName + " ON " + view1FullName
+                            + " (VIEW_COLB) ASYNC";
+            conn.createStatement().execute(createViewIndex);
+            conn.commit();
+            // Rebuild using index tool
+            runIndexTool(directApi, useSnapshot, schemaName, view1Name, indexTableName);
+            ResultSet rs =
+                    conn.createStatement()
+                            .executeQuery("SELECT COUNT(*) FROM " + indexTableFullName);
+            rs.next();
+            assertEquals(2, rs.getInt(1));
+            try (org.apache.hadoop.hbase.client.Connection hcon =
+                    ConnectionFactory.createConnection(config)) {

Review comment:
       You can factor out the cell counting into its own helper function to avoid duplication between the 2 tests. (TestUtil.getRawCellCount may also be useful if you can extend it to also keep track of what Cell types are scanned.) 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org