CORE - Load Table Response Test #5118
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
force-pushed
the
kb-load-table-response-tests
branch
3 times, most recently
from
02e3cc4
to
2fa317f
Compare
kbendick
commented
Jun 23, 2022
There was a problem hiding this comment.
Left some notes on what can be done to reduce the scope of things made public or my thoughts on each.
We can also use shims in test code etc to access some values (like lastAssignedFieldId(), but it's just for TestLoadTableResult so that seems like overkill.
cc @rdblue
core/src/test/java/org/apache/iceberg/LocalTableOperations.java
Outdated
Show resolved
Hide resolved
|
Not sure why the Flink CI suite was cancelled after many hours, so I’m going to close and reopen to have CI run again. |
|
Since we’re only testing We’d still be building the |
|
On reflecting on this a bit, I think writing the JSON out into test files and then parsing it is probably the ideal way to go, given we’re not testing |
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 23, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
kbendick
force-pushed
the
kb-load-table-response-tests
branch
from
2fa317f
to
9ac577e
Compare
kbendick
force-pushed
the
kb-load-table-response-tests
branch
from
87cc9ab
to
a0d0f1d
Compare
kbendick
commented
Jun 25, 2022
core/src/main/java/org/apache/iceberg/rest/RESTObjectMapper.java
Outdated
Show resolved
Hide resolved
kbendick
force-pushed
the
kb-load-table-response-tests
branch
from
3597242
to
7e3e6b9
Compare
kbendick
commented
Jun 27, 2022
core/src/test/java/org/apache/iceberg/rest/requests/TestCreateTableRequest.java
Outdated
Show resolved
Hide resolved
kbendick
commented
Jun 27, 2022
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
Outdated
Show resolved
Hide resolved
kbendick
commented
Jun 27, 2022
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
Outdated
Show resolved
Hide resolved
kbendick
commented
Jun 27, 2022
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
Outdated
Show resolved
Hide resolved
kbendick
commented
Jun 27, 2022
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 28, 2022
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 28, 2022
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 28, 2022
core/src/test/java/org/apache/iceberg/LocalTableOperations.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 28, 2022
rdblue
reviewed
Jun 28, 2022
| The table metadata JSON is returned in the `metadata` field. The corresponding file location of table metadata should be returned in the `metadata-location` field, unless it hasn't been determined yet - such as when a transaction begins to stage a table for creation but has not commit. | ||
|
|
||
|
|
||
| Clients can check whether metadata has changed by comparing metadata locations after the table has been created. |
There was a problem hiding this comment.
Let's leave this as part of the previous paragraph.
There was a problem hiding this comment.
Updated what was requested to be part of the previous paragraph.
rdblue
reviewed
Jun 28, 2022
rdblue
reviewed
Jun 28, 2022
rdblue
reviewed
Jun 28, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 28, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
kbendick
force-pushed
the
kb-load-table-response-tests
branch
2 times, most recently
from
5bc8ef9
to
3213221
Compare
kbendick
commented
Jun 28, 2022
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
Outdated
Show resolved
Hide resolved
kbendick
commented
Jun 28, 2022
core/src/test/java/org/apache/iceberg/rest/RequestResponseTestBase.java
Outdated
Show resolved
Hide resolved
kbendick
force-pushed
the
kb-load-table-response-tests
branch
2 times, most recently
from
7ac948f
to
a5f2bb9
Compare
…o show metadata-location is not required * Add TestLoadTableResponse * Update OpenAPI spec for REST catalog to show that LoadTableResult's `metadata-location` is not required, as LoadTableResponse can be sent for a request that open's a transaction to create a table, in which case there is no metadata location yet.
rdblue
reviewed
Jun 28, 2022
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 28, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Jun 28, 2022
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java
Outdated
Show resolved
Hide resolved
rdblue
approved these changes
Jun 29, 2022
|
Thanks, @kbendick |
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 PR adds serde tests for
LoadTableResponse.It also removes
metadata-locationas a required field forLoadTableResponse, asLoadTableResponseis returned in response to aCreateTableRequestfor staging a table for creation without committing it during a transaction. There is not yet necessarily a metadata location as the table has not been fully created.To avoid making a number of things public, I'm reading in existing
TableMetadataJSON files and testingLoadTableResponsewith those (asLoadTableResponseis what's being tested, notTableMetadata).