[
https://issues.apache.org/jira/browse/AVRO-160?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12792370#action_12792370
]
Philip Zeyliger commented on AVRO-160:
--------------------------------------
Took a look at the patch. I hadn't read the old code, so made some comments on
stuff that's probably pre-existing...
I love the (relative) simplicity of the new file format.
bq. File metadata consists of:
Would it be fair to add to spec.xml, that the file metadata
can be read with the schema
{ type: array, items: record { fields: [string, bytes] } }
Ah, no, looking at DataFileStream, it's really (more fake-syntax)
{ type: map, key-type: string, value-type: bytes }
bq. in.read(magic);
Should this throw an exception if in.read(magic)
didn't return magic.length?
bq. this.vin = new BinaryDecoder(in);
I think we should use the specific API here
if we can.
bq. public synchronized byte[] getMeta(String key)
Why does this needs to be synchronized? The meta
map is created at construction, so could be marked
final, no?
bq. public synchronized D next(D reuse) throws IOException {
As you suggested in person, this API is a bit
broken for iteration, since values may well be null.
That's fair game for another schema, though.
bq. long blockCount; // # entries in block
I was surprised that blockCount was decremented, instead of incremented,
when I first ran into it. I don't care eight way, but perhaps
"# entries remaining in current block" would be more clear.
bq. /** Move to the specified synchronization point, as returned by {...@link
DataFileWriter#sync()}. */
I'm a bit lost as to what that comment means. Ah, ok. I think what might
be more helpful is: "Move the specific synchronization point,
as returned by {...@link DataFileWriter#sync())}. If pre-established
synchornization points are not available, use {...@link sync(pos)} to move
to the first synchronization point at or past pos." i.e.,
specifically discuss the distinction between seek() and sync(), since
they're similar, but quite different.
bq. if (j == sync.length) { /* position before sync */
sin.seek(sin.tell() - DataFileConstants.SYNC_SIZE);
Does this work? In the old code, next() first skipped a synchronization marker,
so this was right. But in the new code, if blockCount=0, it reads in the
number of records. So I think this should have been changed too. Of course,
I'm probably just missing something.
bq. ((SeekableBufferedInput)in).seek(position);
These casts feel icky. You could just have an extra
field in DataFileReader, no?
bq. DataFileWriter: appendTo, create
Why are these synchronized? (There are lots of methods in this
class that are synchronized... in the common case, there's
only one thread writing, so seems to be easier to force
the user to do his own.)
Would be great to have tests for trying to setMeta() when appending,
or after file has had records in it.
I didn't see any tests for the random access stuff.
> file format should be friendly to streaming
> -------------------------------------------
>
> Key: AVRO-160
> URL: https://issues.apache.org/jira/browse/AVRO-160
> Project: Avro
> Issue Type: Improvement
> Components: spec
> Reporter: Doug Cutting
> Assignee: Doug Cutting
> Attachments: AVRO-160.patch
>
>
> It should be possible to stream through an Avro data file without seeking to
> the end.
> Currently the interpretation is that schemas written to the file apply to all
> entries before them. If this were changed so that they instead apply to all
> entries that follow, and the initial schema is written at the start of the
> file, then streaming could be supported.
> Note that the only change permitted to a schema as a file is written is to,
> if it is a union, to add new branches at the end of that union. If it is not
> a union, no changes may be made. So it is still the case that the final
> schema in a file can read every entry in the file and thus may be used to
> randomly access the file.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
