Skip to content

Add a replaceColumn method to Page #22493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
elharo merged 1 commit into from
Apr 13, 2024
Merged

Add a replaceColumn method to Page #22493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions presto-common/src/main/java/com/facebook/presto/common/Page.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,24 @@ private long updateRetainedSize()
return retainedSizeInBytes.longValue();
}

/**
* Returns a new page with the same columns as the original page except for the one column replaced.
*
* @param channelIndex the column to replace
* @param column the replacement column
* @return a new page with the replacement column substituted for the old column
*/
public Page replaceColumn(int channelIndex, Block column)
{
Copy link
Copy Markdown
Collaborator

@sdruzkin sdruzkin Apr 12, 2024

Choose a reason for hiding this comment

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

Check and throw if the channelIndex is invalid

Copy link
Copy Markdown
Contributor Author

@elharo elharo Apr 12, 2024

Choose a reason for hiding this comment

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

There are unit tests that verify the correct exception is thrown in this case.

if (column.getPositionCount() != positionCount) {
throw new IllegalArgumentException("New column does not have same number of rows as old column");
Copy link
Copy Markdown
Collaborator

@sdruzkin sdruzkin Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Replace: New column does not have same number of rows as old column -> New block does not have the same number of rows as the page

Technically current page implementation allows to have blocks with different number of rows, but I think it's ok to enforce this constraint.

Copy link
Copy Markdown
Contributor Author

@elharo elharo Apr 12, 2024

Choose a reason for hiding this comment

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

appendColumn and prependColumn do check for this though interestingly, as you point out, the constructor does not.

}

Block[] newBlocks = Arrays.copyOf(blocks, blocks.length);
newBlocks[channelIndex] = column;
return Page.wrapBlocksWithoutCopy(newBlocks.length, newBlocks);
}

private static class DictionaryBlockIndexes
{
private final List<DictionaryBlock> blocks = new ArrayList<>();
Expand Down
104 changes: 104 additions & 0 deletions presto-common/src/test/java/com/facebook/presto/common/TestPage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,110 @@ public void testGetPositions()
}
}

@Test
public void testDropColumn()
{
int entries = 10;
BlockBuilder blockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(blockBuilder, i);
}
Block block = blockBuilder.build();

Page page = new Page(block, block, block);
assertEquals(page.getChannelCount(), 3);
Page newPage = page.dropColumn(1);
assertEquals(page.getChannelCount(), 3, "Page was modified");
assertEquals(newPage.getChannelCount(), 2);

assertEquals(newPage.getBlock(0).getLong(0), 0);
assertEquals(newPage.getBlock(1).getLong(1), 1);
}

@Test
public void testReplaceColumn()
{
int entries = 10;
BlockBuilder blockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(blockBuilder, i);
}
Block block = blockBuilder.build();
Page page = new Page(block, block, block);
assertEquals(page.getBlock(1).getLong(0), 0);

BlockBuilder newBlockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(newBlockBuilder, -i);
}
Block newBlock = newBlockBuilder.build();
Page newPage = page.replaceColumn(1, newBlock);

assertEquals(newPage.getChannelCount(), 3);
assertEquals(newPage.getBlock(1).getLong(0), 0);
assertEquals(newPage.getBlock(1).getLong(1), -1);
}

@Test(expectedExceptions = IndexOutOfBoundsException.class)
public void testReplaceColumn_channelTooLow()
{
int entries = 10;
BlockBuilder blockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(blockBuilder, i);
}
Block block = blockBuilder.build();
Page page = new Page(block, block, block);
assertEquals(page.getBlock(1).getLong(0), 0);

BlockBuilder newBlockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(newBlockBuilder, -i);
}
Block newBlock = newBlockBuilder.build();
page.replaceColumn(-1, newBlock);
}

@Test(expectedExceptions = IndexOutOfBoundsException.class)
public void testReplaceColumn_channelTooHigh()
{
int entries = 10;
BlockBuilder blockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(blockBuilder, i);
}
Block block = blockBuilder.build();
Page page = new Page(block, block, block);
assertEquals(page.getBlock(1).getLong(0), 0);

BlockBuilder newBlockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(newBlockBuilder, -i);
}
Block newBlock = newBlockBuilder.build();
page.replaceColumn(page.getChannelCount(), newBlock);
}

@Test(expectedExceptions = IllegalArgumentException.class)
public void testReplaceColumn_WrongNumberOfRows()
{
int entries = 10;
BlockBuilder blockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries; i++) {
BIGINT.writeLong(blockBuilder, i);
}
Block block = blockBuilder.build();
Page page = new Page(block, block, block);
assertEquals(page.getBlock(1).getLong(0), 0);

BlockBuilder newBlockBuilder = BIGINT.createBlockBuilder(null, entries);
for (int i = 0; i < entries - 5; i++) {
BIGINT.writeLong(newBlockBuilder, -i);
}
Block newBlock = newBlockBuilder.build();
page.replaceColumn(1, newBlock);
}

@Test
public void testRetainedSizeIsCorrect()
{
Expand Down