gjacoby126 commented on code in PR #1531:
URL: https://github.com/apache/phoenix/pull/1531#discussion_r1065081919
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/UncoveredGlobalIndexRegionScannerIT.java:
##########
@@ -80,7 +93,39 @@ private void populateTable(String tableName) throws
Exception {
}
@Test
- public void testUncoveredIndexWithPhoenixRowTimestamp() throws Exception {
+ public void testDDL() throws Exception {
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ String dataTableName = generateUniqueName();
+ String indexTableName = generateUniqueName();
+ conn.createStatement().execute("create table " + dataTableName +
+ " (id varchar(10) not null primary key, val1 varchar(10),
val2 varchar(10), val3 varchar(10))");
+ if (uncovered) {
+ // The INCLUDE clause should not be allowed
+ try {
+ conn.createStatement().execute("CREATE UNCOVERED INDEX " +
indexTableName
+ + " on " + dataTableName + " (val1) INCLUDE
(val2)");
+ Assert.fail();
+ } catch (Exception e) {
+ // Expected
+ }
+ // The LOCAL keyword should not be allowed with UNCOVERED
+ try {
+ conn.createStatement().execute("CREATE UNCOVERED LOCAL
INDEX " + indexTableName
Review Comment:
What's wrong with an UNCOVERED local? They were implicitly allowed even
before this JIRA, weren't they?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/UncoveredGlobalIndexRegionScannerIT.java:
##########
@@ -80,7 +93,39 @@ private void populateTable(String tableName) throws
Exception {
}
@Test
- public void testUncoveredIndexWithPhoenixRowTimestamp() throws Exception {
+ public void testDDL() throws Exception {
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ String dataTableName = generateUniqueName();
+ String indexTableName = generateUniqueName();
+ conn.createStatement().execute("create table " + dataTableName +
+ " (id varchar(10) not null primary key, val1 varchar(10),
val2 varchar(10), val3 varchar(10))");
+ if (uncovered) {
+ // The INCLUDE clause should not be allowed
+ try {
+ conn.createStatement().execute("CREATE UNCOVERED INDEX " +
indexTableName
+ + " on " + dataTableName + " (val1) INCLUDE
(val2)");
+ Assert.fail();
+ } catch (Exception e) {
Review Comment:
Shouldn't this be a SqlException in particular?
##########
phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -807,14 +810,39 @@ private void handleLocalIndexUpdates(TableName table,
miniBatchOp.addOperationsFromCP(0, localUpdates.toArray(new
Mutation[localUpdates.size()]));
}
}
+
+ private boolean isPartialUncoveredIndexUpdate(PhoenixIndexMetaData
indexMetaData,
+ MiniBatchOperationInProgress<Mutation> miniBatchOp) throws
IOException {
+ int indexedColumnCount = 0;
+ for (IndexMaintainer indexMaintainer :
indexMetaData.getIndexMaintainers()) {
+ indexedColumnCount += indexMaintainer.getIndexedColumns().size();
+ }
+ Set<ColumnReference> indexedColumns = new
HashSet<ColumnReference>(indexedColumnCount);
+ for (IndexMaintainer indexMaintainer :
indexMetaData.getIndexMaintainers()) {
+ indexedColumns.addAll(indexMaintainer.getIndexedColumns());
+ }
+ for (int i = 0; i < miniBatchOp.size(); i++) {
+ if (miniBatchOp.getOperationStatus(i) == IGNORE) {
+ continue;
+ }
+ Mutation m = miniBatchOp.getOperation(i);
+ if (!this.builder.isEnabled(m)) {
+ continue;
+ }
+ for (ColumnReference indexedColumn : indexedColumns) {
+ if (m.get(indexedColumn.getFamily(),
indexedColumn.getQualifier()).isEmpty()) {
+ return true;
Review Comment:
nit: a comment here explaining how an empty column qualifier means the index
update is partial
##########
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UncoveredIndexRegionScanner.java:
##########
@@ -162,7 +174,7 @@ protected Scan prepareDataTableScan(Collection<byte[]>
dataRowKeys) throws IOExc
scanRanges.initializeScan(dataScan);
SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
dataScan.setFilter(new SkipScanFilter(skipScanFilter, false));
- scan.setAttribute(BaseScannerRegionObserver.SERVER_PAGE_SIZE_MS,
+ dataScan.setAttribute(BaseScannerRegionObserver.SERVER_PAGE_SIZE_MS,
Review Comment:
Was this a pre-existing bug that we weren't paging properly in this case?
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/PTable.java:
##########
@@ -104,8 +104,9 @@ public ViewType combine(ViewType otherType) {
}
public enum IndexType {
- GLOBAL((byte)1),
- LOCAL((byte)2);
+ GLOBAL((byte)1), // Covered Global
+ LOCAL((byte)2), // Covered Local
+ UNCOVERED((byte)3); // Uncovered Global
Review Comment:
Since we can do uncovered queries on local indexes, would it be more clear
if the enum were UNCOVERED_GLOBAL (or GLOBAL_UNCOVERED)?
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexUpgradeTool.java:
##########
@@ -52,9 +52,7 @@
import org.apache.phoenix.schema.PIndexState;
import org.apache.phoenix.schema.PTable;
import org.apache.phoenix.schema.PTableType;
-import org.apache.phoenix.util.EnvironmentEdgeManager;
-import org.apache.phoenix.util.MetaDataUtil;
-import org.apache.phoenix.util.PhoenixRuntime;
+import org.apache.phoenix.util.*;
Review Comment:
nit: avoid star imports
##########
phoenix-core/src/main/java/org/apache/phoenix/execute/MutationState.java:
##########
@@ -1693,11 +1693,14 @@ private void filterIndexCheckerMutations(Map<TableInfo,
List<Mutation>> mutation
continue;
}
if (m instanceof Delete) {
- Put put = new Put(m.getRow());
- put.addColumn(emptyCF, emptyCQ,
IndexRegionObserver.getMaxTimestamp(m),
- QueryConstants.UNVERIFIED_BYTES);
- // The Delete gets marked as unverified in Phase 1 and
gets deleted on Phase 3.
- addToMap(unverifiedIndexMutations, tableInfo, put);
+ if
(tableInfo.getOrigTableRef().getTable().getIndexType()
Review Comment:
nit: a quick comment on why the uncovered case doesn't need the Phase 1
Unverified flag nor the Phase 3 verified flag would be helpful (since otherwise
it's just mentioned in the JIRA)
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/UncoveredGlobalIndexRegionScannerIT.java:
##########
@@ -80,7 +93,39 @@ private void populateTable(String tableName) throws
Exception {
}
@Test
- public void testUncoveredIndexWithPhoenixRowTimestamp() throws Exception {
+ public void testDDL() throws Exception {
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ String dataTableName = generateUniqueName();
+ String indexTableName = generateUniqueName();
+ conn.createStatement().execute("create table " + dataTableName +
+ " (id varchar(10) not null primary key, val1 varchar(10),
val2 varchar(10), val3 varchar(10))");
+ if (uncovered) {
+ // The INCLUDE clause should not be allowed
+ try {
+ conn.createStatement().execute("CREATE UNCOVERED INDEX " +
indexTableName
+ + " on " + dataTableName + " (val1) INCLUDE
(val2)");
+ Assert.fail();
+ } catch (Exception e) {
+ // Expected
+ }
+ // The LOCAL keyword should not be allowed with UNCOVERED
+ try {
+ conn.createStatement().execute("CREATE UNCOVERED LOCAL
INDEX " + indexTableName
Review Comment:
This makes more sense when you know from the PTable.IndexType enum comments
that UNCOVERED really means "UNCOVERED GLOBAL", but that's not obvious from the
syntax.
##########
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java:
##########
@@ -105,16 +107,7 @@
import org.apache.phoenix.schema.tuple.ValueGetterTuple;
import org.apache.phoenix.schema.types.PDataType;
import org.apache.phoenix.transaction.PhoenixTransactionProvider.Feature;
-import org.apache.phoenix.util.BitSet;
-import org.apache.phoenix.util.ByteUtil;
-import org.apache.phoenix.util.EncodedColumnsUtil;
-import org.apache.phoenix.util.ExpressionUtil;
-import org.apache.phoenix.util.IndexUtil;
-import org.apache.phoenix.util.MetaDataUtil;
-import org.apache.phoenix.util.PhoenixRuntime;
-import org.apache.phoenix.util.SchemaUtil;
-import org.apache.phoenix.util.TransactionUtil;
-import org.apache.phoenix.util.TrustedByteArrayOutputStream;
+import org.apache.phoenix.util.*;
Review Comment:
nit: avoid star imports
##########
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UncoveredIndexRegionScanner.java:
##########
@@ -244,20 +256,62 @@ protected boolean scanIndexTableRows(List<Cell> result,
return scanIndexTableRows(result, startTime, null, 0);
}
- private boolean getNextCoveredIndexRow(List<Cell> result) {
+ private boolean verifyIndexRowAndRepairIfNecessary(Result dataRow, byte[]
indexRowKey, long ts) throws IOException {
+ Put put = new Put(dataRow.getRow());
+ for (Cell cell : dataRow.rawCells()) {
+ put.add(cell);
+ }
+ if (indexMaintainer.checkIndexRow(indexRowKey, put)) {
+ if (IndexUtil.getMaxTimestamp(put) != ts) {
+ Mutation[] mutations;
+ Put indexPut = new Put(indexRowKey);
+ indexPut.addColumn(emptyCF, emptyCQ, ts,
QueryConstants.VERIFIED_BYTES);
+ if ((EnvironmentEdgeManager.currentTimeMillis() - ts) >
ageThreshold) {
+ Delete indexDelete =
indexMaintainer.buildRowDeleteMutation(indexRowKey,
Review Comment:
Not sure I follow the Delete logic here. The index row matches the data row,
but the index timestamp is old enough to be before the age threshold, so since
we _should_ have deleted it earlier we delete it anyway even though it's
correct? Is this because we assume it's still from a previous, failed data
table update that was retried successfully later?
Or am I completely misunderstanding this?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/UncoveredGlobalIndexRegionScannerIT.java:
##########
@@ -369,18 +523,97 @@ public void testCount() throws Exception {
ResultSet rs = conn.createStatement().executeQuery(selectSql);
assertTrue(rs.next());
long count = rs.getLong(1);
- selectSql = "SELECT /*+ INDEX(" + dataTableName + " " +
indexTableName
- + ")*/ Count(v3) from " + dataTableName + " where v1 = 5";
- //Verify that we will read from the index table
+ selectSql = "SELECT"+ (uncovered ? " " : "/*+ INDEX(" +
dataTableName + " " + indexTableName + ")*/ ")
+ + "Count(v3) from " + dataTableName + " where v1 = 5";
+ // Verify that we will read from the index table
assertExplainPlan(conn, selectSql, dataTableName, indexTableName);
rs = conn.createStatement().executeQuery(selectSql);
assertTrue(rs.next());
assertEquals(count, rs.getInt(1));
}
}
+ @Test
+ public void testFailDataTableRowUpdate() throws Exception {
+ String dataTableName = generateUniqueName();
+ populateTable(dataTableName); // with two rows ('a', 'ab', 'abc',
'abcd') and ('b', 'bc', 'bcd', 'bcde')
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ String indexTableName = generateUniqueName();
+ conn.createStatement().execute("CREATE " + (uncovered ? "UNCOVERED
" : " ") + "INDEX "
+ + indexTableName + " on " + dataTableName + " (val1)");
+ // Configure IndexRegionObserver to fail the data table update
+ // and check that this does not impact the correctness
+ IndexRegionObserver.setFailDataTableUpdatesForTesting(true);
+ conn.createStatement().execute("upsert into " + dataTableName + "
(id, val2) values ('a', 'abcc')");
+ try {
+ conn.commit();
+ fail();
+ } catch (Exception e) {
+ // this is expected
+ }
+ IndexRegionObserver.setFailDataTableUpdatesForTesting(false);
+
+ conn.createStatement().execute("upsert into " + dataTableName + "
(id, val3) values ('a', 'abcdd')");
+ conn.commit();
+ String selectSql = "SELECT"+ (uncovered ? " " : "/*+ INDEX(" +
dataTableName + " "
+ + indexTableName + ")*/ ") + "val2, val3 from " +
dataTableName
+ + " WHERE val1 = 'ab'";
+ // Verify that we will read from the first index table
+ assertExplainPlan(conn, selectSql, dataTableName, indexTableName);
+ ResultSet rs = conn.createStatement().executeQuery(selectSql);
+ assertTrue(rs.next());
+ assertEquals("abc", rs.getString(1));
+ assertEquals("abcdd", rs.getString(2));
+ assertFalse(rs.next());
+ }
+ }
+ @Test
+ public void testFailPostIndexDeleteUpdate() throws Exception {
+ String dataTableName = generateUniqueName();
+ populateTable(dataTableName); // with two rows ('a', 'ab', 'abc',
'abcd') and ('b', 'bc', 'bcd', 'bcde')
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ String indexTableName = generateUniqueName();
+ conn.createStatement().execute("CREATE " + (uncovered ? "UNCOVERED
" : " ") + "INDEX "
+ + indexTableName + " on " + dataTableName + " (val1)");
+ String selectSql = "SELECT id from " + dataTableName + " WHERE
val1 = 'ab'";
+
+ // Verify that we will read from the index table
+ assertExplainPlan(conn, selectSql, dataTableName, indexTableName);
+ ResultSet rs = conn.createStatement().executeQuery(selectSql);
+ assertTrue(rs.next());
+ assertEquals("a", rs.getString(1));
+ assertFalse(rs.next());
+ TestUtil.dumpTable(conn, TableName.valueOf(indexTableName));
Review Comment:
nit: are these TestUtil.dumpTable calls still necessary?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
