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, Test: Parsing and Writing Tests for V3 Metadata #12025

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

HonahX
Copy link
Contributor

@HonahX HonahX commented Jan 21, 2025

This used to be part of #11947 that does not include new unsafe builders for Snapshot and TableMetadata.

  • Refactor TableMetadata tests by parametrizing table format versions in all tests
  • Add example metadata files for V3 table
  • Add constants for several features/fields' min format version

@github-actions github-actions bot added the core label Jan 21, 2025
@HonahX HonahX marked this pull request as ready for review January 21, 2025 23:48
@HonahX HonahX linked an issue Jan 21, 2025 that may be closed by this pull request
@HonahX HonahX changed the title Core, Test: Tests for V3 Table Metadata Core, Test: Parsing and Writing Tests for V3 Metadata Jan 21, 2025
@Test
public void testInvalidMainBranch() throws IOException {
@ParameterizedTest
@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, we could also put it as:

Suggested change
@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
@ValueSource(ints = MIN_FORMAT_VERSION_V2)

This avoids having to rely on the assumeThat, which I feel makes it harder to interpret the test. I'm not super strong on this, though. Maybe our test-connaisseur @nastra can share his opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to do the same as mentioned in #11948 (comment) and parameterize this at the class level

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.

Parsing and Writing Tests for V3 Metadata
3 participants