Add Avro row position reader #1222
Conversation
core/src/main/java/org/apache/iceberg/data/avro/DataReader.java
Outdated
Show resolved
Hide resolved
shardulm94
left a comment
shardulm94
left a comment
There was a problem hiding this comment.
Probably an unhandled edge case, but otherwise +1
| constantList.add(idToConstant.get(field.fieldId())); | ||
| } else if (field.fieldId() == MetadataColumns.ROW_POSITION.fieldId()) { | ||
| // replace the _pos field reader with a position reader | ||
| // only if the position reader is set and this is a top-level field |
There was a problem hiding this comment.
I don't understand this comment line
only if the position reader is set
reader is set where?
and this is a top-level field
I don't see where the top-level field restriction comes from
There was a problem hiding this comment.
This is out of date. I had to move the check because the position reader is set after this constructor. I'll update the comment.
| long totalRows = 0; | ||
| long nextSyncPos = in.getPos(); | ||
|
|
||
| while (nextSyncPos < start) { |
There was a problem hiding this comment.
Not sure if this valid start state, but I think there can be an edge case here
row-count|compressed-size-in-bytes|block-bytes|sync|EOF
^
|
start
In this case it will seek and read the sync and then continue to read next block row count even after EOF
So should probably be while (nextSyncPos + 16 < start)?
There was a problem hiding this comment.
The loop condition is correct with respect to start because a block starts at the beginning of a sync. You're right about the EOF behavior, though. I'll take a look at that.
There was a problem hiding this comment.
Thanks for the insightful comment! (before it went away 😃 ) I was not familiar Avro's split reading behavior.
There was a problem hiding this comment.
Sorry, I should have edited, not deleted. I missed the part about it being just before the EOF.
There was a problem hiding this comment.
The fix is to catch EOFException and return the number of rows.
rdsr
left a comment
rdsr
left a comment
There was a problem hiding this comment.
+1 . LG apart from the one EOF issue
rdblue
force-pushed
the
add-avro-pos-reader
branch
from
d107f77 to
df1cae9
Compare
| } | ||
|
|
||
| @Test | ||
| public void testPosWithEOFSplit() throws IOException { |
There was a problem hiding this comment.
@shardulm94, here's a test for the EOF case.
|
+1 LGTM |
3 participants
This adds an Avro
ValueReaderthat returns the position of a row within a file.The position reader's initial position is set from a callback that returns the starting row position of the split that is being read. The callback is passed to classes that implement a new interface,
SupportsRowPosition. This uses a callback so that if there is no position reader, the starting row position does not need to be calculated, which is expensive.Finding the row position at the start of a split requires scanning through an Avro file stream.
AvroIOnow includes a utility method that keeps track of the number of rows in each Avro block and seeks past the block content until the next block is after the given split starting point. This validates Avro sync bytes to ensure the count is accurate.Fixes #1019.