Skip to content

Commit

Permalink
Fix reading empty and all-nulls lists with filters
Browse files Browse the repository at this point in the history
  • Loading branch information
@mbasmanova
mbasmanova committed Sep 5, 2019
1 parent 2e17628 commit 7c28d0b
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 15 deletions.
35 changes: 27 additions & 8 deletions presto-orc/src/main/java/com/facebook/presto/orc/reader/ListSelectiveStreamReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.facebook.presto.spi.block.Block;
import com.facebook.presto.spi.block.BlockLease;
import com.facebook.presto.spi.block.RunLengthEncodedBlock;
import com.facebook.presto.spi.type.ArrayType;
import com.facebook.presto.spi.type.Type;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
Expand Down Expand Up @@ -71,7 +72,7 @@ public class ListSelectiveStreamReader
private final boolean nonNullsAllowed;
private final boolean outputRequired;
@Nullable
private final Type outputType;
private final ArrayType outputType;
// elementStreamReader is null if output is not required and filter is a simple IS [NOT] NULL
@Nullable
private final SelectiveStreamReader elementStreamReader;
Expand Down Expand Up @@ -129,7 +130,7 @@ public ListSelectiveStreamReader(

this.streamDescriptor = requireNonNull(streamDescriptor, "streamDescriptor is null");
this.outputRequired = requireNonNull(outputType, "outputType is null").isPresent();
this.outputType = outputType.orElse(null);
this.outputType = (ArrayType) outputType.orElse(null);
this.level = subfieldLevel;

if (listFilter != null) {
Expand Down Expand Up @@ -189,7 +190,7 @@ private static Optional<TupleDomainFilter> getTopLevelFilter(Map<Subfield, Tuple
public int read(int offset, int[] positions, int positionCount)
throws IOException
{
checkArgument(positionCount > 0, "positionCount must be greater than zero");
checkArgument(positionCount > 0 || listFilter != null, "positionCount must be greater than zero");
checkState(!valuesInUse, "BlockLease hasn't been closed yet");

if (!rowGroupOpen) {
Expand Down Expand Up @@ -267,17 +268,25 @@ private int readAllNulls(int[] positions, int positionCount)
if (nullsFilter.testNull()) {
outputPositionCount++;
}
else {
outputPositionCount -= nullsFilter.getPrecedingPositionsToFail();
i += nullsFilter.getSucceedingPositionsToFail();
}
}
}
else {
outputPositionCount = positionCount;
allNulls = true;
}
}
else {
outputPositionCount = 0;
}

if (listFilter != null) {
listFilter.populateElementFilters(0, null, null, 0);
}

allNulls = true;
return positions[positionCount - 1] + 1;
}

Expand Down Expand Up @@ -373,6 +382,9 @@ private int readNotAllNulls(int[] positions, int positionCount)
if (elementStreamReader != null && elementPositionCount > 0) {
elementStreamReader.read(elementReadOffset, elementPositions, elementPositionCount);
}
else if (listFilter != null && listFilter.getChild() != null) {
elementStreamReader.read(elementReadOffset, elementPositions, elementPositionCount);
}
elementReadOffset += elementPositionCount + skippedElements;

if (listFilter == null || level > 0) {
Expand Down Expand Up @@ -470,11 +482,15 @@ public Block getBlock(int[] positions, int positionCount)
boolean mayHaveNulls = nullsAllowed && presentStream != null;

if (positionCount == outputPositionCount) {
Block elementBlock;
if (elementOutputPositionCount == 0) {
return new RunLengthEncodedBlock(outputType.createBlockBuilder(null, 1).appendNull().build(), outputPositionCount);
elementBlock = outputType.getElementType().createBlockBuilder(null, 0).build();
}
else {
elementBlock = elementStreamReader.getBlock(elementOutputPositions, elementOutputPositionCount);
}

Block block = ArrayBlock.fromElementBlock(positionCount, Optional.ofNullable(mayHaveNulls ? nulls : null), offsets, elementStreamReader.getBlock(elementOutputPositions, elementOutputPositionCount));
Block block = ArrayBlock.fromElementBlock(positionCount, Optional.ofNullable(mayHaveNulls ? nulls : null), offsets, elementBlock);
nulls = null;
offsets = null;
return block;
Expand Down Expand Up @@ -537,12 +553,15 @@ public BlockLease getBlockView(int[] positions, int positionCount)
compactValues(positions, positionCount, includeNulls);
}

BlockLease elementBlockLease;
if (elementOutputPositionCount == 0) {
return newLease(new RunLengthEncodedBlock(outputType.createBlockBuilder(null, 1).appendNull().build(), positionCount));
elementBlockLease = newLease(outputType.getElementType().createBlockBuilder(null, 0).build());
}
else {
elementBlockLease = elementStreamReader.getBlockView(elementOutputPositions, elementOutputPositionCount);
}

valuesInUse = true;
BlockLease elementBlockLease = elementStreamReader.getBlockView(elementOutputPositions, elementOutputPositionCount);
Block block = ArrayBlock.fromElementBlock(positionCount, Optional.ofNullable(includeNulls ? nulls : null), offsets, elementBlockLease.get());
return newLease(block, () -> closeBlockLease(elementBlockLease));
}
Expand Down
8 changes: 6 additions & 2 deletions presto-orc/src/test/java/com/facebook/presto/orc/OrcTester.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -745,9 +745,12 @@ private static boolean testSubfieldValue(Type type, Object value, Subfield subfi
if (nestedValue == null) {
return filter == IS_NULL;
}
int index = toIntExact(((Subfield.LongSubscript) pathElement).getIndex());
int index = toIntExact(((Subfield.LongSubscript) pathElement).getIndex()) - 1;
nestedType = ((ArrayType) nestedType).getElementType();
nestedValue = ((List) nestedValue).get(index - 1);
if (index >= ((List) nestedValue).size()) {
return true;
}
nestedValue = ((List) nestedValue).get(index);
}
else {
fail("Unsupported type: " + type);
Expand Down Expand Up @@ -808,6 +811,7 @@ private static void assertColumnValueEquals(Type type, Object actual, Object exp
if (StandardTypes.ARRAY.equals(baseType)) {
List<?> actualArray = (List<?>) actual;
List<?> expectedArray = (List<?>) expected;
assertEquals(actualArray == null, expectedArray == null);
assertEquals(actualArray.size(), expectedArray.size());

Type elementType = type.getTypeParameters().get(0);
Expand Down
64 changes: 59 additions & 5 deletions presto-orc/src/test/java/com/facebook/presto/orc/TestSelectiveOrcReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.facebook.presto.orc.TupleDomainFilter.BooleanValue;
import com.facebook.presto.orc.TupleDomainFilter.DoubleRange;
import com.facebook.presto.orc.TupleDomainFilter.FloatRange;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.Subfield;
import com.facebook.presto.spi.type.SqlDate;
import com.facebook.presto.spi.type.SqlTimestamp;
Expand Down Expand Up @@ -70,6 +71,8 @@
import static java.util.Collections.nCopies;
import static java.util.stream.Collectors.toList;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;

public class TestSelectiveOrcReader
{
Expand Down Expand Up @@ -370,13 +373,64 @@ public void testArrays()
1, ImmutableMap.of(new Subfield("c[1]"), IS_NULL))));
}

@Test
public void testArrayIndexOutOfBounds()
throws Exception
{
Random random = new Random(0);

// non-null arrays of varying sizes
try {
tester.testRoundTrip(arrayType(INTEGER),
IntStream.range(0, 30_000).map(i -> random.nextInt(10)).mapToObj(size -> makeArray(size, random)).collect(toImmutableList()),
ImmutableList.of(ImmutableMap.of(new Subfield("c[2]"), IS_NULL)));
fail("Expected 'Array subscript out of bounds' exception");
}
catch (PrestoException e) {
assertTrue(e.getMessage().contains("Array subscript out of bounds"));
}

// non-null nested arrays of varying sizes
try {
tester.testRoundTrip(arrayType(arrayType(INTEGER)),
IntStream.range(0, 30_000).mapToObj(i -> ImmutableList.of(makeArray(random.nextInt(5), random), makeArray(random.nextInt(5), random))).collect(toImmutableList()),
ImmutableList.of(ImmutableMap.of(new Subfield("c[2][3]"), IS_NULL)));
fail("Expected 'Array subscript out of bounds' exception");
}
catch (PrestoException e) {
assertTrue(e.getMessage().contains("Array subscript out of bounds"));
}

// empty arrays
try {
tester.testRoundTrip(arrayType(INTEGER),
nCopies(30_000, ImmutableList.of()),
ImmutableList.of(ImmutableMap.of(new Subfield("c[2]"), IS_NULL)));
fail("Expected 'Array subscript out of bounds' exception");
}
catch (PrestoException e) {
assertTrue(e.getMessage().contains("Array subscript out of bounds"));
}

// empty nested arrays
try {
tester.testRoundTrip(arrayType(arrayType(INTEGER)),
nCopies(30_000, ImmutableList.of()),
ImmutableList.of(ImmutableMap.of(new Subfield("c[2][3]"), IS_NULL)));
fail("Expected 'Array subscript out of bounds' exception");
}
catch (PrestoException e) {
assertTrue(e.getMessage().contains("Array subscript out of bounds"));
}
}

@Test
public void testArraysOfNulls()
throws Exception
{
for (Type type : ImmutableList.of(BOOLEAN, BIGINT, INTEGER, SMALLINT, TINYINT, DOUBLE, REAL, TIMESTAMP)) {
for (Type type : ImmutableList.of(BOOLEAN, BIGINT, INTEGER, SMALLINT, TINYINT, DOUBLE, REAL, TIMESTAMP, arrayType(INTEGER))) {
tester.testRoundTrip(arrayType(type),
IntStream.range(0, 30_000).mapToObj(i -> Collections.nCopies(5, null)).collect(toImmutableList()),
nCopies(30_000, nCopies(5, null)),
ImmutableList.of(
ImmutableMap.of(new Subfield("c[2]"), IS_NULL),
ImmutableMap.of(new Subfield("c[2]"), IS_NOT_NULL)));
Expand Down Expand Up @@ -418,7 +472,7 @@ public void testMaps()
ImmutableList.of(
IntStream.range(0, 30_000).mapToObj(i -> random.nextInt()).collect(toImmutableList()),
IntStream.range(0, 30_000).boxed().map(i -> createMap(i)).collect(toImmutableList())),
ImmutableList.of(
toSubfieldFilters(
ImmutableMap.of(0, BigintRange.of(0, Integer.MAX_VALUE, false)),
ImmutableMap.of(1, IS_NOT_NULL),
ImmutableMap.of(1, IS_NULL)));
Expand All @@ -429,7 +483,7 @@ public void testMaps()
ImmutableList.of(
IntStream.range(0, 30_000).mapToObj(i -> random.nextInt()).collect(toImmutableList()),
IntStream.range(0, 30_000).boxed().map(i -> i % 5 == 0 ? null : createMap(i)).collect(toList())),
ImmutableList.of(
toSubfieldFilters(
ImmutableMap.of(0, BigintRange.of(0, Integer.MAX_VALUE, false)),
ImmutableMap.of(1, IS_NOT_NULL),
ImmutableMap.of(1, IS_NULL),
Expand All @@ -442,7 +496,7 @@ public void testMaps()
ImmutableList.of(
IntStream.range(0, 30_000).boxed().map(i -> i % 5 == 0 ? null : createMap(i)).collect(toList()),
IntStream.range(0, 30_000).mapToObj(i -> random.nextInt()).collect(toImmutableList())),
ImmutableList.of(
toSubfieldFilters(
ImmutableMap.of(0, IS_NULL, 1, BigintRange.of(0, Integer.MAX_VALUE, false)),
ImmutableMap.of(0, IS_NOT_NULL, 1, BigintRange.of(0, Integer.MAX_VALUE, false))));
}
Expand Down

0 comments on commit 7c28d0b

Please sign in to comment.