This is an automated email from the ASF dual-hosted git repository. yanxinyi pushed a commit to branch 4.x in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/4.x by this push: new 7a2d316 PHOENIX-6022 RVC Offset does not handle trailing nulls properly 7a2d316 is described below commit 7a2d316c173a3c718f53df9d2e733092288eefc8 Author: Daniel Wong <daniel.w...@salesforce.com> AuthorDate: Mon Jul 27 18:43:56 2020 -0700 PHOENIX-6022 RVC Offset does not handle trailing nulls properly Signed-off-by: Xinyi Yan <yanxi...@apache.org> --- .../end2end/RowValueConstructorOffsetIT.java | 83 ++++++++++++++++++-- .../apache/phoenix/compile/RVCOffsetCompiler.java | 91 ++++++++++++++++------ .../phoenix/compile/RVCOffsetCompilerTest.java | 33 ++++---- 3 files changed, 162 insertions(+), 45 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorOffsetIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorOffsetIT.java index 841e4aa..0981757 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorOffsetIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorOffsetIT.java @@ -21,6 +21,7 @@ import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -1116,19 +1117,22 @@ public class RowValueConstructorOffsetIT extends ParallelStatsDisabledIT { @Test public void rvcOffsetTrailingVariableLengthKeyTest() throws Exception { - String TEST_DDL = "CREATE TABLE IF NOT EXISTS TEST_SCHEMA (\n" + + String tableName = generateUniqueName(); + String ddl = String.format("CREATE TABLE IF NOT EXISTS %s (\n" + " ORGANIZATION_ID VARCHAR(15), \n" + " TEST_ID VARCHAR(15), \n" + " CREATED_DATE DATE, \n" + " LAST_UPDATE DATE\n" - + " CONSTRAINT TEST_SCHEMA_PK PRIMARY KEY (ORGANIZATION_ID, TEST_ID) \n" + ")"; + + " CONSTRAINT TEST_SCHEMA_PK PRIMARY KEY (ORGANIZATION_ID, TEST_ID) \n" + ")",tableName); try (Statement statement = conn.createStatement()) { - statement.execute(TEST_DDL); + statement.execute(ddl); } //setup + String upsert = "UPSERT INTO %s(ORGANIZATION_ID,TEST_ID) VALUES (%s,%s)"; List<String> upserts = new ArrayList<>(); - upserts.add("UPSERT INTO TEST_SCHEMA(ORGANIZATION_ID,TEST_ID) VALUES ('1','1')"); - upserts.add("UPSERT INTO TEST_SCHEMA(ORGANIZATION_ID,TEST_ID) VALUES ('1','10')"); - upserts.add("UPSERT INTO TEST_SCHEMA(ORGANIZATION_ID,TEST_ID) VALUES ('2','2')"); + upserts.add(String.format(upsert,tableName,"'1'","'1'")); + upserts.add(String.format(upsert,tableName,"'1'","'10'")); + upserts.add(String.format(upsert,tableName,"'2'","'2'")); for(String sql : upserts) { try (Statement statement = conn.createStatement()) { @@ -1137,7 +1141,7 @@ public class RowValueConstructorOffsetIT extends ParallelStatsDisabledIT { } conn.commit(); - String query = "SELECT * FROM TEST_SCHEMA OFFSET (ORGANIZATION_ID,TEST_ID) = ('1','1')"; + String query = String.format("SELECT * FROM %s OFFSET (ORGANIZATION_ID,TEST_ID) = ('1','1')",tableName); try (Statement statement = conn.createStatement() ; ResultSet rs2 = statement.executeQuery(query) ) { assertTrue(rs2.next()); @@ -1150,4 +1154,69 @@ public class RowValueConstructorOffsetIT extends ParallelStatsDisabledIT { } } + //Note that phoenix does not suport a rowkey with a null inserted so there is no single key version of this. + @Test + public void rvcOffsetTrailingNullKeyTest() throws Exception { + String tableName = generateUniqueName(); + String ddl = String.format("CREATE TABLE IF NOT EXISTS %s (\n" + + " ORGANIZATION_ID VARCHAR(15), \n" + " TEST_ID VARCHAR(15), \n" + + " CREATED_DATE DATE, \n" + " LAST_UPDATE DATE\n" + + " CONSTRAINT TEST_SCHEMA_PK PRIMARY KEY (ORGANIZATION_ID, TEST_ID) \n" + ")",tableName); + + try (Statement statement = conn.createStatement()) { + statement.execute(ddl); + } + //setup + String upsert = "UPSERT INTO %s(ORGANIZATION_ID,TEST_ID) VALUES (%s,%s)"; + List<String> upserts = new ArrayList<>(); + upserts.add(String.format(upsert,tableName,"'0'","null")); + upserts.add(String.format(upsert,tableName,"'1'","null")); + upserts.add(String.format(upsert,tableName,"'1'","'1'")); + upserts.add(String.format(upsert,tableName,"'1'","'10'")); + upserts.add(String.format(upsert,tableName,"'2'","null")); + + for(String sql : upserts) { + try (Statement statement = conn.createStatement()) { + statement.execute(sql); + } + } + conn.commit(); + + String query = String.format("SELECT * FROM %s OFFSET (ORGANIZATION_ID,TEST_ID) = ('1',null)",tableName); + + try (Statement statement = conn.createStatement() ; ResultSet rs2 = statement.executeQuery(query) ) { + assertTrue(rs2.next()); + assertEquals("1",rs2.getString(1)); + assertEquals("1",rs2.getString(2)); + assertTrue(rs2.next()); + assertEquals("1",rs2.getString(1)); + assertEquals("10",rs2.getString(2)); + assertTrue(rs2.next()); + assertEquals("2",rs2.getString(1)); + assertNull(rs2.getString(2)); + assertFalse(rs2.next()); + } + + String preparedQuery = String.format("SELECT * FROM %s OFFSET (ORGANIZATION_ID,TEST_ID) = (?,?)",tableName); + + try (PreparedStatement statement = conn.prepareStatement(preparedQuery) ) { + + statement.setString(1,"1"); + statement.setString(2,null); + + try(ResultSet rs2 = statement.executeQuery()) { + assertTrue(rs2.next()); + assertEquals("1", rs2.getString(1)); + assertEquals("1", rs2.getString(2)); + assertTrue(rs2.next()); + assertEquals("1", rs2.getString(1)); + assertEquals("10", rs2.getString(2)); + assertTrue(rs2.next()); + assertEquals("2", rs2.getString(1)); + assertNull(rs2.getString(2)); + assertFalse(rs2.next()); + } + } + } + } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/compile/RVCOffsetCompiler.java b/phoenix-core/src/main/java/org/apache/phoenix/compile/RVCOffsetCompiler.java index 61f28ca..aae6ea4 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/compile/RVCOffsetCompiler.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/RVCOffsetCompiler.java @@ -20,6 +20,7 @@ package org.apache.phoenix.compile; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.collect.Lists; +import com.ibm.icu.impl.number.Parse; import org.apache.commons.lang.StringUtils; import org.apache.phoenix.expression.AndExpression; import org.apache.phoenix.expression.CoerceExpression; @@ -32,6 +33,7 @@ import org.apache.phoenix.parse.ColumnParseNode; import org.apache.phoenix.parse.EqualParseNode; import org.apache.phoenix.parse.FilterableStatement; import org.apache.phoenix.parse.HintNode; +import org.apache.phoenix.parse.LiteralParseNode; import org.apache.phoenix.parse.OffsetNode; import org.apache.phoenix.parse.ParseNode; import org.apache.phoenix.parse.RowValueConstructorParseNode; @@ -45,6 +47,7 @@ import org.apache.phoenix.schema.RowValueConstructorOffsetNotAllowedInQueryExcep import org.apache.phoenix.schema.RowValueConstructorOffsetNotCoercibleException; import org.apache.phoenix.schema.TypeMismatchException; import org.apache.phoenix.util.IndexUtil; +import org.apache.phoenix.util.ParseNodeUtil; import org.apache.phoenix.util.ScanUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -195,9 +198,12 @@ public class RVCOffsetCompiler { } // Now that columns etc have been resolved lets check to make sure they match the pk order - List<RowKeyColumnExpression> - rowKeyColumnExpressionList = + RowKeyColumnExpressionOutput rowKeyColumnExpressionOutput = buildListOfRowKeyColumnExpressions(whereExpression, isIndex); + + List<RowKeyColumnExpression> + rowKeyColumnExpressionList = rowKeyColumnExpressionOutput.getRowKeyColumnExpressions(); + if (rowKeyColumnExpressionList.size() != numUserColumns) { LOGGER.warn("Unexpected error while compiling RVC Offset, expected " + numUserColumns + " found " + rowKeyColumnExpressionList.size()); @@ -240,34 +246,41 @@ public class RVCOffsetCompiler { } } + byte[] key; + // check to see if this was a single key expression ScanRanges scanRanges = context.getScanRanges(); + //We do not generate a point lookup today in phoenix if the rowkey has a trailing null, we generate a range scan. if (!scanRanges.isPointLookup()) { - throw new RowValueConstructorOffsetNotCoercibleException( - "RVC Offset must be a point lookup."); - } - - - RowKeySchema.RowKeySchemaBuilder builder = new RowKeySchema.RowKeySchemaBuilder(columns.size()); + //Since we use a range scan to guarantee we get only the null value and the upper bound is unset this suffices + //sanity check + if (!rowKeyColumnExpressionOutput.isTrailingNull()) { + throw new RowValueConstructorOffsetNotCoercibleException( + "RVC Offset must be a point lookup."); + } + key = scanRanges.getScanRange().getUpperRange(); + } else { + RowKeySchema.RowKeySchemaBuilder builder = new RowKeySchema.RowKeySchemaBuilder(columns.size()); - for(PColumn column : columns) { - builder.addField(column, column.isNullable(), column.getSortOrder()); - } + for (PColumn column : columns) { + builder.addField(column, column.isNullable(), column.getSortOrder()); + } - RowKeySchema rowKeySchema = builder.build(); + RowKeySchema rowKeySchema = builder.build(); - //we make a ScanRange with 1 keyslots that cover the entire PK to reuse the code - KeyRange pointKeyRange = scanRanges.getScanRange(); - KeyRange keyRange = KeyRange.getKeyRange(pointKeyRange.getLowerRange(), false, KeyRange.UNBOUND, true); - List<KeyRange> myRangeList = Lists.newArrayList(keyRange); - List<List<KeyRange>> slots = new ArrayList<>(); - slots.add(myRangeList); - int[] slotSpan = new int[1]; + //we make a ScanRange with 1 keyslots that cover the entire PK to reuse the code + KeyRange pointKeyRange = scanRanges.getScanRange(); + KeyRange keyRange = KeyRange.getKeyRange(pointKeyRange.getLowerRange(), false, KeyRange.UNBOUND, true); + List<KeyRange> myRangeList = Lists.newArrayList(keyRange); + List<List<KeyRange>> slots = new ArrayList<>(); + slots.add(myRangeList); + int[] slotSpan = new int[1]; - //subtract 1 see ScanUtil.SINGLE_COLUMN_SLOT_SPAN is 0 - slotSpan[0] = columns.size() - 1; - byte[] key = ScanUtil.getMinKey(rowKeySchema, slots, slotSpan); + //subtract 1 see ScanUtil.SINGLE_COLUMN_SLOT_SPAN is 0 + slotSpan[0] = columns.size() - 1; + key = ScanUtil.getMinKey(rowKeySchema, slots, slotSpan); + } // Note the use of ByteUtil.nextKey() to generate exclusive offset CompiledOffset @@ -279,10 +292,30 @@ public class RVCOffsetCompiler { } @VisibleForTesting - List<RowKeyColumnExpression> buildListOfRowKeyColumnExpressions ( + static class RowKeyColumnExpressionOutput { + public RowKeyColumnExpressionOutput(List<RowKeyColumnExpression> rowKeyColumnExpressions, boolean trailingNull) { + this.rowKeyColumnExpressions = rowKeyColumnExpressions; + this.trailingNull = trailingNull; + } + + private final List<RowKeyColumnExpression> rowKeyColumnExpressions; + private final boolean trailingNull; + + public List<RowKeyColumnExpression> getRowKeyColumnExpressions() { + return rowKeyColumnExpressions; + } + + public boolean isTrailingNull() { + return trailingNull; + } + } + + @VisibleForTesting + RowKeyColumnExpressionOutput buildListOfRowKeyColumnExpressions ( Expression whereExpression, boolean isIndex) throws RowValueConstructorOffsetNotCoercibleException, RowValueConstructorOffsetInternalErrorException { + boolean trailingNull = false; List<Expression> expressions; if((whereExpression instanceof AndExpression)) { expressions = whereExpression.getChildren(); @@ -298,13 +331,21 @@ public class RVCOffsetCompiler { List<RowKeyColumnExpression> rowKeyColumnExpressionList = new ArrayList<RowKeyColumnExpression>(); - for (Expression child : expressions) { + for (int i = 0; i < expressions.size(); i++) { + Expression child = expressions.get(i); if (!(child instanceof ComparisonExpression || child instanceof IsNullExpression)) { LOGGER.warn("Unexpected error while compiling RVC Offset"); throw new RowValueConstructorOffsetNotCoercibleException( "RVC Offset must specify the tables PKs."); } + //if this is the last position + if(i == expressions.size() - 1) { + if(child instanceof IsNullExpression) { + trailingNull = true; + } + } + //For either case comparison/isNull the first child should be the rowkey Expression possibleRowKeyColumnExpression = child.getChildren().get(0); @@ -325,7 +366,7 @@ public class RVCOffsetCompiler { } rowKeyColumnExpressionList.add((RowKeyColumnExpression) possibleRowKeyColumnExpression); } - return rowKeyColumnExpressionList; + return new RowKeyColumnExpressionOutput(rowKeyColumnExpressionList,trailingNull); } @VisibleForTesting List<ColumnParseNode> buildListOfColumnParseNodes( diff --git a/phoenix-core/src/test/java/org/apache/phoenix/compile/RVCOffsetCompilerTest.java b/phoenix-core/src/test/java/org/apache/phoenix/compile/RVCOffsetCompilerTest.java index a1c62b4..93c2c48 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/compile/RVCOffsetCompilerTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/compile/RVCOffsetCompilerTest.java @@ -18,6 +18,7 @@ package org.apache.phoenix.compile; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import java.util.ArrayList; @@ -119,9 +120,10 @@ public class RVCOffsetCompilerTest { AndExpression expression = mock(AndExpression.class); Mockito.when(expression.getChildren()).thenReturn(expressions); + RVCOffsetCompiler.RowKeyColumnExpressionOutput + output = offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); List<RowKeyColumnExpression> - result = - offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); + result = output.getRowKeyColumnExpressions(); assertEquals(2,result.size()); assertEquals(rvc1,result.get(0)); @@ -156,9 +158,10 @@ public class RVCOffsetCompilerTest { AndExpression expression = mock(AndExpression.class); Mockito.when(expression.getChildren()).thenReturn(expressions); + RVCOffsetCompiler.RowKeyColumnExpressionOutput + output = offsetCompiler.buildListOfRowKeyColumnExpressions(expression, true); List<RowKeyColumnExpression> - result = - offsetCompiler.buildListOfRowKeyColumnExpressions(expression, true); + result = output.getRowKeyColumnExpressions(); assertEquals(2,result.size()); assertEquals(rvc1,result.get(0)); @@ -175,9 +178,10 @@ public class RVCOffsetCompilerTest { Mockito.when(expression.getChildren()).thenReturn(Lists.<Expression>newArrayList(rvc)); + RVCOffsetCompiler.RowKeyColumnExpressionOutput + output = offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); List<RowKeyColumnExpression> - result = - offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); + result = output.getRowKeyColumnExpressions(); assertEquals(1,result.size()); assertEquals(rvc,result.get(0)); @@ -193,12 +197,14 @@ public class RVCOffsetCompilerTest { Mockito.when(expression.getChildren()).thenReturn(Lists.<Expression>newArrayList(rvc)); - List<RowKeyColumnExpression> - result = - offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); + RVCOffsetCompiler.RowKeyColumnExpressionOutput output = offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); + + List<RowKeyColumnExpression> result = output.getRowKeyColumnExpressions(); assertEquals(1,result.size()); assertEquals(rvc,result.get(0)); + + assertTrue(output.isTrailingNull()); } @Test @@ -220,13 +226,14 @@ public class RVCOffsetCompilerTest { AndExpression expression = mock(AndExpression.class); Mockito.when(expression.getChildren()).thenReturn(expressions); - List<RowKeyColumnExpression> - result = - offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); + RVCOffsetCompiler.RowKeyColumnExpressionOutput output = offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false); + + List<RowKeyColumnExpression> result = output.getRowKeyColumnExpressions(); assertEquals(2,result.size()); assertEquals(rvc1,result.get(0)); - assertEquals(rvc2,result.get(1)); + + assertTrue(output.isTrailingNull()); } }