Skip to content

rest: build resource paths from final config#482

Merged
wgtmac merged 3 commits into
apache:mainfrom
plusplusjiajia:fix-prefix
Jan 6, 2026
Merged

rest: build resource paths from final config#482
wgtmac merged 3 commits into
apache:mainfrom
plusplusjiajia:fix-prefix

Conversation

@plusplusjiajia

@plusplusjiajia plusplusjiajia commented Jan 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Build ResourcePaths using final URI + prefix after fetching server config: ref:https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L233

@plusplusjiajia plusplusjiajia force-pushed the fix-prefix branch 2 times, most recently from 21775d9 to f2213de Compare January 3, 2026 16:57
@plusplusjiajia plusplusjiajia reopened this Jan 3, 2026
@plusplusjiajia plusplusjiajia reopened this Jan 4, 2026
wgtmac
wgtmac reviewed Jan 4, 2026
View reviewed changes
Comment thread src/iceberg/catalog/rest/rest_catalog.cc

std::unique_ptr<RestCatalogProperties> final_config = RestCatalogProperties::FromMap(
MergeConfigs(server_config.overrides, config.configs(), server_config.defaults));
MergeConfigs(server_config.defaults, config.configs(), server_config.overrides));

@wgtmac wgtmac Jan 4, 2026

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.

Thanks for fixing this! Let's not complicate the PR. I think this change can be added without the test.

@plusplusjiajia plusplusjiajia Jan 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for fixing this! Let's not complicate the PR. I think this change can be added without the test.

Yes, i create a new pull request for it.https://github.com/apache/iceberg-cpp/pull/483/files

wgtmac
wgtmac reviewed Jan 4, 2026
View reviewed changes
Comment thread src/iceberg/catalog/rest/rest_catalog.cc
@plusplusjiajia plusplusjiajia force-pushed the fix-prefix branch 2 times, most recently from f2213de to d74a7dc Compare January 4, 2026 06:59
@plusplusjiajia plusplusjiajia changed the title rest: rebuild resource paths from final config rest: build resource paths from final config Jan 4, 2026
HeartLinked
HeartLinked reviewed Jan 5, 2026
View reviewed changes

@HeartLinked HeartLinked left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to build resource paths from the final config. In this case, the SetBaseUri will no longer be necessary. Could you help remove it?
Btw, I don't think it's worth changing to the signature of the ResourcePaths::Config.

@plusplusjiajia

plusplusjiajia commented Jan 5, 2026

Copy link
Copy Markdown
Member Author

I think it makes sense to build resource paths from the final config, and I agree to create a new ResourcePaths instance. In this case, the SetBaseUri will no longer be necessary. Could you help remove it? Btw, I don't think it's worth changing to the signature of the ResourcePaths::Config.

SetBaseUri removed. Do you mean build a ResourcePaths to get config?

@HeartLinked

HeartLinked commented Jan 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

I think it makes sense to build resource paths from the final config, and I agree to create a new ResourcePaths instance. In this case, the SetBaseUri will no longer be necessary. Could you help remove it? Btw, I don't think it's worth changing to the signature of the ResourcePaths::Config.

SetBaseUri removed. Do you mean build a ResourcePaths to get config?

Well, sorry—I might not have put it clearly. I just wanted to say that I agree with your changes to src/iceberg/catalog/rest/rest_catalog.cc. :)

wgtmac
wgtmac reviewed Jan 5, 2026
View reviewed changes
Comment thread src/iceberg/catalog/rest/resource_paths.h Outdated

/// \brief Get the /v1/config endpoint path.
Result<std::string> Config() const;
static Result<std::string> Config(const std::string& base_uri);

@wgtmac wgtmac Jan 5, 2026 •
edited
Loading

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.

It sounds good to remove SetBaseUri. However, it leads to confusion to make this a static function. I'd suggest to revert this function as you have created a new ResourcePaths based on the server config.

@plusplusjiajia plusplusjiajia Jan 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It sounds good to remove SetBaseUri. However, it leads to confusion to make this a static function. I'd suggest to revert this function as you have created a new ResourcePaths based on the server config.

it's reasonable, i've reverted.

wgtmac
wgtmac approved these changes Jan 6, 2026
View reviewed changes

@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.

Thanks for fixing this @plusplusjiajia and for the review @HeartLinked

@wgtmac wgtmac merged commit 95ec7a3 into apache:main Jan 6, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmac wgtmac wgtmac approved these changes

+1 more reviewer

@HeartLinked HeartLinked HeartLinked approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@plusplusjiajia @HeartLinked @wgtmac