Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ private static void assertInvalidPosition(Block block, int[] positions, int offs
{
assertThatThrownBy(() -> block.getPositions(positions, offset, length).getLong(0, 0))
.isInstanceOfAny(IllegalArgumentException.class, IndexOutOfBoundsException.class)
.hasMessage("Invalid position %d in block with %d positions", positions[0], block.getPositionCount());
.hasMessage("Invalid position %d and length %d in block with %d positions", positions[0], length, block.getPositionCount());
}

private static void assertInvalidOffset(Block block, int[] positions, int offset, int length)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@
public class TestDictionaryBlock
extends AbstractTestBlock
{
@Test
public void testConstructionIdsOffset()
{
Slice[] expectedValues = createExpectedValues(10);
Block dictionary = createSlicesBlock(expectedValues);

Block block = DictionaryBlock.create(1, 2, dictionary, new int[] {1, 3, 6, 8});
assertThat(block).isInstanceOf(DictionaryBlock.class);
assertThat(block.getPositionCount()).isEqualTo(2);
assertThat(block.getSlice(0, 0, block.getSliceLength(0))).isEqualTo(expectedValues[3]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertBlock?

assertThat(block.getSlice(1, 0, block.getSliceLength(1))).isEqualTo(expectedValues[6]);
}

@Test
public void testConstructionNoPositions()
{
Expand All @@ -64,6 +77,18 @@ public void testConstructionOnePositions()
assertThat(block.getSlice(0, 0, block.getSliceLength(0))).isEqualTo(expectedValues[1]);
}

@Test
public void testConstructionOnePositionsIdsOffset()
{
Slice[] expectedValues = createExpectedValues(10);
Block dictionary = createSlicesBlock(expectedValues);

Block block = DictionaryBlock.create(2, 1, dictionary, new int[] {1, 5, 9});
assertThat(block).isInstanceOf(VariableWidthBlock.class);
assertThat(block.getPositionCount()).isEqualTo(1);
assertThat(block.getSlice(0, 0, block.getSliceLength(0))).isEqualTo(expectedValues[9]);
}

@Test
public void testConstructionUnnestDictionary()
{
Expand All @@ -79,6 +104,21 @@ public void testConstructionUnnestDictionary()
assertThat(actualDictionary).isSameAs(innerDictionary);
}

@Test
public void testConstructionUnnestDictionaryIdsOffset()
{
Slice[] expectedValues = createExpectedValues(10);
Block innerDictionary = createSlicesBlock(expectedValues);
DictionaryBlock dictionary = (DictionaryBlock) DictionaryBlock.create(4, innerDictionary, new int[] {1, 3, 5, 7});

Block block = DictionaryBlock.create(1, 2, dictionary, new int[] {0, 1, 3});
assertThat(block).isInstanceOf(DictionaryBlock.class);
assertBlock(block, new Slice[] {expectedValues[3], expectedValues[7]});

Block actualDictionary = ((DictionaryBlock) block).getDictionary();
assertThat(actualDictionary).isSameAs(innerDictionary);
}

@Test
public void testSizeInBytes()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.trino.spi.block.Block;
import io.trino.spi.block.BlockBuilder;
import io.trino.spi.block.DictionaryBlock;
import io.trino.spi.block.LongArrayBlock;
import io.trino.spi.type.Type;
import org.testng.annotations.Test;

Expand Down Expand Up @@ -100,7 +101,7 @@ public void testDifferentPositions()
JoinProbe probe = joinProbeFactory.createJoinProbe(page);
Page output = lookupJoinPageBuilder.build(probe);
assertEquals(output.getChannelCount(), 2);
assertTrue(output.getBlock(0) instanceof DictionaryBlock);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why type changed here?

assertTrue(output.getBlock(0) instanceof LongArrayBlock);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this assert?

assertEquals(output.getPositionCount(), 0);
lookupJoinPageBuilder.reset();

Expand Down
2 changes: 1 addition & 1 deletion core/trino-spi/src/main/java/io/trino/spi/block/Block.java
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ default Block getPositions(int[] positions, int offset, int length)
{
checkArrayRange(positions, offset, length);

return new DictionaryBlock(offset, length, this, positions);
return DictionaryBlock.create(offset, length, this, positions);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ private static ColumnarRow toColumnarRowFromDictionaryWithoutNulls(DictionaryBlo
Block[] fields = new Block[columnarRow.getFieldCount()];
for (int i = 0; i < fields.length; i++) {
// Reuse the dictionary ids array directly since no nulls are present
fields[i] = new DictionaryBlock(
fields[i] = DictionaryBlock.create(
dictionaryBlock.getRawIdsOffset(),
dictionaryBlock.getPositionCount(),
columnarRow.getField(i),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,24 +54,29 @@ public class DictionaryBlock

public static Block create(int positionCount, Block dictionary, int[] ids)
{
return createInternal(positionCount, dictionary, ids, randomDictionaryId());
return create(0, positionCount, dictionary, ids);
}

public static Block create(int idsOffset, int positionCount, Block dictionary, int[] ids)
{
return createInternal(idsOffset, positionCount, dictionary, ids, randomDictionaryId());
}

/**
* This should not only be used when creating a projection of another dictionary block.
*/
public static Block createProjectedDictionaryBlock(int positionCount, Block dictionary, int[] ids, DictionaryId dictionarySourceId)
{
return createInternal(positionCount, dictionary, ids, dictionarySourceId);
return createInternal(0, positionCount, dictionary, ids, dictionarySourceId);
}

private static Block createInternal(int positionCount, Block dictionary, int[] ids, DictionaryId dictionarySourceId)
private static Block createInternal(int idsOffset, int positionCount, Block dictionary, int[] ids, DictionaryId dictionarySourceId)
{
if (positionCount == 0) {
return dictionary.copyRegion(0, 0);
}
if (positionCount == 1) {
return dictionary.getRegion(ids[0], 1);
return dictionary.getRegion(ids[idsOffset], 1);
}

// if dictionary is an RLE then this can just be a new RLE
Expand All @@ -83,18 +88,14 @@ private static Block createInternal(int positionCount, Block dictionary, int[] i
if (dictionary instanceof DictionaryBlock dictionaryBlock) {
int[] newIds = new int[positionCount];
for (int position = 0; position < positionCount; position++) {
newIds[position] = dictionaryBlock.getId(ids[position]);
newIds[position] = dictionaryBlock.getId(ids[idsOffset + position]);
}
dictionary = dictionaryBlock.getDictionary();
dictionarySourceId = randomDictionaryId();
ids = newIds;
idsOffset = 0;
}
return new DictionaryBlock(0, positionCount, dictionary, ids, false, false, dictionarySourceId);
}

DictionaryBlock(int idsOffset, int positionCount, Block dictionary, int[] ids)
{
this(idsOffset, positionCount, dictionary, ids, false, false, randomDictionaryId());
return new DictionaryBlock(idsOffset, positionCount, dictionary, ids, false, false, dictionarySourceId);
}

private DictionaryBlock(int idsOffset, int positionCount, Block dictionary, int[] ids, boolean dictionaryIsCompacted, boolean isSequentialIds, DictionaryId dictionarySourceId)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constructor is used in multiple places inside DictionaryBlok e.g. copyPositions that can make the dictionary to be RLE.
If we really want to protect ourselves from this case we need a precondition inside this constructor

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, I missed the usages within this class, we need some more changes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking closer at the other usages, the only way we found to make RLE dictionary out of non-RLE dictionary through get/copyPositions is to use a BlockBuilder as the dictionary. Although it's technically allowed, I'm not sure we need to worry about that. It would be odd to create a DictionaryBlock with a BlockBuilder as the dictionary rather than a pre-built Block.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@
public class MergeFileWriter
implements FileWriter
{
private static final Page EMPTY_PAGE = new Page(0);

// The bucketPath looks like this: /root/dir/delta_nnnnnnn_mmmmmmm_ssss/bucket_bbbbb(_aaaa)?
private static final Pattern BUCKET_PATH_MATCHER = Pattern.compile("(?s)(?<rootDir>.*)/(?<dirStart>delta_\\d+_\\d+)_(?<statementId>\\d+)/(?<filenameBase>bucket_(?<bucketNumber>\\d+))(?<attemptId>_\\d+)?$");

Expand Down Expand Up @@ -228,8 +230,11 @@ public PartitionUpdateAndMergeResults getPartitionUpdateAndMergeResults(Partitio
private Page buildDeletePage(Block rowIds, long writeId)
{
ColumnarRow columnarRow = toColumnarRow(rowIds);
checkArgument(!columnarRow.mayHaveNull(), "The rowIdsRowBlock may not have null rows");
int positionCount = rowIds.getPositionCount();
if (positionCount == 0) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move before toColumnarRow(rowIds).

return EMPTY_PAGE;
}
checkArgument(!columnarRow.mayHaveNull(), "The rowIdsRowBlock may not have null rows");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undelated change?

// We've verified that the rowIds block has no null rows, so it's okay to get the field blocks
Block[] blockArray = {
RunLengthEncodedBlock.create(DELETE_OPERATION_BLOCK, positionCount),
Expand Down