ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2160952515
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats>
computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(),
partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
I tried to update the code like below
```
public static CloseableIterable<PartitionStats> readPartitionStatsFile(
Schema schema, InputFile inputFile) {
Preconditions.checkArgument(schema != null, "Invalid schema: null");
Preconditions.checkArgument(inputFile != null, "Invalid input file:
null");
FileFormat fileFormat = FileFormat.fromFileName(inputFile.location());
Preconditions.checkArgument(
fileFormat != null, "Unable to determine format of file: %s",
inputFile.location());
CloseableIterable<StructLike> records =
InternalData.read(fileFormat, inputFile).project(schema).build();
CloseableIterable<PartitionStats> transformed =
CloseableIterable.transform(records,
PartitionStatsHandler::recordToPartitionStats);
return new CloseableIterable<>() {
@Override
public CloseableIterator<PartitionStats> iterator() {
return new InvalidStatsCatchingIterator(transformed.iterator());
}
@Override
public void close() throws IOException {
transformed.close();
}
};
}
private static class InvalidStatsCatchingIterator implements
CloseableIterator<PartitionStats> {
private final CloseableIterator<PartitionStats> delegate;
InvalidStatsCatchingIterator(CloseableIterator<PartitionStats> delegate)
{
this.delegate = delegate;
}
@Override
public boolean hasNext() {
return runWithCatch(delegate::hasNext);
}
@Override
public PartitionStats next() {
return runWithCatch(delegate::next);
}
@Override
public void close() throws IOException {
delegate.close();
}
private static <T> T runWithCatch(Supplier<T> action) {
try {
return action.get();
} catch (Exception exception) {
throw new InvalidStatsFileException(exception);
}
}
}
```
But still the exception will be thrown when the iterator is accessed. Not
when `readPartitionStatsFile` is called. So, I don't see much value in wrapping
this as now we are catching any exception as `InvalidStatsFileException`.
So, the current code look good to me and I don't think we need to do this.
Thoughts?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
