You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@phoenix.apache.org by ya...@apache.org on 2020/07/24 17:36:27 UTC
[phoenix] branch master updated: PHOENIX-6013 RVC Offset does not
handle coerced literal nulls properly
This is an automated email from the ASF dual-hosted git repository.
yanxinyi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/master by this push:
new 66f20fd PHOENIX-6013 RVC Offset does not handle coerced literal nulls properly
66f20fd is described below
commit 66f20fdd5aae30ad36f2772dff1b768e3ef96ab8
Author: Daniel Wong <da...@salesforce.com>
AuthorDate: Mon Jul 20 11:47:18 2020 -0700
PHOENIX-6013 RVC Offset does not handle coerced literal nulls properly
Signed-off-by: Xinyi Yan <ya...@apache.org>
---
.../end2end/RowValueConstructorOffsetIT.java | 108 ++++++++++++++++++++-
.../apache/phoenix/compile/RVCOffsetCompiler.java | 40 +++++---
.../phoenix/compile/RVCOffsetCompilerTest.java | 77 ++++++++++++++-
3 files changed, 204 insertions(+), 21 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 38d4557..841e4aa 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
@@ -18,6 +18,7 @@
package org.apache.phoenix.end2end;
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.assertTrue;
@@ -35,6 +36,7 @@ import java.util.List;
import java.util.Properties;
import org.apache.phoenix.compile.QueryPlan;
+import org.apache.phoenix.jdbc.PhoenixResultSet;
import org.apache.phoenix.schema.PTableType;
import org.apache.phoenix.schema.RowValueConstructorOffsetNotCoercibleException;
import org.apache.phoenix.util.PhoenixRuntime;
@@ -932,6 +934,105 @@ public class RowValueConstructorOffsetIT extends ParallelStatsDisabledIT {
}
@Test
+ public void testIndexMultiColumnsMultiIndexesVariableLengthNullLiteralsRVCOffset() throws SQLException {
+ String ddlTemplate = "CREATE TABLE %s (k1 VARCHAR,\n" +
+ "k2 VARCHAR,\n" +
+ "k3 VARCHAR,\n" +
+ "k4 VARCHAR,\n" +
+ "k5 VARCHAR,\n" +
+ "k6 VARCHAR,\n" +
+ "v1 VARCHAR,\n" +
+ "v2 VARCHAR,\n" +
+ "v3 VARCHAR,\n" +
+ "v4 VARCHAR,\n" +
+ "CONSTRAINT pk PRIMARY KEY (k1, k2, k3, k4, k5, k6)) ";
+
+ String longKeyTableName = "T_" + generateUniqueName();
+ String longKeyIndex1Name = "INDEX_1_" + longKeyTableName;
+
+ String ddl = String.format(ddlTemplate,longKeyTableName);
+ try(Statement statement = conn.createStatement()) {
+ statement.execute(ddl);
+ }
+
+ String createIndex1 = "CREATE INDEX IF NOT EXISTS " + longKeyIndex1Name + " ON " + longKeyTableName + " (k2 ,v1, k4)";
+
+ try(Statement statement = conn.createStatement()) {
+ statement.execute(createIndex1);
+ }
+
+ String sql0 = "SELECT v1,v3 FROM " + longKeyTableName + " LIMIT 3 OFFSET (k1 ,k2, k3, k4, k5, k6)=('0','1',null,null,null,'2')";
+ try(Statement statement = conn.createStatement(); ResultSet rs = statement.executeQuery(sql0)) {
+ PhoenixResultSet phoenixResultSet = rs.unwrap(PhoenixResultSet.class);
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().size());
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).size());
+ byte[] startRow = phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).get(0).getStartRow();
+ byte[] expectedRow = new byte[] {'0',0,'1',0,0,0,0,'2',1}; //note trailing 1 not 0 due to phoenix internal inconsistency
+ assertArrayEquals(expectedRow,startRow);
+ }
+
+ String sql = "SELECT k2,v1,k4 FROM " + longKeyTableName + " LIMIT 3 OFFSET (k2,v1,k4,k1,k3,k5,k6)=('2',null,'4','1','3','5','6')";
+ try(Statement statement = conn.createStatement(); ResultSet rs = statement.executeQuery(sql)) {
+ PhoenixResultSet phoenixResultSet = rs.unwrap(PhoenixResultSet.class);
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().size());
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).size());
+ byte[] startRow = phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).get(0).getStartRow();
+ byte[] expectedRow = new byte[] {'2',0,0,'4',0,'1',0,'3',0,'5',0,'6',1}; //note trailing 1 not 0 due to phoenix internal inconsistency
+ assertArrayEquals(expectedRow,startRow);
+ }
+ }
+
+ @Test
+ public void testIndexMultiColumnsIndexedFixedLengthNullLiteralsRVCOffset() throws SQLException {
+ String ddlTemplate = "CREATE TABLE %s (k1 VARCHAR,\n" +
+ "v1 TINYINT,\n" +
+ "v2 TINYINT,\n" +
+ "v3 TINYINT,\n" +
+ "v4 TINYINT,\n" +
+ "CONSTRAINT pk PRIMARY KEY (k1)) ";
+
+ String longKeyTableName = "T_" + generateUniqueName();
+ String longKeyIndex1Name = "INDEX_1_" + longKeyTableName;
+
+ String ddl = String.format(ddlTemplate,longKeyTableName);
+ try(Statement statement = conn.createStatement()) {
+ statement.execute(ddl);
+ }
+
+ String createIndex1 = "CREATE INDEX IF NOT EXISTS " + longKeyIndex1Name + " ON " + longKeyTableName + " (v1, k1, v2, v3)";
+
+ try(Statement statement = conn.createStatement()) {
+ statement.execute(createIndex1);
+ }
+
+ String sql0 = "SELECT v1,v3 FROM " + longKeyTableName + " LIMIT 3 OFFSET (k1)=('-1')";
+ try(Statement statement = conn.createStatement(); ResultSet rs = statement.executeQuery(sql0)) {
+ PhoenixResultSet phoenixResultSet = rs.unwrap(PhoenixResultSet.class);
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().size());
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).size());
+ byte[] startRow = phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).get(0).getStartRow();
+ byte[] expectedRow = new byte[] {'-','1',1}; //note trailing 1 not 0 due to phoenix internal inconsistency
+ assertArrayEquals(expectedRow,startRow);
+ }
+
+ String sql = "SELECT v1,v3 FROM " + longKeyTableName + " LIMIT 3 OFFSET (v1, k1, v2, v3)=(null,'a',null,0)";
+ try(Statement statement = conn.createStatement(); ResultSet rs = statement.executeQuery(sql)) {
+ PhoenixResultSet phoenixResultSet = rs.unwrap(PhoenixResultSet.class);
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().size());
+ assertEquals(1,phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).size());
+ byte[] startRow = phoenixResultSet.getStatement().getQueryPlan().getScans().get(0).get(0).getStartRow();
+ /* decimal is used for fixed with types
+ * we represent fixed integer keys as decimal form in the index as they need to be variable length.
+ * -128 normal two's compliment form would be 0b10000000
+ * but we flip the leading edge to keep the rowkey sorted, yielding 0.
+ */
+ byte[] expectedRow = new byte[] {0,'a',0,0,-128,1}; //note trailing 1 not 0 due to phoenix internal inconsistency
+ assertArrayEquals(expectedRow,startRow);
+ }
+
+ }
+
+ @Test
public void testOffsetExplain() throws SQLException {
String sql = "EXPLAIN SELECT * FROM " + DATA_TABLE_NAME + " LIMIT 2 OFFSET (k1,k2,k3)=(2, 3, 2)";
try(Statement statement = conn.createStatement(); ResultSet rs = statement.executeQuery(sql)) {
@@ -1030,11 +1131,10 @@ public class RowValueConstructorOffsetIT extends ParallelStatsDisabledIT {
upserts.add("UPSERT INTO TEST_SCHEMA(ORGANIZATION_ID,TEST_ID) VALUES ('2','2')");
for(String sql : upserts) {
- try (Statement statement = conn.createStatement())
-
- { statement.execute(sql); }
+ try (Statement statement = conn.createStatement()) {
+ statement.execute(sql);
+ }
}
-
conn.commit();
String query = "SELECT * FROM TEST_SCHEMA OFFSET (ORGANIZATION_ID,TEST_ID) = ('1','1')";
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 e665e3d..bf6514d 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
@@ -25,6 +25,7 @@ import org.apache.phoenix.expression.AndExpression;
import org.apache.phoenix.expression.CoerceExpression;
import org.apache.phoenix.expression.ComparisonExpression;
import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.IsNullExpression;
import org.apache.phoenix.expression.RowKeyColumnExpression;
import org.apache.phoenix.parse.CastParseNode;
import org.apache.phoenix.parse.ColumnParseNode;
@@ -185,24 +186,18 @@ public class RVCOffsetCompiler {
throw new RowValueConstructorOffsetInternalErrorException(
"RVC Offset unexpected failure.");
}
- if (expression == null) {
+
+ //If the whereExpression is a single term comparison/isNull it will be entirely removed
+ if (expression == null && whereExpression instanceof AndExpression) {
LOGGER.error("Unexpected error while compiling RVC Offset, got null expression.");
throw new RowValueConstructorOffsetInternalErrorException(
"RVC Offset unexpected failure.");
}
// Now that columns etc have been resolved lets check to make sure they match the pk order
- // Today the RowValueConstuctor Equality gets Rewritten into AND of EQ Ops.
- if (!(whereExpression instanceof AndExpression)) {
- LOGGER.warn("Unexpected error while compiling RVC Offset, expected AndExpression got "
- + whereExpression.getClass().getName());
- throw new RowValueConstructorOffsetInternalErrorException(
- "RVC Offset must specify the tables PKs.");
- }
-
List<RowKeyColumnExpression>
rowKeyColumnExpressionList =
- buildListOfRowKeyColumnExpressions(whereExpression.getChildren(), isIndex);
+ buildListOfRowKeyColumnExpressions(whereExpression, isIndex);
if (rowKeyColumnExpressionList.size() != numUserColumns) {
LOGGER.warn("Unexpected error while compiling RVC Offset, expected " + numUserColumns
+ " found " + rowKeyColumnExpressionList.size());
@@ -272,7 +267,7 @@ public class RVCOffsetCompiler {
//subtract 1 see ScanUtil.SINGLE_COLUMN_SLOT_SPAN is 0
slotSpan[0] = columns.size() - 1;
- byte[] key = ScanUtil.getMinKey(rowKeySchema,slots,slotSpan);
+ byte[] key = ScanUtil.getMinKey(rowKeySchema, slots, slotSpan);
// Note the use of ByteUtil.nextKey() to generate exclusive offset
CompiledOffset
@@ -283,19 +278,34 @@ public class RVCOffsetCompiler {
return compiledOffset;
}
- @VisibleForTesting List<RowKeyColumnExpression> buildListOfRowKeyColumnExpressions(
- List<Expression> expressions, boolean isIndex)
- throws RowValueConstructorOffsetNotCoercibleException {
+ @VisibleForTesting
+ List<RowKeyColumnExpression> buildListOfRowKeyColumnExpressions (
+ Expression whereExpression, boolean isIndex)
+ throws RowValueConstructorOffsetNotCoercibleException, RowValueConstructorOffsetInternalErrorException {
+
+ List<Expression> expressions;
+ if((whereExpression instanceof AndExpression)) {
+ expressions = whereExpression.getChildren();
+ } else if (whereExpression instanceof ComparisonExpression || whereExpression instanceof IsNullExpression) {
+ expressions = Lists.newArrayList(whereExpression);
+ } else {
+ LOGGER.warn("Unexpected error while compiling RVC Offset, expected either a Comparison/IsNull Expression of a AndExpression got "
+ + whereExpression.getClass().getName());
+ throw new RowValueConstructorOffsetInternalErrorException(
+ "RVC Offset must specify the tables PKs.");
+ }
+
List<RowKeyColumnExpression>
rowKeyColumnExpressionList =
new ArrayList<RowKeyColumnExpression>();
for (Expression child : expressions) {
- if (!(child instanceof ComparisonExpression)) {
+ 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.");
}
+ //For either case comparison/isNull the first child should be the rowkey
Expression possibleRowKeyColumnExpression = child.getChildren().get(0);
// Note that since we store indexes in variable length form there may be casts from fixed types to
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 70a48a1..a1c62b4 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
@@ -24,9 +24,11 @@ import java.util.ArrayList;
import java.util.List;
import org.apache.hadoop.hbase.HConstants;
+import org.apache.phoenix.expression.AndExpression;
import org.apache.phoenix.expression.CoerceExpression;
import org.apache.phoenix.expression.ComparisonExpression;
import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.IsNullExpression;
import org.apache.phoenix.expression.RowKeyColumnExpression;
import org.apache.phoenix.parse.ColumnParseNode;
import org.apache.phoenix.parse.ParseNode;
@@ -114,9 +116,12 @@ public class RVCOffsetCompilerTest {
expressions.add(expression1);
expressions.add(expression2);
+ AndExpression expression = mock(AndExpression.class);
+ Mockito.when(expression.getChildren()).thenReturn(expressions);
+
List<RowKeyColumnExpression>
result =
- offsetCompiler.buildListOfRowKeyColumnExpressions(expressions, false);
+ offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false);
assertEquals(2,result.size());
assertEquals(rvc1,result.get(0));
@@ -148,12 +153,80 @@ public class RVCOffsetCompilerTest {
expressions.add(expression1);
expressions.add(expression2);
+ AndExpression expression = mock(AndExpression.class);
+ Mockito.when(expression.getChildren()).thenReturn(expressions);
+
List<RowKeyColumnExpression>
result =
- offsetCompiler.buildListOfRowKeyColumnExpressions(expressions, true);
+ offsetCompiler.buildListOfRowKeyColumnExpressions(expression, true);
assertEquals(2,result.size());
assertEquals(rvc1,result.get(0));
assertEquals(rvc2,result.get(1));
}
+
+ @Test
+ public void buildListOfRowKeyColumnExpressionsSingleNodeComparisonTest() throws Exception {
+ List<Expression> expressions = new ArrayList<>();
+
+ RowKeyColumnExpression rvc = new RowKeyColumnExpression();
+
+ ComparisonExpression expression = mock(ComparisonExpression.class);
+
+ Mockito.when(expression.getChildren()).thenReturn(Lists.<Expression>newArrayList(rvc));
+
+ List<RowKeyColumnExpression>
+ result =
+ offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false);
+
+ assertEquals(1,result.size());
+ assertEquals(rvc,result.get(0));
+ }
+
+ @Test
+ public void buildListOfRowKeyColumnExpressionsSingleNodeIsNullTest() throws Exception {
+ List<Expression> expressions = new ArrayList<>();
+
+ RowKeyColumnExpression rvc = new RowKeyColumnExpression();
+
+ IsNullExpression expression = mock(IsNullExpression.class);
+
+ Mockito.when(expression.getChildren()).thenReturn(Lists.<Expression>newArrayList(rvc));
+
+ List<RowKeyColumnExpression>
+ result =
+ offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false);
+
+ assertEquals(1,result.size());
+ assertEquals(rvc,result.get(0));
+ }
+
+ @Test
+ public void buildListOfRowKeyColumnExpressionsIsNullTest() throws Exception {
+ List<Expression> expressions = new ArrayList<>();
+
+ RowKeyColumnExpression rvc1 = new RowKeyColumnExpression();
+ RowKeyColumnExpression rvc2 = new RowKeyColumnExpression();
+
+ IsNullExpression expression1 = mock(IsNullExpression.class);
+ IsNullExpression expression2 = mock(IsNullExpression.class);
+
+ Mockito.when(expression1.getChildren()).thenReturn(Lists.<Expression>newArrayList(rvc1));
+ Mockito.when(expression2.getChildren()).thenReturn(Lists.<Expression>newArrayList(rvc2));
+
+ expressions.add(expression1);
+ expressions.add(expression2);
+
+ AndExpression expression = mock(AndExpression.class);
+ Mockito.when(expression.getChildren()).thenReturn(expressions);
+
+ List<RowKeyColumnExpression>
+ result =
+ offsetCompiler.buildListOfRowKeyColumnExpressions(expression, false);
+
+ assertEquals(2,result.size());
+ assertEquals(rvc1,result.get(0));
+
+ assertEquals(rvc2,result.get(1));
+ }
}