Skip to content

Conversation

@fcfangcc
Copy link
Contributor

@fcfangcc fcfangcc commented Sep 25, 2025

Rationale for this change

Impl #2516

Then changed it like

catalog = load_catalog(
    "docs",
    **{
        "type": "rest",
        "uri": "http://lakekeeper:8181/catalog",
        "s3.force-virtual-addressing": True ,
        ...
    },
)

Are these changes tested?

Add test case for fsspec s3 addressing_style properties

Are there any user-facing changes?

The default behavior is not modified

S3_SIGNER_ENDPOINT_DEFAULT = "v1/aws/s3/sign"
S3_ROLE_ARN = "s3.role-arn"
S3_ROLE_SESSION_NAME = "s3.role-session-name"
S3_FORCE_VIRTUAL_ADDRESSING = "s3.force-virtual-addressing"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we already have this one, should we re-use that one? I'm not sure why it isn't being used 🤔 Seems like a faulty refactor somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see we already have this one, should we re-use that one? I'm not sure why it isn't being used 🤔 Seems like a faulty refactor somewhere

Because two different S3 dependency libraries are used, the configuration parameters for this in the two libraries are different.

@fcfangcc
Copy link
Contributor Author

@Fokko I modified and used the same parameter s3.force-virtual-addressing to changed addressing_style

In fsspec impl, s3 api dependent s3fs -> aiobotocore, this lib use s3.addressing_style to control.

But in pyarrow impl, s3 api dependent from pyarrow.fs import S3FileSystem, this lib use force-virtual-addressing to control.

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM!

config_kwargs["read_timeout"] = float(request_timeout)

if _force_virtual_addressing := properties.get(S3_FORCE_VIRTUAL_ADDRESSING):
config_kwargs["s3"] = {"addressing_style": "virtual"}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍
config_kwargs is passed to botocore.client.Config
https://s3fs.readthedocs.io/en/latest/api.html#s3fs.core.S3FileSystem

botocore.client.Config expects botocore.client.Config in s3 dict
https://botocore.amazonaws.com/v1/documentation/api/latest/reference/config.html

@kevinjqliu kevinjqliu merged commit 2624100 into apache:main Sep 26, 2025
10 checks passed
@kevinjqliu
Copy link
Contributor

Thanks for the fix @fcfangcc

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.

3 participants