Skip to content
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.

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

WIP: Page extends RowBlock #23768

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

dain
Copy link
Member

@dain dain commented Oct 13, 2024

Description

This is the first step in replacing Page with RowBlock in Trino. Now that RowBlock is not null-suppressed, the difference between Page and RowBlock are minimal. The remaining main difference is the that getPositions method of a Page wraps each row in a dictionary, whereas RowBlock just wraps the entire RowBlock with a dictionary.

Release notes

( ) 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.
(X) Release notes are required, with the following suggested text:

## SPI
* Change `Page` to extend `RowBlock` . ({issue}`issuenumber`)

@dain dain requested a review from martint October 13, 2024 01:18
@cla-bot cla-bot bot added the cla-signed label Oct 13, 2024
@github-actions github-actions bot added hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector mongodb MongoDB connector labels Oct 13, 2024
@wendigo wendigo changed the title WIP: Page exetends row block WIP: Page extends RowBlock Oct 13, 2024
@wendigo
Copy link
Contributor

wendigo commented Oct 13, 2024

I didn't think of that. Actually makes a lot of sense!

@wendigo
Copy link
Contributor

wendigo commented Oct 13, 2024

@dain I was going through this PR and pushed a couple of fixups that should resolve remaining failures

@dain dain force-pushed the page-exetends-row-block branch from 0ca24c3 to 9646cdb Compare October 13, 2024 21:19
@dain
Copy link
Member Author

dain commented Oct 13, 2024

I didn't think of that. Actually makes a lot of sense!

I didn't think of it easier. @martint was complaining about the differences in a chat, and I was like "we can change this now" :D

@wendigo
Copy link
Contributor

wendigo commented Oct 14, 2024

@dain I've fixed one of my tests that was broken (#23774) and fixed some retained size calculations (last fixup).

@wendigo
Copy link
Contributor

wendigo commented Oct 14, 2024

It seems that this is now good to go (except for a failing compile commit due to a deprecation in the second commit being fixed in the third one) so we should probably add @deprecated in the third commit, rather than in the second one

@wendigo wendigo requested a review from raunaqmorarka October 14, 2024 13:03
exports io.trino.spi.statistics;
exports io.trino.spi.transaction;
exports io.trino.spi.type;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing new line

@wendigo wendigo force-pushed the page-exetends-row-block branch from f55f61b to 8f725e0 Compare October 14, 2024 13:10
@dain dain force-pushed the page-exetends-row-block branch from 8f725e0 to c7a6348 Compare October 14, 2024 22:06
@dain dain force-pushed the page-exetends-row-block branch from c7a6348 to 7ed50e2 Compare October 14, 2024 22:51
@wendigo
Copy link
Contributor

wendigo commented Oct 17, 2024

@dain can you rebase this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed delta-lake Delta Lake connector hive Hive connector hudi Hudi connector iceberg Iceberg connector mongodb MongoDB connector
Development

Successfully merging this pull request may close these issues.

2 participants