Skip to content

Conversation

SongChujun
Copy link
Member

@SongChujun SongChujun commented Sep 24, 2025

Description

This PR add more tests to TestRowBlock

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Sep 24, 2025
@SongChujun SongChujun marked this pull request as ready for review September 24, 2025 16:04
List<Object>[] testRows = generateTestRows(fieldTypes, 5);
RowBlock rowBlock = (RowBlock) createBlockBuilderWithValues(fieldTypes, testRows).build();

int[] positions = {0, 2, 4};
Copy link
Member

Choose a reason for hiding this comment

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

Nit: these can be inlined into the copyPositions calls since they're not referenced afterwards.

// Test with nulls
List<Object>[] testRowsWithNulls = alternatingNullValues(generateTestRows(fieldTypes, 3));
RowBlock rowBlockWithNulls = (RowBlock) createBlockBuilderWithValues(fieldTypes, testRowsWithNulls).build().getRegion(1, testRowsWithNulls.length - 1);
int[] positions2 = {0, 2};
Copy link
Member

Choose a reason for hiding this comment

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

Note about inlining these


// Test empty positions
int[] emptyPositions = {};
RowBlock emptyBlock = rowBlock.copyPositions(emptyPositions, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: inline as new int[0]

RowBlock regionBlock = largerBlock.getRegion(1, 2);
RowBlock offsetAppendedBlock = regionBlock.copyWithAppendedNull();
assertThat(offsetAppendedBlock.getPositionCount()).isEqualTo(3);
for (int i = 0; i < 2; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Probably unnecessary to assert these in a two-iteration loop, just unroll them this into two separate assertions.

@SongChujun SongChujun force-pushed the rowblock_copyWithAppendedNull_fix branch from 9314230 to 4516abb Compare September 25, 2025 18:54
Copy link
Member

@pettyjamesm pettyjamesm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @SongChujun

@pettyjamesm pettyjamesm merged commit a8d07e4 into trinodb:master Sep 25, 2025
188 of 189 checks passed
@github-actions github-actions bot added this to the 478 milestone Sep 25, 2025
@SongChujun SongChujun deleted the rowblock_copyWithAppendedNull_fix branch September 25, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants