-
Notifications
You must be signed in to change notification settings - Fork 418
Make REST catalog namespace separator configurable #2826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @rambleraptor
I agree with @kevinjqliu's comments, but apart from that this looks good to me. Thanks for adding this 👍
20cea63 to
dac44f4
Compare
The REST spec currently uses %1F as the UTF-8 encoded namespace separator for multi-part namespaces.
dac44f4 to
2c616fc
Compare
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?