-
Notifications
You must be signed in to change notification settings - Fork 330
Revert "Extensible pagination token implementation" #2123
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
Conversation
This reverts commit fb418a2.
|
Confirmed that catalog creation succeeds again after this revert. |
|
Good catch! Fixing this (unexpected) breakage makes sense to me. @eric-maynard : shall we redo the pagination PR with the above error in mind? |
|
The alternative to reverting, I guess, is to fix-forward, but I am personally not available to do this fix today 😅 |
|
+1 to fix-forward |
|
@eric-maynard do you have a reproducer as a test? |
|
The CLI command in the description reproduces the error from the description when Polaris is running (I used We cannot break main and try to fix-forward the next day. |
|
I'll work on a regtest, though -- I'm not sure why this would have passed CI if the error is real. We can hold this until then if it doesn't repro |
|
Hey, @eric-maynard. Thanks for finding this! I agree with you and I think we should revert (rather than fix-forward). Here's what I'd recommend:
What do you think? |
|
+1 @adam-christian-software, we can hold this until I have a test -- I left a comment on the original PR, but I think the issue only occurs when you also have another Scala module on the classpath (like when using the Spark plugin). I'll close this and re-open a revert or fix-forward when there's a test. |
Reverts #1938
After pulling in this change, I see errors when trying to create a catalog on main:
The server logs point to changes from this PR: