Skip to content
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

Revert "Core: Use encoding/decoding methods for namespaces and deprecate Splitter/Joiner" #11574

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Nov 18, 2024

Reverts #10858

I'll revert this one for now and do a proper fix for 1.8.0

@nastra nastra requested a review from bryanck November 18, 2024 07:48
@nastra nastra added this to the Iceberg 1.7.1 milestone Nov 18, 2024
@github-actions github-actions bot added the core label Nov 18, 2024
@bryanck
Copy link
Contributor

bryanck commented Nov 18, 2024

I'm not convinced we need to revert this. In my test env I have Trino 465 + 1.7 with nested namespaces enabled, connecting to an Iceberg 1.6 REST catalog. I'm seeing the same error in the REST catalog: Unhandled error: ErrorResponse(code=404, type=NoSuchNamespaceException, message=Namespace does not exist: bck%1Fdb1) whether Trino is using 1.7.0 or this branch. Perhaps we can assume that if you have nested namespaces enabled in Trino you need a REST catalog with 1.7?

@nastra
Copy link
Contributor Author

nastra commented Nov 19, 2024

@bryanck the issue is that the 1.7 client sends the query param (where the nested namespace is encoded) using %1F but the 1.6 catalog expects the non-UTF-8 encoded character \u001f. Basically Iceberg 1.6 prior to #10858 used RestUtil.encodeNamespace() / RestUtil.decodeNamespace() only for paths and used \u001f directly with a Splitter/Joiner for the query param on the client and the server, which effectively causes the issue.
#10858 was mainly introduced for #10877, so I'll revisit the approach but in the meantime it's probably the safest to just revert.

@bryanck bryanck merged commit 3badfe0 into main Nov 19, 2024
49 checks passed
@bryanck
Copy link
Contributor

bryanck commented Nov 19, 2024

I retested this and this does resolve the issue (I tested the wrong Trino build previously).

@nastra nastra deleted the revert-10858-use-encode-decode-namespace-methods branch November 19, 2024 14:54
@nastra
Copy link
Contributor Author

nastra commented Nov 19, 2024

thanks for reviewing and testing this @bryanck

bryanck pushed a commit that referenced this pull request Nov 19, 2024
@bryanck bryanck mentioned this pull request Nov 19, 2024
@flyrain flyrain mentioned this pull request Nov 19, 2024
11 tasks
@mayankvadariya
Copy link

Related issue #11539

bryanck pushed a commit that referenced this pull request Nov 20, 2024
bryanck pushed a commit that referenced this pull request Nov 20, 2024
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants