PHOENIX-4742 DistinctPrefixFilter potentially seeks to lesser key when descending or null value
Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/48b6f99a Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/48b6f99a Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/48b6f99a Branch: refs/heads/4.x-HBase-1.4 Commit: 48b6f99acdeb91e3167e7beeed49747f7b7dcc6c Parents: 6ab9b37 Author: James Taylor <jtay...@salesforce.com> Authored: Fri May 18 08:46:38 2018 -0700 Committer: James Taylor <jtay...@salesforce.com> Committed: Fri May 18 17:01:47 2018 -0700 ---------------------------------------------------------------------- .../org/apache/phoenix/end2end/OrderByIT.java | 45 +++++++++----------- .../GroupedAggregateRegionObserver.java | 4 +- .../phoenix/filter/DistinctPrefixFilter.java | 31 ++++++++++---- .../apache/phoenix/filter/SkipScanFilter.java | 4 +- .../org/apache/phoenix/schema/RowKeySchema.java | 20 +++++---- 5 files changed, 61 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/48b6f99a/phoenix-core/src/it/java/org/apache/phoenix/end2end/OrderByIT.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/OrderByIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/OrderByIT.java index 9d6a450..578a3af 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/OrderByIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/OrderByIT.java @@ -27,10 +27,10 @@ import static org.apache.phoenix.util.TestUtil.ROW7; import static org.apache.phoenix.util.TestUtil.ROW8; import static org.apache.phoenix.util.TestUtil.ROW9; import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES; +import static org.apache.phoenix.util.TestUtil.assertResultSet; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.apache.phoenix.util.TestUtil.assertResultSet; import java.sql.Connection; import java.sql.Date; @@ -663,7 +663,6 @@ public class OrderByIT extends ParallelStatsDisabledIT { conn = DriverManager.getConnection(getUrl(), props); String tableName=generateUniqueName(); - conn.createStatement().execute("DROP TABLE if exists "+tableName); String sql="CREATE TABLE "+tableName+" ( "+ "ORGANIZATION_ID VARCHAR,"+ "CONTAINER_ID VARCHAR,"+ @@ -871,26 +870,25 @@ public class OrderByIT extends ParallelStatsDisabledIT { } @Test - public void testOrderByReverseOptimizationBug3491() throws Exception { + public void testOrderByReverseOptimization() throws Exception { for(boolean salted: new boolean[]{true,false}) { - doTestOrderByReverseOptimizationBug3491(salted,true,true,true); - doTestOrderByReverseOptimizationBug3491(salted,true,true,false); - doTestOrderByReverseOptimizationBug3491(salted,true,false,true); - doTestOrderByReverseOptimizationBug3491(salted,true,false,false); - doTestOrderByReverseOptimizationBug3491(salted,false,true,true); - doTestOrderByReverseOptimizationBug3491(salted,false,true,false); - doTestOrderByReverseOptimizationBug3491(salted,false,false,true); - doTestOrderByReverseOptimizationBug3491(salted,false,false,false); + doTestOrderByReverseOptimization(salted,true,true,true); + doTestOrderByReverseOptimization(salted,true,true,false); + doTestOrderByReverseOptimization(salted,true,false,true); + doTestOrderByReverseOptimization(salted,true,false,false); + doTestOrderByReverseOptimization(salted,false,true,true); + doTestOrderByReverseOptimization(salted,false,true,false); + doTestOrderByReverseOptimization(salted,false,false,true); + doTestOrderByReverseOptimization(salted,false,false,false); } } - private void doTestOrderByReverseOptimizationBug3491(boolean salted,boolean desc1,boolean desc2,boolean desc3) throws Exception { + private void doTestOrderByReverseOptimization(boolean salted,boolean desc1,boolean desc2,boolean desc3) throws Exception { Connection conn = null; try { Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); conn = DriverManager.getConnection(getUrl(), props); String tableName=generateUniqueName(); - conn.createStatement().execute("DROP TABLE if exists "+tableName); String sql="CREATE TABLE "+tableName+" ( "+ "ORGANIZATION_ID INTEGER NOT NULL,"+ "CONTAINER_ID INTEGER NOT NULL,"+ @@ -965,26 +963,25 @@ public class OrderByIT extends ParallelStatsDisabledIT { } @Test - public void testOrderByReverseOptimizationWithNUllsLastBug3491() throws Exception{ + public void testOrderByReverseOptimizationWithNullsLast() throws Exception{ for(boolean salted: new boolean[]{true,false}) { - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,true,true,true); - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,true,true,false); - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,true,false,true); - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,true,false,false); - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,false,true,true); - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,false,true,false); - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,false,false,true); - doTestOrderByReverseOptimizationWithNUllsLastBug3491(salted,false,false,false); + doTestOrderByReverseOptimizationWithNullsLast(salted,true,true,true); + doTestOrderByReverseOptimizationWithNullsLast(salted,true,true,false); + doTestOrderByReverseOptimizationWithNullsLast(salted,true,false,true); + doTestOrderByReverseOptimizationWithNullsLast(salted,true,false,false); + doTestOrderByReverseOptimizationWithNullsLast(salted,false,true,true); + doTestOrderByReverseOptimizationWithNullsLast(salted,false,true,false); + doTestOrderByReverseOptimizationWithNullsLast(salted,false,false,true); + doTestOrderByReverseOptimizationWithNullsLast(salted,false,false,false); } } - private void doTestOrderByReverseOptimizationWithNUllsLastBug3491(boolean salted,boolean desc1,boolean desc2,boolean desc3) throws Exception { + private void doTestOrderByReverseOptimizationWithNullsLast(boolean salted,boolean desc1,boolean desc2,boolean desc3) throws Exception { Connection conn = null; try { Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); conn = DriverManager.getConnection(getUrl(), props); String tableName=generateUniqueName(); - conn.createStatement().execute("DROP TABLE if exists "+tableName); String sql="CREATE TABLE "+tableName+" ( "+ "ORGANIZATION_ID VARCHAR,"+ "CONTAINER_ID VARCHAR,"+ http://git-wip-us.apache.org/repos/asf/phoenix/blob/48b6f99a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GroupedAggregateRegionObserver.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GroupedAggregateRegionObserver.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GroupedAggregateRegionObserver.java index a6fa6a5..86ab275 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GroupedAggregateRegionObserver.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GroupedAggregateRegionObserver.java @@ -534,8 +534,8 @@ public class GroupedAggregateRegionObserver extends BaseScannerRegionObserver { currentKey.getLength(), SINGLE_COLUMN_FAMILY, SINGLE_COLUMN, AGG_TIMESTAMP, value, 0, value.length); results.add(keyValue); - if (logger.isDebugEnabled()) { - logger.debug(LogUtil.addCustomAnnotations("Adding new aggregate row: " + if (logger.isInfoEnabled()) { + logger.info(LogUtil.addCustomAnnotations("Adding new aggregate row: " + keyValue + ",for current key " + Bytes.toStringBinary(currentKey.get(), currentKey.getOffset(), http://git-wip-us.apache.org/repos/asf/phoenix/blob/48b6f99a/phoenix-core/src/main/java/org/apache/phoenix/filter/DistinctPrefixFilter.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/filter/DistinctPrefixFilter.java b/phoenix-core/src/main/java/org/apache/phoenix/filter/DistinctPrefixFilter.java index 1280cb5..fd376c4 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/filter/DistinctPrefixFilter.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/filter/DistinctPrefixFilter.java @@ -30,6 +30,8 @@ import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.Writables; import org.apache.hadoop.io.Writable; import org.apache.phoenix.schema.RowKeySchema; +import org.apache.phoenix.schema.SortOrder; +import org.apache.phoenix.schema.ValueSchema.Field; import org.apache.phoenix.schema.types.PDataType; import org.apache.phoenix.util.ByteUtil; @@ -38,8 +40,9 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { private int offset; private RowKeySchema schema; - private int prefixLengh; + private int prefixLength; private boolean filterAll = false; + private int lastPosition; private final ImmutableBytesWritable lastKey = new ImmutableBytesWritable(ByteUtil.EMPTY_BYTE_ARRAY, -1, -1); public DistinctPrefixFilter() { @@ -47,7 +50,7 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { public DistinctPrefixFilter(RowKeySchema schema, int prefixLength) { this.schema = schema; - this.prefixLengh = prefixLength; + this.prefixLength = prefixLength; } public void setOffset(int offset) { @@ -60,13 +63,14 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { // First determine the prefix based on the schema int maxOffset = schema.iterator(v.getRowArray(), v.getRowOffset()+offset, v.getRowLength()-offset, ptr); - schema.next(ptr, 0, maxOffset, prefixLengh - 1); + int position = schema.next(ptr, 0, maxOffset, prefixLength - 1); // now check whether we have seen this prefix before if (lastKey.getLength() != ptr.getLength() || !Bytes.equals(ptr.get(), ptr.getOffset(), ptr.getLength(), lastKey.get(), lastKey.getOffset(), ptr.getLength())) { // if we haven't seen this prefix, include the row and remember this prefix lastKey.set(ptr.get(), ptr.getOffset(), ptr.getLength()); + lastPosition = position - 1; return ReturnCode.INCLUDE; } // we've seen this prefix already, seek to the next @@ -75,7 +79,8 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { @Override public Cell getNextCellHint(Cell v) throws IOException { - PDataType<?> type = schema.getField(prefixLengh-1).getDataType(); + Field field = schema.getField(prefixLength - 1); + PDataType<?> type = field.getDataType(); ImmutableBytesWritable tmp; // In the following we make sure we copy the key at most once @@ -83,8 +88,10 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { if (offset > 0) { // make space to copy the missing offset, also 0-pad here if needed // (since we're making a copy anyway) + // We need to pad all null columns, otherwise we'll potentially + // skip rows. byte[] tmpKey = new byte[offset + lastKey.getLength() + - (reversed || type.isFixedWidth() ? 0 : 1)]; + (reversed || type.isFixedWidth() || field.getSortOrder() == SortOrder.DESC ? 0 : 1) + (prefixLength - 1 - lastPosition)]; System.arraycopy(v.getRowArray(), v.getRowOffset(), tmpKey, 0, offset); System.arraycopy(lastKey.get(), lastKey.getOffset(), tmpKey, offset, lastKey.getLength()); tmp = new ImmutableBytesWritable(tmpKey); @@ -105,7 +112,15 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { } else { // pad with a 0x00 byte (makes a copy) tmp = new ImmutableBytesWritable(lastKey); - ByteUtil.nullPad(tmp, tmp.getLength() + 1); + ByteUtil.nullPad(tmp, tmp.getLength() + prefixLength - lastPosition); + // Trim back length if: + // 1) field is descending since the separator byte if 0xFF + // 2) last key has trailing null + // Otherwise, in both cases we'd potentially be seeking to a row before + // our current key. + if (field.getSortOrder() == SortOrder.DESC || prefixLength - lastPosition > 1) { + tmp.set(tmp.get(),tmp.getOffset(),tmp.getLength()-1); + } } // calculate the next key if (!ByteUtil.nextKey(tmp.get(), tmp.getOffset(), tmp.getLength())) { @@ -126,7 +141,7 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { public void write(DataOutput out) throws IOException { out.writeByte(VERSION); schema.write(out); - out.writeInt(prefixLengh); + out.writeInt(prefixLength); } @Override @@ -134,7 +149,7 @@ public class DistinctPrefixFilter extends FilterBase implements Writable { in.readByte(); // ignore schema = new RowKeySchema(); schema.readFields(in); - prefixLengh = in.readInt(); + prefixLength = in.readInt(); } @Override http://git-wip-us.apache.org/repos/asf/phoenix/blob/48b6f99a/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java b/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java index c9d951c..47da151 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java @@ -489,7 +489,9 @@ public class SkipScanFilter extends FilterBase implements Writable { } i++; // If we run out of slots in our key, it means we have a partial key. - if (schema.next(ptr, ScanUtil.getRowKeyPosition(slotSpan, i), maxOffset, slotSpan[i]) == null) { + int rowKeyPos = ScanUtil.getRowKeyPosition(slotSpan, i); + int slotSpans = slotSpan[i]; + if (schema.next(ptr, rowKeyPos, maxOffset, slotSpans) < rowKeyPos + slotSpans) { // If the rest of the slots are checking for IS NULL, then break because // that's the case (since we don't store trailing nulls). if (allTrailingNulls(i)) { http://git-wip-us.apache.org/repos/asf/phoenix/blob/48b6f99a/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java index 72ebddd..1a44ce1 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java @@ -197,10 +197,11 @@ public class RowKeySchema extends ValueSchema { * @return true if a value was found and ptr was set, false if the value is null and ptr was not * set, and null if the value is null and there are no more values */ - public Boolean next(ImmutableBytesWritable ptr, int position, int maxOffset, int extraSpan) { - Boolean returnValue = next(ptr, position, maxOffset); - readExtraFields(ptr, position + 1, maxOffset, extraSpan); - return returnValue; + public int next(ImmutableBytesWritable ptr, int position, int maxOffset, int extraSpan) { + if (next(ptr, position, maxOffset) == null) { + return position-1; + } + return readExtraFields(ptr, position + 1, maxOffset, extraSpan); } @edu.umd.cs.findbugs.annotations.SuppressWarnings( @@ -337,18 +338,21 @@ public class RowKeySchema extends ValueSchema { * @param maxOffset the maximum offset into the bytes pointer to allow * @param extraSpan the number of extra fields to expand the ptr to contain. */ - private void readExtraFields(ImmutableBytesWritable ptr, int position, int maxOffset, int extraSpan) { + private int readExtraFields(ImmutableBytesWritable ptr, int position, int maxOffset, int extraSpan) { int initialOffset = ptr.getOffset(); - for(int i = 0; i < extraSpan; i++) { - Boolean returnValue = next(ptr, position + i, maxOffset); + int i = 0; + Boolean hasValue = Boolean.FALSE; + for(i = 0; i < extraSpan; i++) { + hasValue = next(ptr, position + i, maxOffset); - if(returnValue == null) { + if(hasValue == null) { break; } } int finalLength = ptr.getOffset() - initialOffset + ptr.getLength(); ptr.set(ptr.get(), initialOffset, finalLength); + return position + i - (Boolean.FALSE.equals(hasValue) ? 1 : 0); } }