Skip to content

Conversation

TheR1sing3un
Copy link

At present, there are still many storage backends that do not support the suffix range request. I hope to still retain a parameter for users to control for themselves whether to use suffix range for data reading.

Why not directly tag the suffix range feature for each backend at the IOClient level?

Because some are in the form of protocols, such as S3, which is a standard protocol, many different object storage implementations will attempt to follow the S3 protocol. But, there are also many specific implementations that do not support the suffix range capability. We cannot directly control at the S3 level whether suffix range can be supported

Changes Made

  1. Allow users to disable the suffix range request because some storage backends do not support suffix range

Related Issues

Checklist

  • Documented in API Docs (if applicable)
  • Documented in User Guide (if applicable)
  • If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
  • Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

1. Allow users to disable the suffix range request because some storage backends do not support suffix range
@github-actions github-actions bot added the feat label Sep 11, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

Copy link

codecov bot commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.80%. Comparing base (60f447f) to head (707b7fd).
⚠️ Report is 61 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-parquet/src/metadata.rs 60.00% 4 Missing ⚠️
src/daft-io/src/lib.rs 66.66% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5188      +/-   ##
==========================================
+ Coverage   74.05%   74.80%   +0.74%     
==========================================
  Files         959      972      +13     
  Lines      124302   123714     -588     
==========================================
+ Hits        92055    92539     +484     
+ Misses      32247    31175    -1072     
Files with missing lines Coverage Δ
src/common/io-config/src/config.rs 100.00% <100.00%> (ø)
src/common/io-config/src/python.rs 50.56% <100.00%> (+1.07%) ⬆️
src/daft-connect/src/session.rs 90.83% <100.00%> (+0.15%) ⬆️
src/daft-io/src/range.rs 95.40% <ø> (ø)
src/daft-io/src/lib.rs 73.39% <66.66%> (-2.05%) ⬇️
src/daft-parquet/src/metadata.rs 90.84% <60.00%> (+9.56%) ⬆️

... and 229 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

1. fix code check
@universalmind303
Copy link
Contributor

@TheR1sing3un, Is this ready for review? It appears some of the tests are still failing. Feel free to ping me or @colin-ho once the CI is passing and it's ready for a review.

@TheR1sing3un
Copy link
Author

@TheR1sing3un, Is this ready for review? It appears some of the tests are still failing. Feel free to ping me or @colin-ho once the CI is passing and it's ready for a review.

Hi!Sorry for my late response. I think the previous failure was due to some issues with the CI itself rather than the code changes. I ran it again and everything passed. Could you please review the code for me? Thank you! :)

@TheR1sing3un
Copy link
Author

@universalmind303 @colin-ho hi, Could someone help me review it? Thank you very much

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants