Core: fix NPE in manifests table for contains_nan column, update spec

[yyanyy]

• fix NPE when reading metadata for manifests. #2405 which is caused by NPE brought in by Core: add contains_nan to field_summary #1872. When I changed containsNaN field within partition field summary from primitive to boxed boolean within Core: add contains_nan to field_summary #1872, I forgot to change other places that refers to the primitive boolean, causing NPE when reading metadata for manifests. #2405 and Core: Fix NPE caused by Unboxing a Null in ManifestFileUtil (#2492) #2495 (thanks @RussellSpitzer again for the fix!)
• in NPE when reading metadata for manifests. #2405 we also investigated the other approach that can prevent this issue - to make containsNaN a primitive to avoid potential NPE. This may have the following implication:
  • we generally want to assign true to this column so that we don't falsely skip it; but in this case non-floating-point columns will also have containsNaN=true in memory which doesn't make sense. This field isn't used anywhere for non-floating-point columns so it won't have correctness concern. However when displaying manifests table, this default value will be used, and for any files written before containsNaN getting populated in field summary, containsNaN will be displayed as true which may not be correct.
  • there is a case within manifest evaluator that will produce less accurate result if we don't differentiate true between actually having NaN and default value
  • public facing interface may need to change but it's minor since it is not part of any release

In addition to fixing the NPE problem, this PR attempts to follow the current approach (of nullable containsNaN) to update spec with the current state. We can continue the discussion of how to handle it here and I'll make sure to reflect it in the actual spec before merge.

[RussellSpitzer]

I think this is the right approach for this fix, one of the Spec changes seems unrelated to this issues (field field) but I have no problem with fixing typos in the same pr.

[chenjunjiedada]

Should we add a unit test to read a v1 manifest file?

[RussellSpitzer]

Should we add a unit test to read a v1 manifest file?

I would also like to see this, but I think we probably need a dedicated backwards compatibility suite somewhere

[jackye1995]

we probably need a dedicated backwards compatibility suite somewhere

+1. As we are thinking about v3 now, I am trying to implement this in a way that can be flexible for future spec versions, will send a PR about this soon.

[yyanyy]

Thank you for the quick review and feedback everyone!

Regarding backward compatibility tests for reading manifest files, I think there are a few things worth mentioning:

1. Iceberg does have dedicated tests on verifying backwards compatibility for both manifest file and manifest list.
2. even with these backward compatible tests, this issue can still occur (I did update the test in the original PR). The reason is I think these are two separate issues:
   • My understanding of the problem that's fixed by this specific PR is not related to generally reading of the manifest files/manifest list itself, but rather the display of a metadata table manifests table which is a separate logic.
   • The problem before the fix was that the schema we specified for this manifest table is wrong comparing to the correct specification we have for the actual requirement of manifest files here, and this mismatch caused the NPE problem since we require an originally nullable field to be not null.
   • If we reused the schema construct we defined from ManifestFile (which is the source of truth in persisting manifest list file) in ManifestsTable we can avoid this problem completely, but currently ManifestFile doesn't want to expose them out, and has to specifically define the ID to ensure there's no duplication. I think we can still reuse them if we want, but that requires some code changes, and the current approach of duplicating definition of the same concepts in multiple places seems like a more widely adopted approach across Iceberg project.
   • I think the ultimate way of preventing this specific problem is to create tests around metadata tables, and for this specific problem, we create v1 and v2 normal table, write data to them, and then query the manifest table and ensure output meets expected. However I think we don't have any test around metadata tables today.

I think the follow up item after this PR should be to create an issue to add metadata tables tests to make sure they don't break when reading different versions of tables. Any comment/feedback/suggestions?

[chenjunjiedada]

Agree to follow up with a separated PR.

[RussellSpitzer]

I was wondering about the other NPE in #2495, i was hoping that would have been caught by some backwards compatibility tests. I also have no problem with doing more testing and such in another pr

[yyanyy]

I was wondering about the other NPE in #2495, i was hoping that would have been caught by some backwards compatibility tests. I also have no problem with doing more testing and such in another pr

I think the hard part is that this specific change (and some other spec changes) is adding a field regardless of table versions, and to ensure backward compatibility which means to test on a table before this logic is introduced, we need to have a fine grain control of creating these metadata files to mimic the old behavior when creating the table, so that we can drop things introduced in the new logic. And this basically means to create the table from scratch.

• Tests like TestManifestListVersions covers part of that but it only test the basic interaction, no the end to end table operation exists in such tests that could catch problem in ManifestFileUtil which is used during table commit.
• Some tests extending TableTestBase in core package try to test backward compatibility by running tests with table v1 and v2 format, but again they won't capture logic introduced in all versions of the spec.

I think a way of achieving it could be to add resource files (including data file and all metadata files all the way to the json file) with all optional/later introduced fields missing, and these resource files could be readily readable as an existing table (instead of creating tables via code as those tests currently do). And then we let tests that extend TableTestBase to interact with this table. I've created an issue to track this: #2542

---

I'm merging in this change to unblock bug fixes, but please feel free to continue the discussion here/in #2542 or ping me directly on slack. Thank you again @chenjunjiedada @RussellSpitzer @jackye1995 for the review and quick response! And thanks Jack for creating the issue!