Make REST catalog namespace separator configurable#2826
Make REST catalog namespace separator configurable#2826kevinjqliu merged 7 commits intoapache:mainfrom
Conversation
|
@geruh thanks so much for your review! I've addressed your comments |
|
i was reviewing this and got distracted, a couple of times 😆 |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! Thanks for adding this feature
Could you add an integration test against the iceberg-rest-fixture? Currently http://localhost:8181/v1/config returns
"overrides": {
"namespace-separator": "%2E"
},
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Nice one @rambleraptor
I agree with @kevinjqliu's comments, but apart from that this looks good to me. Thanks for adding this 👍
rambleraptor
force-pushed
the
namespace_configure_rest
branch
2 times, most recently
from
dac44f4 to
2c616fc
Compare
|
@kevinjqliu @Fokko ready to merge! |
The REST spec currently uses %1F as the UTF-8 encoded namespace separator for multi-part namespaces.
rambleraptor
force-pushed
the
namespace_configure_rest
branch
from
56e93b9 to
14742c3
Compare
|
|
||
| separator_from_properties = self.properties.get(NAMESPACE_SEPARATOR_PROPERTY, DEFAULT_NAMESPACE_SEPARATOR) | ||
| if not separator_from_properties: | ||
| raise ValueError("Namespace separator cannot be an empty string") |
There was a problem hiding this comment.
Java's RESTUtil.decodeNamespace detects the legacy %1F separator in existing namespaces even when a new separator is configured. Do we need similar handling?
There was a problem hiding this comment.
this threw me off, i was looking for where decode is used.
i think decode is only applicable on the server side, https://github.com/apache/iceberg/blob/1a4f423397882b8f7d44c4eb027faf9e5e5fb7bb/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java#L717-L718
we probably should add it for completeness, but i dont think its a blocker here
|
|
||
| separator_from_properties = self.properties.get(NAMESPACE_SEPARATOR_PROPERTY, DEFAULT_NAMESPACE_SEPARATOR) | ||
| if not separator_from_properties: | ||
| raise ValueError("Namespace separator cannot be an empty string") |
There was a problem hiding this comment.
this threw me off, i was looking for where decode is used.
i think decode is only applicable on the server side, https://github.com/apache/iceberg/blob/1a4f423397882b8f7d44c4eb027faf9e5e5fb7bb/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java#L717-L718
we probably should add it for completeness, but i dont think its a blocker here
|
Thanks for the PR @rambleraptor and thanks for the review @Fokko @geruh |
4 participants
Closes #1183
This adds a configurable namespace separator in the REST Catalog.
Rationale for this change
Certain implementations expect a different namespace separator.
Are these changes tested?
Tests included.
Are there any user-facing changes?