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

Add Block#isLoaded method #1216

Merged
merged 2 commits into from Aug 2, 2019

Conversation

@sopel39
Copy link
Member

commented Jul 29, 2019

Not only LazyBlocks can be loaded/not loaded.
Some blocks (e.g. dictionary blocks) can be
partially loaded.

@sopel39 sopel39 requested review from dain and Praveen2112 Jul 29, 2019

@cla-bot cla-bot bot added the cla-signed label Jul 29, 2019

@@ -172,17 +171,17 @@ private Page flush()
return output;
}

private static boolean containsNotLoadedLazyBlock(Page page)
private static boolean allBlocksLoaded(Page page)

This comment has been minimized.

Copy link
@Praveen2112

Praveen2112 Jul 30, 2019

Member

Can we move this logic to Page ? As other operators can use the same in the future

This comment has been minimized.

Copy link
@sopel39

sopel39 Jul 30, 2019

Author Member

This method is part of rather poor heuristics and not used anywhere else. Let's see it if becomes useful in other places.

@sopel39 sopel39 force-pushed the starburstdata:ks/is_loaded branch from a072694 to 8efd973 Jul 30, 2019

@dain

dain approved these changes Aug 1, 2019

Copy link
Member

left a comment

Some minor comments about possibly breaking this commit up. This isn't a requirement, just a suggestion.

{
// TODO: provide better heuristics there, e.g check if last produced page was materialized
for (int channel = 0; channel < page.getChannelCount(); ++channel) {
Block block = page.getBlock(channel);
if ((block instanceof LazyBlock) && !((LazyBlock) block).isLoaded()) {
return true;
if (!block.isLoaded()) {

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

I believe this is a behavior change. Consider moving the behavior change to a separate commit.


for (int i = 0; i < page.getChannelCount(); ++i) {
Block block = page.getBlock(i);
if (block instanceof LazyBlock) {
LazyBlock delegateLazyBlock = (LazyBlock) block;
if (delegateLazyBlock.isLoaded()) {

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

Looks like we lost this case, which seems fine to me. If this is a behavior change maybe move it to a separate commit, got keep this one more mechanical.

sopel39 added some commits Aug 2, 2019

Add Block#isLoaded method
Not only LazyBlocks can be loaded/not loaded.
Some blocks (e.g. dictionary blocks) can be
partially loaded.

@sopel39 sopel39 force-pushed the starburstdata:ks/is_loaded branch from 8efd973 to 8ba4a0c Aug 2, 2019

@sopel39 sopel39 merged commit de82684 into prestosql:master Aug 2, 2019

1 of 2 checks passed

Travis CI - Pull Request Build Created
Details
verification/cla-signed
Details

@sopel39 sopel39 deleted the starburstdata:ks/is_loaded branch Aug 2, 2019

@sopel39 sopel39 referenced this pull request Aug 2, 2019
2 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.