Skip to content

Conversation

@aucahuasi
Copy link
Contributor

@aucahuasi aucahuasi commented Nov 28, 2022

This PR is a follow up from #14723:

  • Rename arrow::io::CacheOptions to arrow::io::CoalesceOptions (deprecate CacheOptions)
  • Implement RandomAccessFile::ReadManyAsync for S3 file system.
  • Implement RandomAccessFile::ReadManyAsync for Google Cloud Storage file system.
  • Add tests.

Related Jira ticket:
https://issues.apache.org/jira/browse/ARROW-18113

@github-actions
Copy link

std::shared_ptr<S3RetryStrategy> retry_strategy;

/// Coalesce options for requesting multiple reads at once
io::CoalesceOptions coalesceOptions = io::CoalesceOptions::Defaults();
Copy link
Member

Choose a reason for hiding this comment

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

nit: follow the naming conventions (snake_case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, btw I'll use here io::CoalesceOptions::MakeFromNetworkMetrics(5, 500) as default value (i.e. hole_size_limit = 2.5 MiB and range_size_limit = 22.5 MiB)

Comment on lines +174 to +175
int64_t hole_size_limit = 1;
int64_t range_size_limit = 100;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo, I forgot to delete those fields

Comment on lines +1135 to +1137
ctx, io::internal::CoalesceReadRanges(std::move(ranges),
coalesceOptions.hole_size_limit,
coalesceOptions.range_size_limit));
Copy link
Member

Choose a reason for hiding this comment

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

Since coalescing may merge ranges, you'll need to post-process things so that the returned futures map 1:1 with the original ranges. (May need to refactor things so you don't have to do the coalescing step twice?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks David, I'm writing a test for verifying these cases.

@github-actions
Copy link

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

@github-actions github-actions bot added the Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise label Nov 18, 2025
@github-actions github-actions bot closed this Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: C++ Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants