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

Core: Fix CreateTableRequest so serialized keys align with spec and ensure field name tests are for serialized JSON values per spec #5135

Merged
merged 9 commits into from
Jun 28, 2022

Conversation

kbendick
Copy link
Contributor

@kbendick kbendick commented Jun 25, 2022

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 of hasOnlyKnownFields from TestOAuthTokenResponse, 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).

@github-actions github-actions bot added the core label Jun 25, 2022
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}
Copy link
Contributor Author

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
Copy link
Contributor Author

cc @rdblue as this PR can replace #5133

@kbendick kbendick force-pushed the kb-fix-req-resp-field-tests branch from 4187b53 to ba74432 Compare June 25, 2022 01:01
kbendick added a commit to kbendick/iceberg that referenced this pull request Jun 27, 2022
@rdblue
Copy link
Contributor

rdblue commented Jun 27, 2022

@kbendick, can you please update this since #5133 was merged and the other fields need to be corrected?

@kbendick kbendick force-pushed the kb-fix-req-resp-field-tests branch from 1a93c19 to 6cebed6 Compare June 28, 2022 06:51
@kbendick
Copy link
Contributor Author

@kbendick, can you please update this since #5133 was merged and the other fields need to be corrected?

Sure. Probably the easiest way to fix the other names is to rename the class fields. Will do that.

@kbendick 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 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 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
@kbendick
Copy link
Contributor Author

@kbendick, can you please update this since #5133 was merged and the other fields need to be corrected?

Sure. Probably the easiest way to fix the other names is to rename the class fields. Will do that.

This is done. If we merge this, then #5118 can be rebased and considered for merging afterwards.

@kbendick
Copy link
Contributor Author

kbendick commented Jun 28, 2022

There's a failing bloom filter related test in Java CI

org.apache.iceberg.parquet.TestBloomRowGroupFilter > testBytesEq FAILED
    java.lang.AssertionError: Should not read: cannot match a new generated binary
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertFalse(Assert.java:65)
        at org.apache.iceberg.parquet.TestBloomRowGroupFilter.testBytesEq(TestBloomRowGroupFilter.java:587)

Rerunning to see if it's just flaky.

@rdblue rdblue merged commit 71cbe8b into apache:master 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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants