Skip to content

Conversation

@sravotto
Copy link
Contributor

This commit adds ReadAt and Seek implementations to ResumingReader and its wrapper types (limitedReader, metricsReader), enabling random access to cloud-stored files.

Implementation details:

  • ReadAt creates a new reader at the specified offset using the Opener
  • Seek tracks position logically without seeking the underlying reader
  • Both methods implement ioctx.ReaderAtCtx and ioctx.SeekerCtx interfaces
  • Thread-safe: ReadAt can be called concurrently from multiple goroutines
  • Works with all cloud storage backends that support range requests

This enables efficient reading of formats like Parquet that require random access, without buffering entire files to disk.

Release note: none

Epic: CRDB-23802

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@sravotto sravotto marked this pull request as ready for review November 21, 2025 19:40
@sravotto sravotto requested a review from a team as a code owner November 21, 2025 19:40
@sravotto sravotto requested review from BramGruneir, jeffswenson and mw5h and removed request for a team November 21, 2025 19:40
@github-actions
Copy link

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

@github-actions github-actions bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label Nov 21, 2025
This commit adds ReadAt and Seek implementations to ResumingReader and
its wrapper types (limitedReader, metricsReader), enabling random access
to cloud-stored files.

Implementation details:
- ReadAt creates a new reader at the specified offset using the Opener
- Seek tracks position logically without seeking the underlying reader
- Both methods implement ioctx.ReaderAtCtx and ioctx.SeekerCtx interfaces
- Thread-safe: ReadAt can be called concurrently from multiple goroutines
- Works with all cloud storage backends that support range requests

This enables efficient reading of formats like Parquet that require
random access, without buffering entire files to disk.

Release note: none

Epic: CRDB-23802
@sravotto sravotto force-pushed the sr8_parquet_cloud_io branch from 984449b to 8a3c74f Compare November 21, 2025 20:03
@sravotto sravotto added the O-AI-Review-Real-Issue-Found AI reviewer found real issue label Nov 21, 2025
@sravotto
Copy link
Contributor Author

Claude bug detected:

BUG_DETECTED: The limitedReader.ReadAt() implementation at pkg/cloud/impl_registry.go:468-473 bypasses configured rate limiting by directly delegating to the underlying reader without tracking bytes read or applying rate limit waits.

As suggested, changed the ReadAt function.

func (l *limitedReader) ReadAt(ctx context.Context, p []byte, off int64) (n int, err error) {
    if readerAt, ok := l.r.(ioctx.ReaderAtCtx); ok {
        n, err = readerAt.ReadAt(ctx, p, off)
        if n > 0 {
            if err := l.lim.WaitN(ctx, int64(n)); err != nil {
                log.Dev.Warningf(ctx, "failed to throttle read: %+v", err)
            }
        }
        return n, err
    }
    return 0, errors.New("ReadAt not supported by underlying reader")
}

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

Labels

o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. O-AI-Review-Real-Issue-Found AI reviewer found real issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants