-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Core: Fix CreateTableRequest so serialized keys align with spec and ensure field name tests are for serialized JSON values per spec #5135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kbendick
commented
Jun 25, 2022
core/src/test/java/org/apache/iceberg/rest/requests/TestUpdateNamespacePropertiesRequest.java
Outdated
Show resolved
Hide resolved
kbendick
commented
Jun 25, 2022
} catch (JsonProcessingException e) { | ||
throw new RuntimeException(e); | ||
} | ||
} |
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.
This is now the version of the test that exists in the base class RequestResponseTestBase
, and thus does not need to be overridden anymore.
kbendick
force-pushed
the
kb-fix-req-resp-field-tests
branch
from
June 25, 2022 01:01
4187b53
to
ba74432
Compare
kbendick
added a commit
to kbendick/iceberg
that referenced
this pull request
Jun 27, 2022
…5135 or apache#5133) so other tests can run
rdblue
reviewed
Jun 27, 2022
core/src/test/java/org/apache/iceberg/rest/requests/TestCreateTableRequest.java
Outdated
Show resolved
Hide resolved
…rm has only known fields
…needs to be updated - need to add customer serializer for this
kbendick
force-pushed
the
kb-fix-req-resp-field-tests
branch
from
June 28, 2022 06:51
1a93c19
to
6cebed6
Compare
kbendick
changed the title
Core: Fix REST field name case strategy and ensure field name tests are for serialized versions
Core: Fix REST request with non-kebab-case names and ensure field name tests are for serialized JSON values
Jun 28, 2022
kbendick
changed the title
Core: Fix REST request with non-kebab-case names and ensure field name tests are for serialized JSON values
Core: Fix CreateTableRequest so serialized keys are kebab-case and ensure field name tests are for serialized JSON values
Jun 28, 2022
kbendick
changed the title
Core: Fix CreateTableRequest so serialized keys are kebab-case and ensure field name tests are for serialized JSON values
Core: Fix CreateTableRequest so serialized keys align with spec and ensure field name tests are for serialized JSON values per spec
Jun 28, 2022
There's a failing bloom filter related test in Java CI
Rerunning to see if it's just flaky. |
rdblue
approved these changes
Jun 28, 2022
namrathamyske
pushed a commit
to namrathamyske/iceberg
that referenced
this pull request
Jul 10, 2022
namrathamyske
pushed a commit
to namrathamyske/iceberg
that referenced
this pull request
Jul 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes serialization for CreateTableRequest and LoadTableResponse, which have multi-word fields. Those fields need to be serialized using kebab-case in Jackson rather than camelCase
Previously, we were testing whether or not each RESTCatalog request / response had the expected fields named as in the Java class. This updates to test each objects serialized JSON form to ensure it matches the kebab-case fields from the spec.
This also updated
CreateTableRequest
to use kebab-case properly and removes the overridden definition ofhasOnlyKnownFields
fromTestOAuthTokenResponse
, as that definition is now in the base class for these tests,TestRequestResponseBase
.This PR can replace #5133 entirely (or be merged in after that one and rebasing).