szehon-ho commented on code in PR #5632:
URL: https://github.com/apache/iceberg/pull/5632#discussion_r979320542
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -126,15 +138,12 @@ protected ManifestReader(
specId = Integer.parseInt(specProperty);
}
- if (specsById != null) {
- this.spec = specsById.get(specId);
+ if (specsById != null && specsById.containsKey(specId)) {
Review Comment:
I see it now. Yea I'm not entirely sure we need all these checks if
specsById contains specId, now unfortunately it gets really complicated. But I
dont have the background to say we don't need it.
I do notice here, there is a bit inefficiency in case specsById == null.
Here you will read specId from file. It's unnecessarily , as there is no map
to look it up, instead it should go directly to read the spec from file. In
previous code it would go straight to read spec from file.
Your code:
```
if (specsById != null && specsById.contains(specId)) {
return specsById.get(specId)
else
specId = readSpecId()
if (specsById != null && specsById.contains(specId)
return specById.get(specId)
```
More optimal code:
```
if (specsById == null)
return readSpec
if (specsById.contains(specId)
return specsById.get(specId)
else
specId = readSpecId()
if (specsById.contains(specId)
return specsById.get(specId)
else
return readSpec()
```
I hope I understand it right.
--
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]
