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

remove buffer in block_reader of fuse store #4230

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

youngsofun
Copy link
Member

@youngsofun youngsofun commented Feb 23, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

do not need buffer since "we execute exactly 1 seek and 1 read on them."
https://github.com/jorgecarleitao/arrow2/blob/3d528c99589e96f0539de4c07b11843fa22f23ac/examples/parquet_read_async.rs#L31

a buffer with column chunk length is allocated each time.
https://github.com/jorgecarleitao/arrow2/blob/3d528c99589e96f0539de4c07b11843fa22f23ac/src/io/parquet/read/row_group.rs#L126

thanks to @dantengsky
#4212 (comment)

Changelog

Related Issues

Test Plan

Unit Tests

Stateless Tests

@vercel
Copy link

vercel bot commented Feb 23, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/81YwDsT8wUDK4aaHFox4Q5TFbNNJ
✅ Preview: Canceled

[Deployment for 7373fc5 canceled]

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@youngsofun youngsofun mentioned this pull request Feb 23, 2022
2 tasks
@Xuanwo
Copy link
Member

Xuanwo commented Feb 23, 2022

Hi, as #4203 is going to merge, how about wait a bit so that we don't need to resolve the conflict?

@youngsofun
Copy link
Member Author

@Xuanwo ok, this is a small pr, I will wait for you to merge first

@Xuanwo
Copy link
Member

Xuanwo commented Feb 23, 2022

@Xuanwo ok, this is a small pr, I will wait for you to merge first

Thanks for the patients!

@Xuanwo
Copy link
Member

Xuanwo commented Feb 23, 2022

OpenDAL introduces an API redesign. Welcome any feedback for that!

@youngsofun youngsofun changed the title Parquet2 remove buffer in block_reader of fuse store Feb 23, 2022
@BohuTANG
Copy link
Member

Hi, as #4203 is going to merge, how about wait a bit so that we don't need to resolve the conflict?

Merged!

@@ -63,7 +63,6 @@ impl FuseTable {

let part_stream = futures::stream::iter(iter);

let read_buffer_size = ctx.get_settings().get_storage_read_buffer_size()?;
Copy link
Member

@BohuTANG BohuTANG Feb 24, 2022

Choose a reason for hiding this comment

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

Does ctx.get_storage_read_buffer_size not used anymore?

Copy link
Member Author

@youngsofun youngsofun Feb 24, 2022

Choose a reason for hiding this comment

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

still in use

@mergify mergify bot merged commit f9971bd into databendlabs:main Feb 24, 2022
@youngsofun
Copy link
Member Author

@BohuTANG not needed for the current parquet2 implementation, but this is function is also used to read our JSON metafile. need a little refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants