Skip to content

Comments

feat(bindings/go): implement io.Seeker#6151

Merged
Xuanwo merged 2 commits intomainfrom
bindings-go-seeker
May 8, 2025
Merged

feat(bindings/go): implement io.Seeker#6151
Xuanwo merged 2 commits intomainfrom
bindings-go-seeker

Conversation

@yuchanns
Copy link
Member

@yuchanns yuchanns commented May 6, 2025 •
edited
Loading

Follow #6119.

Part of #4892.

This PR aims to implement the offset-based read operation requested in #5326 by supporting the io.Seeker interface.

FYI: https://pkg.go.dev/io#Seeker

In a previous closed PR, I attempted to implement io.ReaderAt without a proper design. Now, we can directly use opendal_reader_seek to support io.Seeker!

The difference between io.Seeker and io.ReaderAt is that io.ReaderAt allows for parallel offset-based reading.

Clients of ReadAt can execute parallel ReadAt calls on the same input source.

From https://pkg.go.dev/io#ReaderAt

NOTICE: I made some changes to the signature of the C seek function.

@yuchanns yuchanns marked this pull request as ready for review May 6, 2025 15:27
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. releases-note/feat The PR implements a new feature or has a title that begins with "feat" labels May 6, 2025
@yuchanns yuchanns marked this pull request as draft May 6, 2025 15:31
@yuchanns
Copy link
Member Author

yuchanns commented May 6, 2025 •
edited
Loading

2025-05-06T15:36:32.1237126Z D:/a/opendal/opendal/bindings/go/tests/behavior_tests/opendal_test.go:91
2025-05-06T15:36:32.1237873Z Error: Not equal:
2025-05-06T15:36:32.1238462Z expected: 456710
2025-05-06T15:36:32.1239603Z actual : 2054536108672
2025-05-06T15:36:32.1240513Z Test: TestBehavior/ReaderSee

It looks like that seek operation behave differently between *nix and Windows. Maybe it is something about alignment or overflow. I'm not familiar with that.

cc @Xuanwo Any idea?

@Xuanwo
Copy link
Member

Xuanwo commented May 7, 2025

It looks like that seek operation behave differently between *nix and Windows. Maybe it is something about alignment or overflow. I'm not familiar with that.

As far as I know, *nix systems shouldn't behave differently when it comes to seeking. I also haven't found anything wrong with our implementation. Could this issue be more deeply rooted, perhaps within purego itself?

@yuchanns yuchanns force-pushed the bindings-go-seeker branch from fb3cfb3 to c5291e4 Compare May 8, 2025 02:23
@yuchanns
Copy link
Member Author

yuchanns commented May 8, 2025 •
edited
Loading

Well, I see now. It's my bad to have treated the struct as a pointer.

So happy that the behavior test is actually ensuring multiplatform support.

@yuchanns yuchanns marked this pull request as ready for review May 8, 2025 02:32
@yuchanns
Copy link
Member Author

yuchanns commented May 8, 2025 •
edited
Loading

It is ready now.

PS: I added the breaking-changes label as I change the C-binding signature of reader_seek

@yuchanns yuchanns mentioned this pull request Apr 15, 2025
Closed
26 tasks
Xuanwo
Xuanwo approved these changes May 8, 2025
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @yuchanns for adding this support!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 8, 2025
@Xuanwo Xuanwo merged commit d2d97ac into main May 8, 2025
75 checks passed
@Xuanwo Xuanwo deleted the bindings-go-seeker branch May 8, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

@xyjixyjixyji xyjixyjixyji Awaiting requested review from xyjixyjixyji xyjixyjixyji is a code owner

Assignees

No one assigned

Labels

breaking-changes lgtm This PR has been approved by a maintainer releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yuchanns @Xuanwo