Skip to content

Conversation

@nullccxsy
Copy link
Contributor

kContent in manifest entry should be optional, not required

Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

Make sense. Thanks!

Copy link
Collaborator

@zhjwpku zhjwpku left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Should there be a corresponding test for this?

@wgtmac
Copy link
Member

wgtmac commented Aug 14, 2025

@zeroshade More test cases will be added progressively because we only have a basic reader implementation:

  • We are adding test manifest list and manifest files to verify manifest reader implementations are basically correct: test:  enhance ManifestListReaderV2Test and unified test architecture #168 and test: add test of manifestlistv1 #171. But this heavily depends on the generated files and not easy to produce various files.
  • We are on the way to implement manifest writers so we can add round trip test to verify v1/v2/v3 compatibility. This can help test all the required/optional fields like this PR.
  • We will add spark docker integration test once we can run e2e table scan.

@zeroshade zeroshade merged commit 25d4d62 into apache:main Aug 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants