wypoon commented on code in PR #11520:
URL: https://github.com/apache/iceberg/pull/11520#discussion_r1844645606
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java:
##########
@@ -541,7 +550,19 @@ private static NullabilityHolder newNullabilityHolder(int
size) {
@Override
public void setRowGroupInfo(
PageReadStore source, Map<ColumnPath, ColumnChunkMetaData> metadata,
long rowPosition) {
- this.rowStart = rowPosition;
+ setRowGroupInfo(source, metadata);
+ }
+
+ @Override
+ public void setRowGroupInfo(
+ PageReadStore source, Map<ColumnPath, ColumnChunkMetaData> metadata) {
+ this.rowStart =
+ source
+ .getRowIndexOffset()
+ .orElseThrow(
+ () ->
+ new IllegalArgumentException(
Review Comment:
I can see why you might consider `IllegalStateException`. I do think
`IllegalArgumentException` is appropriate, because the `PageReadStore` is an
argument to the method being called, and the problem is with the
`PageReadStore`. I think `IllegalStateException` is typically used to indicate
an internal inconsistency in the module.
Consider if we use Guava's `Preconditions` to check a condition here. The
condition would be that `source.getRowIndexOffset().isPresent()`. The
`checkArgument` methods throw `IllegalArgumentException` and "Ensures the truth
of an expression involving one or more parameters to the calling method." The
`checkState` methods throw `IllegalStateException` and "Ensures the truth of an
expression involving the state of the calling instance, *but not involving any
parameters to the calling method*." (my emphasis)
Of course, that just expresses the opinions of the authors of Guava. There
are others who might argue that `IllegalStateException` is appropriate here, or
neither `IllegalStateException` nor `IllegalArgumentException`. It really
doesn't matter too much. I can just throw `RuntimeException` if you do not
agree with `IllegalArgumentException`.
--
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]
