Skip to content

Produce RunLengthEncodedBlock in RowBlockBuilder when all values are null#12044

Merged
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ls/012-row-has-non-null-row
Apr 22, 2022
Merged

Produce RunLengthEncodedBlock in RowBlockBuilder when all values are null#12044
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ls/012-row-has-non-null-row

Conversation

@lukasz-stec
Copy link
Copy Markdown
Member

@lukasz-stec lukasz-stec commented Apr 20, 2022

Description

Add support in RowBlockBuilder for producing RunLengthEncodedBlock if all positions are null.

Is this change a fix, improvement, new feature, refactoring, or other?

improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

changes only in RowBlockBuilder, AbstractRowBlock classes in the trino-spi, not affecting API.

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Similar to #12043

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 20, 2022
@lukasz-stec lukasz-stec requested a review from sopel39 April 20, 2022 08:37
@findepi findepi changed the title Add hasNonNullValue support to RowBlockBuilder Produce RunLengthEncodedBlock in RowBlockBuilder when all values are null Apr 20, 2022
@lukasz-stec lukasz-stec force-pushed the ls/012-row-has-non-null-row branch from 6ec8e83 to 68759c3 Compare April 20, 2022 09:18
sopel39
sopel39 reviewed Apr 20, 2022
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/block/AbstractRowBlock.java Outdated
Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

nit: separate commit

Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

IMO it's easier to see the reason for this change if it's part of the commit that overrides this method.

Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

I would remove this change for now as it causes test failure and create an issue in Github that SPI checker shouldn't fail for such case

Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

ok, done. #12053

core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

move this before:

        Block[] fieldBlocks = new Block[numFields];
        for (int i = 0; i < numFields; i++) {
            fieldBlocks[i] = fieldBlockBuilders[i].build();
        }

and just call:

        if (!hasNonNullRow) {
            return nullRle(positionCount);
        }

Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

done

sopel39
sopel39 approved these changes Apr 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

% comments

core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

super.getRegion

core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

inlien this with nullRle

core/trino-spi/src/test/java/io/trino/spi/block/TestRowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

testBuildProducesNullRle

Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

renamed to testBuilderProducesNullRleForNullRows

@lukasz-stec lukasz-stec force-pushed the ls/012-row-has-non-null-row branch from 68759c3 to c26528e Compare April 20, 2022 09:57
lukasz-stec
lukasz-stec commented Apr 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

comments addressed

core/trino-spi/src/test/java/io/trino/spi/block/TestRowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

renamed to testBuilderProducesNullRleForNullRows

core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

done

core/trino-spi/src/main/java/io/trino/spi/block/AbstractRowBlock.java Outdated
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

IMO it's easier to see the reason for this change if it's part of the commit that overrides this method.

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented Apr 20, 2022

@lukasz-stec there are test failures

@lukasz-stec
Copy link
Copy Markdown
Member Author

lukasz-stec commented Apr 20, 2022

@lukasz-stec there are test failures

io.trino.spi.TestSpiBackwardCompatibility#testSpiSingleVersionBackwardCompatibility fails.
Looks like it doesn't support final keyword removal which is a backward compatible change as far as I can tell.
@kokosing is this intended behavior?

@kokosing
Copy link
Copy Markdown
Member

kokosing commented Apr 20, 2022

Looks like it doesn't support final keyword removal which is a backward compatible change as far as I can tell.
@kokosing is this intended behavior?

It was not intended.

sopel39
sopel39 approved these changes Apr 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

% comments

core/trino-spi/src/main/java/io/trino/spi/block/AbstractRowBlock.java Outdated
Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

I would remove this change for now as it causes test failure and create an issue in Github that SPI checker shouldn't fail for such case

core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member

@sopel39 sopel39 Apr 20, 2022

Choose a reason for hiding this comment

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

undo, unrelated change

Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

removed

@lukasz-stec lukasz-stec force-pushed the ls/012-row-has-non-null-row branch from c26528e to bd961fb Compare April 20, 2022 14:08
lukasz-stec
lukasz-stec commented Apr 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

copyPositions removed due to SPI compatibility check failing

core/trino-spi/src/main/java/io/trino/spi/block/AbstractRowBlock.java Outdated
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

ok, done. #12053

core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java Outdated
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec Apr 20, 2022

Choose a reason for hiding this comment

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

removed

@findepi findepi mentioned this pull request Apr 20, 2022
Closed
@lukasz-stec lukasz-stec force-pushed the ls/012-row-has-non-null-row branch from f192630 to 1db5a42 Compare April 22, 2022 07:35
@lukasz-stec
Copy link
Copy Markdown
Member Author

lukasz-stec commented Apr 22, 2022

since #12062 is merged i rebased on the master and re-added RowBlockBuilder:copyPositions

@lukasz-stec lukasz-stec requested a review from sopel39 April 22, 2022 13:33
@sopel39 sopel39 merged commit 0825882 into trinodb:master Apr 22, 2022
@mosabua mosabua mentioned this pull request Apr 22, 2022
Merged
@github-actions github-actions bot added this to the 379 milestone Apr 22, 2022
@lukasz-stec lukasz-stec mentioned this pull request Sep 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sopel39 sopel39 sopel39 approved these changes

Assignees

No one assigned

Labels

cla-signed

Milestone

379

Development

Successfully merging this pull request may close these issues.

3 participants

@lukasz-stec @sopel39 @kokosing