Skip to content

feat(io): restore the oss scheme with a round trip test and docs - #893

Merged
wgtmac merged 3 commits into
apache:mainfrom
plusplusjiajia:test-oss-scheme-roundtrip
Aug 26, 2026
Merged

feat(io): restore the oss scheme with a round trip test and docs#893
wgtmac merged 3 commits into
apache:mainfrom
plusplusjiajia:test-oss-scheme-roundtrip

Conversation

@plusplusjiajia

@plusplusjiajia plusplusjiajia commented Aug 19, 2026

Copy link
Copy Markdown
Member

Per the suggestion on #889: oss comes back with a test and documentation.

oss returns to kS3Schemes, which also restores the oss:// credential prefix now that IsS3CredentialPrefix derives from that list. A bare oss prefix stays rejected, so nothing widens beyond what main had before #889. CanonicalizeS3Scheme now derives from the same list too — an alias missing there would not
fail, it would silently stop matching its credential and fall back to the default one.

rest_arrow_file_io_test.cc and its CMake target are restored; #889 removed them, leaving REST -> ResolvingFileIO -> registry -> Arrow FileIO uncovered. The new test writes, reads back and deletes an object addressed as oss://, against ICEBERG_TEST_S3_URI — the MinIO the AWS jobs already start — and skips when
unset. It passes in 99 ms locally and fails with URI scheme 'oss' is not supported for FileIO resolution once oss leaves the scheme table. It proves routing, not credential matching: the same keys also reach the default client through the AWS chain.

file-io.md gains an S3 property table and a section on S3-compatible storage. The OSS example is verified, not transcribed: the signing region is cn-hangzhou while the endpoint carries the oss- prefix, and path-style is rejected with SecondLevelDomainForbidden: Please use virtual hosted style to access.

@wgtmac

wgtmac commented Aug 19, 2026

Copy link
Copy Markdown
Member

How about still removing oss support from #889 and then you add it back by adding test and documentation to fileio.md to instruct users how to config for oss?

@plusplusjiajia

Copy link
Copy Markdown
Member Author

How about still removing oss support from #889 and then you add it back by adding test and documentation to fileio.md to instruct users how to config for oss?

@wgtmac Works for me — I'll keep this PR and grow it into that: once #889 lands I'll rebase, restore the scheme, and add fileio.md covering how to configure it.

@wgtmac

wgtmac commented Aug 21, 2026

Copy link
Copy Markdown
Member

#889 has been merged. You may want to proceed with a rebase.

@plusplusjiajia
plusplusjiajia force-pushed the test-oss-scheme-roundtrip branch from 554a067 to 02a2d21 Compare August 24, 2026 03:18
@plusplusjiajia plusplusjiajia changed the title test(io): cover an oss:// round trip against the S3 test endpoint feat(io): restore the oss scheme with a round trip test and docs Aug 24, 2026
@plusplusjiajia
plusplusjiajia force-pushed the test-oss-scheme-roundtrip branch 3 times, most recently from 65e04c5 to e5c8b9c Compare August 24, 2026 06:51
@plusplusjiajia

Copy link
Copy Markdown
Member Author

#889 has been merged. You may want to proceed with a rebase.

@wgtmac Done — rebased and grown into the restore we discussed: oss back in kS3Schemes, the round-trip test restored, and file-io.md extended with S3 configuration and an OSS section. The test also caught a Windows regression on its first run — ParseScheme took drive letters for URI schemes — fixed minimally inlocation_util.cc

@plusplusjiajia
plusplusjiajia force-pushed the test-oss-scheme-roundtrip branch from e5c8b9c to dfb372c Compare August 24, 2026 07:04

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just took an initial review. Have you checked it against a real OSS setup to make sure it works?

Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc Outdated
Comment thread src/iceberg/util/location_util.cc Outdated
Comment thread src/iceberg/util/location_util.cc
Comment thread mkdocs/docs/file-io.md
Comment thread src/iceberg/test/rest_arrow_file_io_test.cc Outdated
Comment thread src/iceberg/test/arrow_s3_file_io_test.cc Outdated
@plusplusjiajia

Copy link
Copy Markdown
Member Author

I just took an initial review. Have you checked it against a real OSS setup to make sure it works?

@wgtmac Thanks for the review! Yes — verified against a real OSS bucket (that's where the addressing/endpoint guidance in file-io.md comes from).

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @plusplusjiajia for working on this!

@wgtmac
wgtmac merged commit 86821d7 into apache:main Aug 26, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants