Feature/storage/avro parser #11483
Feature/storage/avro parser #11483
Conversation
seanmcc-msft
requested review from
amishra-dev,
amnguye,
JoshLove-msft,
kasobol-msft and
tg-msft
as code owners
|
@kasobol-msft , @gapra-msft can you take a look? |
| { | ||
| public static async Task<byte[]> ReadFixedBytesAsync( | ||
| Stream stream, | ||
| int length, |
There was a problem hiding this comment.
Worst case avro predicts is bytes that's as long as long can define. Would it be a problem for QQ ?
There was a problem hiding this comment.
I believe the max size of an array in .Net is max int, so we can't return more bytes than that from this method. QQ records are limited to 4 MB, so I don't think this will be an issue.
https://docs.microsoft.com/en-us/dotnet/api/system.array.length?view=netstandard-2.0
There was a problem hiding this comment.
This is internal, so I'm not worried about getting into another "we need to expose longs" problem. Especially since every stream read method in .NET uses int for the count.
There was a problem hiding this comment.
4MB sounds like safe limit.
Would returning Iterable ease "we need to expose longs" effort here? (should that ever happen).
There was a problem hiding this comment.
Maybe we could address this in the future, if this situation comes up. I'd prefer not to change this now, as it would require a significant refactor of the parser (which is currently working with Quick Query and Change Feed).
There was a problem hiding this comment.
Can we at least leave a comment expressing this, so if anyone picks it up and enhances it, they can keep that in mind?
There was a problem hiding this comment.
I added a comment mentioning this.
| return bytes[0]; | ||
| } | ||
|
|
||
| // Stolen because the linked references in the Avro spec were subpar... |
There was a problem hiding this comment.
Might want to revisit that comment. End of day it's legit under their license.
There was a problem hiding this comment.
Removed the comment.
| // Validate codec | ||
| _metadata.TryGetValue(AvroConstants.CodecKey, out string codec); | ||
| if (codec == AvroConstants.DeflateCodec) | ||
| { | ||
| throw new ArgumentException("Deflate codec is not supported"); | ||
| } |
There was a problem hiding this comment.
I'd rather inverse this validation as there are more codecs available.
See https://github.com/apache/avro/blob/6b0d470a79b4c4e10d9890183d6c913608a2a225/lang/py3/avro/datafile.py#L94-L99
|
|
||
| namespace Azure.Storage.Internal.Avro | ||
| { | ||
| internal class Record |
There was a problem hiding this comment.
Deleted, it was an artifact of a previous implementation attempt.
| { | ||
| List<TestCase> testCases = new List<TestCase> | ||
| { | ||
| new TestCase("test_null_0.avro", o => Assert.IsNull(o)), // null |
|
|
||
| namespace Azure.Storage.Internal.Avro.Tests | ||
| { | ||
| public class AvroReaderTests |
There was a problem hiding this comment.
I'd love to see a test case where we're skipping some records, so that we leverage option to pass two streams into it.
| CancellationToken cancellationToken) | ||
| { | ||
| byte b = await ReadByteAsync(stream, async, cancellationToken).ConfigureAwait(false); | ||
| return b != 0; |
There was a problem hiding this comment.
Can we add explicit checking here - if 0 return false, if 1 return true, otherwise throw. I find it weird that the original Apache code didnt do this check
| Stream stream, | ||
| bool async, | ||
| CancellationToken cancellationToken) => | ||
| (int)(await ReadLongAsync(stream, async, cancellationToken).ConfigureAwait(false)); |
There was a problem hiding this comment.
Do we want to do any checking here (make sure the num is int sized) or can we just let the cast fail?
There was a problem hiding this comment.
I think its fine to let the cast fail.
| bool async, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| int size = await ReadIntAsync(stream, async, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
can you either leave a comment saying this should be long as per spec, use int since array length is int or just make this read a long and convert that to int explicitly?
| bool async, | ||
| CancellationToken cancellationToken); | ||
|
|
||
| public static AvroType FromSchema(JsonElement schema) |
There was a problem hiding this comment.
this may be neater if you split it further into FromStringSchema, FromArraySchema, FromObjectSchema
There was a problem hiding this comment.
Do you think it would be helpful to add constants for each type?
There was a problem hiding this comment.
Added constants for each type.
There was a problem hiding this comment.
Split into FromStringSchema, FromArraySchema, FromObjectSchema.
| _initalized = false; | ||
| BlockOffset = currentBlockOffset; | ||
| ObjectIndex = indexWithinCurrentBlock; | ||
| _initalized = false; |
| _initalized = false; | ||
| } | ||
|
|
||
| private async Task Initalize(bool async, CancellationToken cancellationToken = default) |
| /// <summary> | ||
| /// If this Avro Reader has been initalized. | ||
| /// </summary> | ||
| private bool _initalized; |
| } | ||
| } | ||
|
|
||
| public bool HasNext() => !_initalized || _itemsRemainingInBlock > 0; |
There was a problem hiding this comment.
Is it better we call Initalize() here in case the avro file contains 0 records?
No description provided.