amogh-jahagirdar commented on code in PR #18725:
URL: https://github.com/apache/iceberg/pull/18725#discussion_r4445068046
##########
core/src/main/java/org/apache/iceberg/FilePlanner.java:
##########
@@ -192,6 +196,13 @@ private TaskContext newTaskContext(int specId) {
ResidualEvaluator.of(spec, filter, caseSensitive));
}
+ private static boolean hasLiveFiles(ManifestFile leaf) {
+ return leaf.hasAddedFiles()
+ || leaf.hasExistingFiles()
+ || leaf.modifiedFilesCount() == null
Review Comment:
I think or'ing the modifiedFilesCount == null would lead to reading more
older manifests than we would need (when we end up supporting reading them from
this path in the future). From an API perspective we'd return null for the
modifiedFilesCount for older manifests. But we can leverage the fact that we
know that for v1-v3 manifests they could've only neccessarily been written with
only added/existing/deleted to avoid reading those manifests. In other words,
we know that modified files count must be null for the older manifests, we know
that the only conditions for live files in the older manifests is if it's added
or existing > 0?
I think could either express this as:
`(leaf.modifiedFilesCount() != null && leaf.modifiedFilesCount() > 0)` note
the parens or we could leverage format version and say `(leaf.formatVersion() >
4 && leaf.modifiedFilesCount() > 0)`
I think both work, slight preference to the first one since wherever
possible we try to express things in a format version agnostic manner, but I'm
OK either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
