From 79f480bc807954c4486b670c42015fb9c396f010 Mon Sep 17 00:00:00 2001 From: Ying Su Date: Tue, 8 Jan 2019 12:24:03 -0800 Subject: [PATCH] Revert lazy build hashtable This reverts 1) commit ad05dcbda5a9da048a255589e6c86d75ec61cf1e. 2) commit 23de11f7d312866101fbf3b0465703a1736cffef. PR #11791 (commit 23de11f and ad05dcb), which lazily builds the hashtables for maps, introduced a regression for the case where the MapBlock is created through AbstractMapBlock.getRegion(). The hashtables built on the MapBlock region were not updated in the original MapBlock, thus causing hashtables repeatedly being built on the same base MapBlock. --- .../facebook/presto/block/TestMapBlock.java | 29 ----- .../presto/spi/block/AbstractMapBlock.java | 77 +++++-------- .../facebook/presto/spi/block/MapBlock.java | 103 +++++------------- .../presto/spi/block/MapBlockBuilder.java | 58 +++------- .../presto/spi/block/MapBlockEncoding.java | 30 ++--- .../presto/spi/block/SingleMapBlock.java | 99 ++++++++--------- .../spi/block/SingleMapBlockEncoding.java | 48 ++------ .../com/facebook/presto/spi/type/MapType.java | 4 +- 8 files changed, 130 insertions(+), 318 deletions(-) diff --git a/presto-main/src/test/java/com/facebook/presto/block/TestMapBlock.java b/presto-main/src/test/java/com/facebook/presto/block/TestMapBlock.java index da97247b12345..87239562767ca 100644 --- a/presto-main/src/test/java/com/facebook/presto/block/TestMapBlock.java +++ b/presto-main/src/test/java/com/facebook/presto/block/TestMapBlock.java @@ -27,7 +27,6 @@ import org.testng.annotations.Test; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -81,34 +80,6 @@ public void testCompactBlock() testIncompactBlock(mapType(TINYINT, TINYINT).createBlockFromKeyValue(Optional.of(mapIsNull), offsets, inCompactKeyBlock, inCompactValueBlock)); } - // TODO: remove this test when we have a more unified testWith() using assertBlock() - @Test - public void testLazyHashTableBuildOverBlockRegion() - { - Map[] values = createTestMap(9, 3, 4, 0, 8, 0, 6, 5); - Block block = createBlockWithValuesFromKeyValueBlock(values); - BlockBuilder blockBuilder = createBlockBuilderWithValues(values); - - // Create a MapBlock that is a region of another MapBlock. It doesn't have hashtables built at the time of creation. - int offset = block.getPositionCount() / 2; - Block blockRegion = block.getRegion(offset, block.getPositionCount() - offset); - - // Lazily build the hashtables for the block region and use them to do position/value check. - Map[] expectedValues = Arrays.copyOfRange(values, values.length / 2, values.length); - assertBlock(blockRegion, () -> blockBuilder.newBlockBuilderLike(null), expectedValues); - - Map[] valuesWithNull = alternatingNullValues(values); - Block blockWithNull = createBlockWithValuesFromKeyValueBlock(valuesWithNull); - - // Create a MapBlock that is a region of another MapBlock with null values. It doesn't have hashtables built at the time of creation. - offset = blockWithNull.getPositionCount() / 2; - Block blockRegionWithNull = blockWithNull.getRegion(offset, blockWithNull.getPositionCount() - offset); - - // Lazily build the hashtables for the block region and use them to do position/value check. - Map[] expectedValuesWithNull = Arrays.copyOfRange(valuesWithNull, valuesWithNull.length / 2, valuesWithNull.length); - assertBlock(blockRegionWithNull, () -> blockBuilder.newBlockBuilderLike(null), expectedValuesWithNull); - } - private Map[] createTestMap(int... entryCounts) { Map[] result = new Map[entryCounts.length]; diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/block/AbstractMapBlock.java b/presto-spi/src/main/java/com/facebook/presto/spi/block/AbstractMapBlock.java index 714c038d50ee9..8b86570047f0b 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/block/AbstractMapBlock.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/block/AbstractMapBlock.java @@ -39,23 +39,20 @@ public abstract class AbstractMapBlock protected final Type keyType; protected final MethodHandle keyNativeHashCode; protected final MethodHandle keyBlockNativeEquals; - protected final MethodHandle keyBlockHashCode; - public AbstractMapBlock(Type keyType, MethodHandle keyNativeHashCode, MethodHandle keyBlockNativeEquals, MethodHandle keyBlockHashCode) + public AbstractMapBlock(Type keyType, MethodHandle keyNativeHashCode, MethodHandle keyBlockNativeEquals) { this.keyType = requireNonNull(keyType, "keyType is null"); // keyNativeHashCode can only be null due to map block kill switch. deprecated.new-map-block this.keyNativeHashCode = keyNativeHashCode; // keyBlockNativeEquals can only be null due to map block kill switch. deprecated.new-map-block this.keyBlockNativeEquals = keyBlockNativeEquals; - this.keyBlockHashCode = requireNonNull(keyBlockHashCode, "keyBlockHashCode is null"); } protected abstract Block getRawKeyBlock(); protected abstract Block getRawValueBlock(); - @Nullable protected abstract int[] getHashTables(); /** @@ -73,8 +70,6 @@ public AbstractMapBlock(Type keyType, MethodHandle keyNativeHashCode, MethodHand @Nullable protected abstract boolean[] getMapIsNull(); - protected abstract void ensureHashTableLoaded(); - int getOffset(int position) { return getOffsets()[position + getOffsetBase()]; @@ -117,35 +112,21 @@ public Block copyPositions(int[] positions, int offset, int length) } int[] hashTable = getHashTables(); - int[] newHashTable = null; - if (hashTable != null) { - newHashTable = new int[newOffsets[newOffsets.length - 1] * HASH_MULTIPLIER]; - int newHashIndex = 0; - for (int i = offset; i < offset + length; ++i) { - int position = positions[i]; - int entriesStartOffset = getOffset(position); - int entriesEndOffset = getOffset(position + 1); - for (int hashIndex = entriesStartOffset * HASH_MULTIPLIER; hashIndex < entriesEndOffset * HASH_MULTIPLIER; hashIndex++) { - newHashTable[newHashIndex] = hashTable[hashIndex]; - newHashIndex++; - } + int[] newHashTable = new int[newOffsets[newOffsets.length - 1] * HASH_MULTIPLIER]; + int newHashIndex = 0; + for (int i = offset; i < offset + length; ++i) { + int position = positions[i]; + int entriesStartOffset = getOffset(position); + int entriesEndOffset = getOffset(position + 1); + for (int hashIndex = entriesStartOffset * HASH_MULTIPLIER; hashIndex < entriesEndOffset * HASH_MULTIPLIER; hashIndex++) { + newHashTable[newHashIndex] = hashTable[hashIndex]; + newHashIndex++; } } Block newKeys = getRawKeyBlock().copyPositions(entriesPositions.elements(), 0, entriesPositions.size()); Block newValues = getRawValueBlock().copyPositions(entriesPositions.elements(), 0, entriesPositions.size()); - return createMapBlockInternal( - 0, - length, - Optional.of(newMapIsNull), - newOffsets, - newKeys, - newValues, - Optional.ofNullable(newHashTable), - keyType, - keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); + return createMapBlockInternal(0, length, Optional.of(newMapIsNull), newOffsets, newKeys, newValues, newHashTable, keyType, keyBlockNativeEquals, keyNativeHashCode); } @Override @@ -161,11 +142,10 @@ public Block getRegion(int position, int length) getOffsets(), getRawKeyBlock(), getRawValueBlock(), - Optional.ofNullable(getHashTables()), + getHashTables(), keyType, keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); + keyNativeHashCode); } @Override @@ -227,12 +207,7 @@ public Block copyRegion(int position, int length) int[] newOffsets = compactOffsets(getOffsets(), position + getOffsetBase(), length); boolean[] mapIsNull = getMapIsNull(); boolean[] newMapIsNull = mapIsNull == null ? null : compactArray(mapIsNull, position + getOffsetBase(), length); - - int[] hashTables = getHashTables(); - int[] newHashTable = null; - if (hashTables != null) { - newHashTable = compactArray(hashTables, startValueOffset * HASH_MULTIPLIER, (endValueOffset - startValueOffset) * HASH_MULTIPLIER); - } + int[] newHashTable = compactArray(getHashTables(), startValueOffset * HASH_MULTIPLIER, (endValueOffset - startValueOffset) * HASH_MULTIPLIER); if (newKeys == getRawKeyBlock() && newValues == getRawValueBlock() && newOffsets == getOffsets() && newMapIsNull == mapIsNull && newHashTable == getHashTables()) { return this; @@ -244,11 +219,10 @@ public Block copyRegion(int position, int length) newOffsets, newKeys, newValues, - Optional.ofNullable(newHashTable), + newHashTable, keyType, keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); + keyNativeHashCode); } @Override @@ -264,7 +238,12 @@ public T getObject(int position, Class clazz) return clazz.cast(new SingleMapBlock( startEntryOffset * 2, (endEntryOffset - startEntryOffset) * 2, - this)); + getRawKeyBlock(), + getRawValueBlock(), + getHashTables(), + keyType, + keyNativeHashCode, + keyBlockNativeEquals)); } @Override @@ -284,12 +263,7 @@ public Block getSingleValueBlock(int position) int valueLength = endValueOffset - startValueOffset; Block newKeys = getRawKeyBlock().copyRegion(startValueOffset, valueLength); Block newValues = getRawValueBlock().copyRegion(startValueOffset, valueLength); - - int[] hashTables = getHashTables(); - int[] newHashTable = null; - if (hashTables != null) { - newHashTable = Arrays.copyOfRange(hashTables, startValueOffset * HASH_MULTIPLIER, endValueOffset * HASH_MULTIPLIER); - } + int[] newHashTable = Arrays.copyOfRange(getHashTables(), startValueOffset * HASH_MULTIPLIER, endValueOffset * HASH_MULTIPLIER); return createMapBlockInternal( 0, @@ -298,11 +272,10 @@ public Block getSingleValueBlock(int position) new int[] {0, valueLength}, newKeys, newValues, - Optional.ofNullable(newHashTable), + newHashTable, keyType, keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); + keyNativeHashCode); } @Override diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlock.java b/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlock.java index f8981f6b603b8..0a51354f1b46c 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlock.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlock.java @@ -27,7 +27,6 @@ import static com.facebook.presto.spi.block.MapBlockBuilder.buildHashTable; import static io.airlift.slice.SizeOf.sizeOf; -import static io.airlift.slice.SizeOf.sizeOfIntArray; import static java.lang.String.format; import static java.util.Objects.requireNonNull; @@ -43,7 +42,7 @@ public class MapBlock private final int[] offsets; private final Block keyBlock; private final Block valueBlock; - private volatile int[] hashTables; // hash to location in map. Writes to the field is protected by "this" monitor. + private final int[] hashTables; // hash to location in map; private volatile long sizeInBytes; private final long retainedSizeInBytes; @@ -69,6 +68,20 @@ public static MapBlock fromKeyValueBlock( validateConstructorArguments(0, offsets.length - 1, mapIsNull.orElse(null), offsets, keyBlock, valueBlock, mapType.getKeyType(), keyBlockNativeEquals, keyNativeHashCode); int mapCount = offsets.length - 1; + int elementCount = keyBlock.getPositionCount(); + int[] hashTables = new int[elementCount * HASH_MULTIPLIER]; + Arrays.fill(hashTables, -1); + for (int i = 0; i < mapCount; i++) { + int keyOffset = offsets[i]; + int keyCount = offsets[i + 1] - keyOffset; + if (keyCount < 0) { + throw new IllegalArgumentException(format("Offset is not monotonically ascending. offsets[%s]=%s, offsets[%s]=%s", i, offsets[i], i + 1, offsets[i + 1])); + } + if (mapIsNull.isPresent() && mapIsNull.get()[i] && keyCount != 0) { + throw new IllegalArgumentException("A null map must have zero entries"); + } + buildHashTable(keyBlock, keyOffset, keyCount, keyBlockHashCode, hashTables, keyOffset * HASH_MULTIPLIER, keyCount * HASH_MULTIPLIER); + } return createMapBlockInternal( 0, @@ -77,11 +90,10 @@ public static MapBlock fromKeyValueBlock( offsets, keyBlock, valueBlock, - Optional.empty(), + hashTables, mapType.getKeyType(), keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); + keyNativeHashCode); } /** @@ -100,25 +112,13 @@ public static MapBlock createMapBlockInternal( int[] offsets, Block keyBlock, Block valueBlock, - Optional hashTables, + int[] hashTables, Type keyType, MethodHandle keyBlockNativeEquals, - MethodHandle keyNativeHashCode, - MethodHandle keyBlockHashCode) + MethodHandle keyNativeHashCode) { validateConstructorArguments(startOffset, positionCount, mapIsNull.orElse(null), offsets, keyBlock, valueBlock, keyType, keyBlockNativeEquals, keyNativeHashCode); - return new MapBlock( - startOffset, - positionCount, - mapIsNull.orElse(null), - offsets, - keyBlock, - valueBlock, - hashTables.orElse(null), - keyType, - keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); + return new MapBlock(startOffset, positionCount, mapIsNull.orElse(null), offsets, keyBlock, valueBlock, hashTables, keyType, keyBlockNativeEquals, keyNativeHashCode); } private static void validateConstructorArguments( @@ -171,15 +171,15 @@ private MapBlock( int[] offsets, Block keyBlock, Block valueBlock, - @Nullable int[] hashTables, + int[] hashTables, Type keyType, MethodHandle keyBlockNativeEquals, - MethodHandle keyNativeHashCode, - MethodHandle keyBlockHashCode) + MethodHandle keyNativeHashCode) { - super(keyType, keyNativeHashCode, keyBlockNativeEquals, keyBlockHashCode); + super(keyType, keyNativeHashCode, keyBlockNativeEquals); - if (hashTables != null && hashTables.length < keyBlock.getPositionCount() * HASH_MULTIPLIER) { + requireNonNull(hashTables, "hashTables is null"); + if (hashTables.length < keyBlock.getPositionCount() * HASH_MULTIPLIER) { throw new IllegalArgumentException(format("keyBlock/valueBlock size does not match hash table size: %s %s", keyBlock.getPositionCount(), hashTables.length)); } @@ -192,16 +192,7 @@ private MapBlock( this.hashTables = hashTables; this.sizeInBytes = -1; - - // We will add the hashtable size to the retained size even if it's not built yet. This could be overestimating - // but is necessary to avoid reliability issues. Currently the memory counting framework only pull the retained - // size once for each operator so updating in the middle of the processing would not work. - this.retainedSizeInBytes = INSTANCE_SIZE - + keyBlock.getRetainedSizeInBytes() - + valueBlock.getRetainedSizeInBytes() - + sizeOf(offsets) - + sizeOf(mapIsNull) - + sizeOfIntArray(keyBlock.getPositionCount() * HASH_MULTIPLIER); // hashtable size if it was built + this.retainedSizeInBytes = INSTANCE_SIZE + keyBlock.getRetainedSizeInBytes() + valueBlock.getRetainedSizeInBytes() + sizeOf(offsets) + sizeOf(mapIsNull) + sizeOf(hashTables); } @Override @@ -312,47 +303,9 @@ public Block getLoadedBlock() offsets, keyBlock, loadedValueBlock, - Optional.ofNullable(hashTables), + hashTables, keyType, keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); - } - - @Override - protected void ensureHashTableLoaded() - { - if (this.hashTables != null) { - return; - } - - // This can only happen for MapBlock, not MapBlockBuilder because the latter always has non-null hashtables - synchronized (this) { - if (this.hashTables != null) { - return; - } - - int[] hashTables = new int[getRawKeyBlock().getPositionCount() * HASH_MULTIPLIER]; - Arrays.fill(hashTables, -1); - for (int i = 0; i < offsets.length - 1; i++) { - int keyOffset = offsets[i]; - int keyCount = offsets[i + 1] - keyOffset; - if (keyCount < 0) { - throw new IllegalArgumentException(format("Offset is not monotonically ascending. offsets[%s]=%s, offsets[%s]=%s", i, offsets[i], i + 1, offsets[i + 1])); - } - if (mapIsNull != null && mapIsNull[i] && keyCount != 0) { - throw new IllegalArgumentException("A null map must have zero entries"); - } - buildHashTable( - getRawKeyBlock(), - keyOffset, - keyCount, - keyBlockHashCode, - hashTables, - keyOffset * HASH_MULTIPLIER, - keyCount * HASH_MULTIPLIER); - } - this.hashTables = hashTables; - } + keyNativeHashCode); } } diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockBuilder.java b/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockBuilder.java index d7f3e6e979c3a..d4b41cea4d6e9 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockBuilder.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockBuilder.java @@ -39,6 +39,7 @@ public class MapBlockBuilder private static final int INSTANCE_SIZE = ClassLayout.parseClass(MapBlockBuilder.class).instanceSize(); private final MethodHandle keyBlockEquals; + private final MethodHandle keyBlockHashCode; @Nullable private final BlockBuilderStatus blockBuilderStatus; @@ -89,9 +90,10 @@ private MapBlockBuilder( boolean[] mapIsNull, int[] hashTables) { - super(keyType, keyNativeHashCode, keyBlockNativeEquals, keyBlockHashCode); + super(keyType, keyNativeHashCode, keyBlockNativeEquals); this.keyBlockEquals = requireNonNull(keyBlockEquals, "keyBlockEquals is null"); + this.keyBlockHashCode = requireNonNull(keyBlockHashCode, "keyBlockHashCode is null"); this.blockBuilderStatus = blockBuilderStatus; this.positionCount = 0; @@ -155,12 +157,7 @@ public long getSizeInBytes() @Override public long getRetainedSizeInBytes() { - long size = INSTANCE_SIZE - + keyBlockBuilder.getRetainedSizeInBytes() - + valueBlockBuilder.getRetainedSizeInBytes() - + sizeOf(offsets) - + sizeOf(mapIsNull) - + sizeOf(hashTables); + long size = INSTANCE_SIZE + keyBlockBuilder.getRetainedSizeInBytes() + valueBlockBuilder.getRetainedSizeInBytes() + sizeOf(offsets) + sizeOf(mapIsNull) + sizeOf(hashTables); if (blockBuilderStatus != null) { size += BlockBuilderStatus.INSTANCE_SIZE; } @@ -202,14 +199,7 @@ public BlockBuilder closeEntry() int previousAggregatedEntryCount = offsets[positionCount - 1]; int aggregatedEntryCount = offsets[positionCount]; int entryCount = aggregatedEntryCount - previousAggregatedEntryCount; - buildHashTable( - keyBlockBuilder, - previousAggregatedEntryCount, - entryCount, - keyBlockHashCode, - hashTables, - previousAggregatedEntryCount * HASH_MULTIPLIER, - entryCount * HASH_MULTIPLIER); + buildHashTable(keyBlockBuilder, previousAggregatedEntryCount, entryCount, keyBlockHashCode, hashTables, previousAggregatedEntryCount * HASH_MULTIPLIER, entryCount * HASH_MULTIPLIER); return this; } @@ -246,7 +236,7 @@ public BlockBuilder closeEntryStrict() return this; } - private BlockBuilder closeEntry(@Nullable int[] providedHashTable, int providedHashTableOffset) + private BlockBuilder closeEntry(int[] providedHashTable, int providedHashTableOffset) { if (!currentEntryOpened) { throw new IllegalStateException("Expected entry to be opened but was closed"); @@ -256,29 +246,14 @@ private BlockBuilder closeEntry(@Nullable int[] providedHashTable, int providedH currentEntryOpened = false; ensureHashTableSize(); - int previousAggregatedEntryCount = offsets[positionCount - 1]; - int aggregatedEntryCount = offsets[positionCount]; - if (providedHashTable != null) { - // Directly copy instead of building hashtable if providedHashTable is not null - int hashTableOffset = previousAggregatedEntryCount * HASH_MULTIPLIER; - int hashTableSize = (aggregatedEntryCount - previousAggregatedEntryCount) * HASH_MULTIPLIER; - for (int i = 0; i < hashTableSize; i++) { - hashTables[hashTableOffset + i] = providedHashTable[providedHashTableOffset + i]; - } - } - else { - // Build hash table for this map entry. - int entryCount = aggregatedEntryCount - previousAggregatedEntryCount; - buildHashTable( - keyBlockBuilder, - previousAggregatedEntryCount, - entryCount, - keyBlockHashCode, - hashTables, - previousAggregatedEntryCount * HASH_MULTIPLIER, - entryCount * HASH_MULTIPLIER); + // Directly copy instead of building hashtable + int hashTableOffset = offsets[positionCount - 1] * HASH_MULTIPLIER; + int hashTableSize = (offsets[positionCount] - offsets[positionCount - 1]) * HASH_MULTIPLIER; + for (int i = 0; i < hashTableSize; i++) { + hashTables[hashTableOffset + i] = providedHashTable[providedHashTableOffset + i]; } + return this; } @@ -336,11 +311,9 @@ public Block build() offsets, keyBlockBuilder.build(), valueBlockBuilder.build(), - Optional.of(Arrays.copyOf(hashTables, offsets[positionCount] * HASH_MULTIPLIER)), + Arrays.copyOf(hashTables, offsets[positionCount] * HASH_MULTIPLIER), keyType, - keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); + keyBlockNativeEquals, keyNativeHashCode); } @Override @@ -437,9 +410,6 @@ public BlockBuilder newBlockBuilderLike(BlockBuilderStatus blockBuilderStatus) newNegativeOneFilledArray(newSize * HASH_MULTIPLIER)); } - @Override - protected void ensureHashTableLoaded() {} - private static int[] newNegativeOneFilledArray(int size) { int[] hashTable = new int[size]; diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockEncoding.java b/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockEncoding.java index d03904f5af3ba..cc0b43537b653 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockEncoding.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlockEncoding.java @@ -64,15 +64,8 @@ public void writeBlock(BlockEncodingSerde blockEncodingSerde, SliceOutput sliceO blockEncodingSerde.writeBlock(sliceOutput, mapBlock.getRawKeyBlock().getRegion(entriesStartOffset, entriesEndOffset - entriesStartOffset)); blockEncodingSerde.writeBlock(sliceOutput, mapBlock.getRawValueBlock().getRegion(entriesStartOffset, entriesEndOffset - entriesStartOffset)); - if (hashTable != null) { - int hashTableLength = (entriesEndOffset - entriesStartOffset) * HASH_MULTIPLIER; - sliceOutput.appendInt(hashTableLength); // hashtable length - sliceOutput.writeBytes(wrappedIntArray(hashTable, entriesStartOffset * HASH_MULTIPLIER, hashTableLength)); - } - else { - // if the hashTable is null, we write the length -1 - sliceOutput.appendInt(-1); // hashtable length - } + sliceOutput.appendInt((entriesEndOffset - entriesStartOffset) * HASH_MULTIPLIER); + sliceOutput.writeBytes(wrappedIntArray(hashTable, entriesStartOffset * HASH_MULTIPLIER, (entriesEndOffset - entriesStartOffset) * HASH_MULTIPLIER)); sliceOutput.appendInt(positionCount); for (int position = 0; position < positionCount + 1; position++) { @@ -89,27 +82,18 @@ public Block readBlock(BlockEncodingSerde blockEncodingSerde, SliceInput sliceIn Block keyBlock = blockEncodingSerde.readBlock(sliceInput); Block valueBlock = blockEncodingSerde.readBlock(sliceInput); - int hashTableLength = sliceInput.readInt(); - int[] hashTable = null; - if (hashTableLength >= 0) { - hashTable = new int[hashTableLength]; - sliceInput.readBytes(wrappedIntArray(hashTable)); - } - - if (keyBlock.getPositionCount() != valueBlock.getPositionCount()) { - throw new IllegalArgumentException( - format("Deserialized MapBlock violates invariants: key %d, value %d", keyBlock.getPositionCount(), valueBlock.getPositionCount())); - } + int[] hashTable = new int[sliceInput.readInt()]; + sliceInput.readBytes(wrappedIntArray(hashTable)); - if (hashTable != null && keyBlock.getPositionCount() * HASH_MULTIPLIER != hashTable.length) { + if (keyBlock.getPositionCount() != valueBlock.getPositionCount() || keyBlock.getPositionCount() * HASH_MULTIPLIER != hashTable.length) { throw new IllegalArgumentException( - format("Deserialized MapBlock violates invariants: expected hashtable size %d, actual hashtable size %d", keyBlock.getPositionCount() * HASH_MULTIPLIER, hashTable.length)); + format("Deserialized MapBlock violates invariants: key %d, value %d, hash %d", keyBlock.getPositionCount(), valueBlock.getPositionCount(), hashTable.length)); } int positionCount = sliceInput.readInt(); int[] offsets = new int[positionCount + 1]; sliceInput.readBytes(wrappedIntArray(offsets)); Optional mapIsNull = EncoderUtil.decodeNullBits(sliceInput, positionCount); - return MapType.createMapBlockInternal(typeManager, keyType, 0, positionCount, mapIsNull, offsets, keyBlock, valueBlock, Optional.ofNullable(hashTable)); + return MapType.createMapBlockInternal(typeManager, keyType, 0, positionCount, mapIsNull, offsets, keyBlock, valueBlock, hashTable); } } diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlock.java b/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlock.java index 9b7a2666d75f9..6abae88e01329 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlock.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlock.java @@ -19,6 +19,7 @@ import io.airlift.slice.Slice; import org.openjdk.jol.info.ClassLayout; +import java.lang.invoke.MethodHandle; import java.util.function.BiConsumer; import static com.facebook.presto.spi.StandardErrorCode.GENERIC_INTERNAL_ERROR; @@ -35,14 +36,24 @@ public class SingleMapBlock private static final int INSTANCE_SIZE = ClassLayout.parseClass(SingleMapBlock.class).instanceSize(); private final int offset; - private final int positionCount; // The number of keys in this single map * 2 - private final AbstractMapBlock mapBlock; - - SingleMapBlock(int offset, int positionCount, AbstractMapBlock mapBlock) + private final int positionCount; + private final Block keyBlock; + private final Block valueBlock; + private final int[] hashTable; + final Type keyType; + private final MethodHandle keyNativeHashCode; + private final MethodHandle keyBlockNativeEquals; + + SingleMapBlock(int offset, int positionCount, Block keyBlock, Block valueBlock, int[] hashTable, Type keyType, MethodHandle keyNativeHashCode, MethodHandle keyBlockNativeEquals) { this.offset = offset; this.positionCount = positionCount; - this.mapBlock = mapBlock; + this.keyBlock = keyBlock; + this.valueBlock = valueBlock; + this.hashTable = hashTable; + this.keyType = keyType; + this.keyNativeHashCode = keyNativeHashCode; + this.keyBlockNativeEquals = keyBlockNativeEquals; } @Override @@ -54,23 +65,23 @@ public int getPositionCount() @Override public long getSizeInBytes() { - return mapBlock.getRawKeyBlock().getRegionSizeInBytes(offset / 2, positionCount / 2) + - mapBlock.getRawValueBlock().getRegionSizeInBytes(offset / 2, positionCount / 2) + + return keyBlock.getRegionSizeInBytes(offset / 2, positionCount / 2) + + valueBlock.getRegionSizeInBytes(offset / 2, positionCount / 2) + sizeOfIntArray(positionCount / 2 * HASH_MULTIPLIER); } @Override public long getRetainedSizeInBytes() { - return INSTANCE_SIZE + mapBlock.getRawKeyBlock().getRetainedSizeInBytes() + mapBlock.getRawValueBlock().getRetainedSizeInBytes() + sizeOf(mapBlock.getHashTables()); + return INSTANCE_SIZE + keyBlock.getRetainedSizeInBytes() + valueBlock.getRetainedSizeInBytes() + sizeOf(hashTable); } @Override public void retainedBytesForEachPart(BiConsumer consumer) { - consumer.accept(mapBlock.getRawKeyBlock(), mapBlock.getRawKeyBlock().getRetainedSizeInBytes()); - consumer.accept(mapBlock.getRawValueBlock(), mapBlock.getRawValueBlock().getRetainedSizeInBytes()); - consumer.accept(mapBlock.getHashTables(), sizeOf(mapBlock.getHashTables())); + consumer.accept(keyBlock, keyBlock.getRetainedSizeInBytes()); + consumer.accept(valueBlock, valueBlock.getRetainedSizeInBytes()); + consumer.accept(hashTable, sizeOf(hashTable)); consumer.accept(this, (long) INSTANCE_SIZE); } @@ -89,13 +100,13 @@ public int getOffset() @Override Block getRawKeyBlock() { - return mapBlock.getRawKeyBlock(); + return keyBlock; } @Override Block getRawValueBlock() { - return mapBlock.getRawValueBlock(); + return valueBlock; } @Override @@ -107,29 +118,29 @@ public String toString() @Override public Block getLoadedBlock() { - if (mapBlock.getRawKeyBlock() != mapBlock.getRawKeyBlock().getLoadedBlock()) { + if (keyBlock != keyBlock.getLoadedBlock()) { // keyBlock has to be loaded since MapBlock constructs hash table eagerly. throw new IllegalStateException(); } - Block loadedValueBlock = mapBlock.getRawValueBlock().getLoadedBlock(); - if (loadedValueBlock == mapBlock.getRawValueBlock()) { + Block loadedValueBlock = valueBlock.getLoadedBlock(); + if (loadedValueBlock == valueBlock) { return this; } return new SingleMapBlock( offset, positionCount, - mapBlock); + keyBlock, + loadedValueBlock, + hashTable, + keyType, + keyNativeHashCode, + keyBlockNativeEquals); } int[] getHashTable() { - return mapBlock.getHashTables(); - } - - Type getKeyType() - { - return mapBlock.keyType; + return hashTable; } /** @@ -141,12 +152,9 @@ public int seekKey(Object nativeValue) return -1; } - mapBlock.ensureHashTableLoaded(); - int[] hashTable = mapBlock.getHashTables(); - long hashCode; try { - hashCode = (long) mapBlock.keyNativeHashCode.invoke(nativeValue); + hashCode = (long) keyNativeHashCode.invoke(nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -163,7 +171,7 @@ public int seekKey(Object nativeValue) Boolean match; try { // assuming maps with indeterminate keys are not supported - match = (Boolean) mapBlock.keyBlockNativeEquals.invoke(mapBlock.getRawKeyBlock(), offset / 2 + keyPosition, nativeValue); + match = (Boolean) keyBlockNativeEquals.invoke(keyBlock, offset / 2 + keyPosition, nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -188,12 +196,9 @@ public int seekKeyExact(long nativeValue) return -1; } - mapBlock.ensureHashTableLoaded(); - int[] hashTable = mapBlock.getHashTables(); - long hashCode; try { - hashCode = (long) mapBlock.keyNativeHashCode.invokeExact(nativeValue); + hashCode = (long) keyNativeHashCode.invokeExact(nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -210,7 +215,7 @@ public int seekKeyExact(long nativeValue) Boolean match; try { // assuming maps with indeterminate keys are not supported - match = (Boolean) mapBlock.keyBlockNativeEquals.invokeExact(mapBlock.getRawKeyBlock(), offset / 2 + keyPosition, nativeValue); + match = (Boolean) keyBlockNativeEquals.invokeExact(keyBlock, offset / 2 + keyPosition, nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -232,12 +237,9 @@ public int seekKeyExact(boolean nativeValue) return -1; } - mapBlock.ensureHashTableLoaded(); - int[] hashTable = mapBlock.getHashTables(); - long hashCode; try { - hashCode = (long) mapBlock.keyNativeHashCode.invokeExact(nativeValue); + hashCode = (long) keyNativeHashCode.invokeExact(nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -254,7 +256,7 @@ public int seekKeyExact(boolean nativeValue) Boolean match; try { // assuming maps with indeterminate keys are not supported - match = (Boolean) mapBlock.keyBlockNativeEquals.invokeExact(mapBlock.getRawKeyBlock(), offset / 2 + keyPosition, nativeValue); + match = (Boolean) keyBlockNativeEquals.invokeExact(keyBlock, offset / 2 + keyPosition, nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -276,12 +278,9 @@ public int seekKeyExact(double nativeValue) return -1; } - mapBlock.ensureHashTableLoaded(); - int[] hashTable = mapBlock.getHashTables(); - long hashCode; try { - hashCode = (long) mapBlock.keyNativeHashCode.invokeExact(nativeValue); + hashCode = (long) keyNativeHashCode.invokeExact(nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -298,7 +297,7 @@ public int seekKeyExact(double nativeValue) Boolean match; try { // assuming maps with indeterminate keys are not supported - match = (Boolean) mapBlock.keyBlockNativeEquals.invokeExact(mapBlock.getRawKeyBlock(), offset / 2 + keyPosition, nativeValue); + match = (Boolean) keyBlockNativeEquals.invokeExact(keyBlock, offset / 2 + keyPosition, nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -320,12 +319,9 @@ public int seekKeyExact(Slice nativeValue) return -1; } - mapBlock.ensureHashTableLoaded(); - int[] hashTable = mapBlock.getHashTables(); - long hashCode; try { - hashCode = (long) mapBlock.keyNativeHashCode.invokeExact(nativeValue); + hashCode = (long) keyNativeHashCode.invokeExact(nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -342,7 +338,7 @@ public int seekKeyExact(Slice nativeValue) Boolean match; try { // assuming maps with indeterminate keys are not supported - match = (Boolean) mapBlock.keyBlockNativeEquals.invokeExact(mapBlock.getRawKeyBlock(), offset / 2 + keyPosition, nativeValue); + match = (Boolean) keyBlockNativeEquals.invokeExact(keyBlock, offset / 2 + keyPosition, nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -364,12 +360,9 @@ public int seekKeyExact(Block nativeValue) return -1; } - mapBlock.ensureHashTableLoaded(); - int[] hashTable = mapBlock.getHashTables(); - long hashCode; try { - hashCode = (long) mapBlock.keyNativeHashCode.invokeExact(nativeValue); + hashCode = (long) keyNativeHashCode.invokeExact(nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); @@ -386,7 +379,7 @@ public int seekKeyExact(Block nativeValue) Boolean match; try { // assuming maps with indeterminate keys are not supported - match = (Boolean) mapBlock.keyBlockNativeEquals.invokeExact(mapBlock.getRawKeyBlock(), offset / 2 + keyPosition, nativeValue); + match = (Boolean) keyBlockNativeEquals.invokeExact(keyBlock, offset / 2 + keyPosition, nativeValue); } catch (Throwable throwable) { throw handleThrowable(throwable); diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlockEncoding.java b/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlockEncoding.java index 6eee30661e0fb..0b28dd4b728f3 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlockEncoding.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/block/SingleMapBlockEncoding.java @@ -22,7 +22,6 @@ import io.airlift.slice.SliceOutput; import java.lang.invoke.MethodHandle; -import java.util.Optional; import static com.facebook.presto.spi.block.AbstractMapBlock.HASH_MULTIPLIER; import static com.facebook.presto.spi.block.MethodHandleUtil.compose; @@ -55,23 +54,15 @@ public String getName() public void writeBlock(BlockEncodingSerde blockEncodingSerde, SliceOutput sliceOutput, Block block) { SingleMapBlock singleMapBlock = (SingleMapBlock) block; - TypeSerde.writeType(sliceOutput, singleMapBlock.getKeyType()); + TypeSerde.writeType(sliceOutput, singleMapBlock.keyType); int offset = singleMapBlock.getOffset(); int positionCount = singleMapBlock.getPositionCount(); blockEncodingSerde.writeBlock(sliceOutput, singleMapBlock.getRawKeyBlock().getRegion(offset / 2, positionCount / 2)); blockEncodingSerde.writeBlock(sliceOutput, singleMapBlock.getRawValueBlock().getRegion(offset / 2, positionCount / 2)); int[] hashTable = singleMapBlock.getHashTable(); - - if (hashTable != null) { - int hashTableLength = positionCount / 2 * HASH_MULTIPLIER; - sliceOutput.appendInt(hashTableLength); // hashtable length - sliceOutput.writeBytes(wrappedIntArray(hashTable, offset / 2 * HASH_MULTIPLIER, hashTableLength)); - } - else { - // if the hashTable is null, we write the length -1 - sliceOutput.appendInt(-1); - } + sliceOutput.appendInt(positionCount / 2 * HASH_MULTIPLIER); + sliceOutput.writeBytes(wrappedIntArray(hashTable, offset / 2 * HASH_MULTIPLIER, positionCount / 2 * HASH_MULTIPLIER)); } @Override @@ -81,41 +72,18 @@ public Block readBlock(BlockEncodingSerde blockEncodingSerde, SliceInput sliceIn MethodHandle keyNativeEquals = typeManager.resolveOperator(OperatorType.EQUAL, asList(keyType, keyType)); MethodHandle keyBlockNativeEquals = compose(keyNativeEquals, nativeValueGetter(keyType)); MethodHandle keyNativeHashCode = typeManager.resolveOperator(OperatorType.HASH_CODE, singletonList(keyType)); - MethodHandle keyBlockHashCode = compose(keyNativeHashCode, nativeValueGetter(keyType)); Block keyBlock = blockEncodingSerde.readBlock(sliceInput); Block valueBlock = blockEncodingSerde.readBlock(sliceInput); - int hashTableLength = sliceInput.readInt(); - int[] hashTable = null; - if (hashTableLength >= 0) { - hashTable = new int[hashTableLength]; - sliceInput.readBytes(wrappedIntArray(hashTable)); - } - - if (keyBlock.getPositionCount() != valueBlock.getPositionCount()) { - throw new IllegalArgumentException( - format("Deserialized SingleMapBlock violates invariants: key %d, value %d", keyBlock.getPositionCount(), valueBlock.getPositionCount())); - } + int[] hashTable = new int[sliceInput.readInt()]; + sliceInput.readBytes(wrappedIntArray(hashTable)); - if (hashTable != null && keyBlock.getPositionCount() * HASH_MULTIPLIER != hashTable.length) { + if (keyBlock.getPositionCount() != valueBlock.getPositionCount() || keyBlock.getPositionCount() * HASH_MULTIPLIER != hashTable.length) { throw new IllegalArgumentException( - format("Deserialized SingleMapBlock violates invariants: expected hashtable size %d, actual hashtable size %d", keyBlock.getPositionCount() * HASH_MULTIPLIER, hashTable.length)); + format("Deserialized SingleMapBlock violates invariants: key %d, value %d, hash %d", keyBlock.getPositionCount(), valueBlock.getPositionCount(), hashTable.length)); } - MapBlock mapBlock = MapBlock.createMapBlockInternal( - 0, - 1, - Optional.empty(), - new int[] {0, keyBlock.getPositionCount()}, - keyBlock, - valueBlock, - Optional.ofNullable(hashTable), - keyType, - keyBlockNativeEquals, - keyNativeHashCode, - keyBlockHashCode); - - return new SingleMapBlock(0, keyBlock.getPositionCount() * 2, mapBlock); + return new SingleMapBlock(0, keyBlock.getPositionCount() * 2, keyBlock, valueBlock, hashTable, keyType, keyNativeHashCode, keyBlockNativeEquals); } } diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/type/MapType.java b/presto-spi/src/main/java/com/facebook/presto/spi/type/MapType.java index 2da884b04aae6..8a6e4b940f687 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/type/MapType.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/type/MapType.java @@ -278,11 +278,11 @@ public static Block createMapBlockInternal( int[] offsets, Block keyBlock, Block valueBlock, - Optional hashTables) + int[] hashTables) { // TypeManager caches types. Therefore, it is important that we go through it instead of coming up with the MethodHandles directly. // BIGINT is chosen arbitrarily here. Any type will do. MapType mapType = (MapType) typeManager.getType(new TypeSignature(StandardTypes.MAP, TypeSignatureParameter.of(keyType.getTypeSignature()), TypeSignatureParameter.of(BIGINT.getTypeSignature()))); - return MapBlock.createMapBlockInternal(startOffset, positionCount, mapIsNull, offsets, keyBlock, valueBlock, hashTables, keyType, mapType.keyBlockNativeEquals, mapType.keyNativeHashCode, mapType.keyBlockHashCode); + return MapBlock.createMapBlockInternal(startOffset, positionCount, mapIsNull, offsets, keyBlock, valueBlock, hashTables, keyType, mapType.keyBlockNativeEquals, mapType.keyNativeHashCode); } }