Add support for reading pbf #44
Conversation
There was a problem hiding this comment.
This looks like a great start!
Perhaps we should include the ProtoBuf configuration somewhere (if it is using such file - if I am wrong pls correct me)
Additionally, let's have some unit tests from the beginning (even now just trying to parse some pbf file without actually doing anything).
If there is any way you need help please let me know. Thanks for the effort!
|
|
||
| mutable struct Blob <: ProtoType | ||
| __protobuf_jl_internal_meta::ProtoMeta | ||
| __protobuf_jl_internal_values::Dict{Symbol,Any} |
There was a problem hiding this comment.
I do not know whether this is just controlled by ProtoBuf, but if we control it could we, for performance reasons, use some concrete type or a type Union perhaps?
There was a problem hiding this comment.
This is generated by ProtoBuf.jl, I was also surprised that it would not create concrete type fields instead of a Dict, I thought the whole idea behind ProtoBuf was to generate concrete specialized structs with fields. I suggest we stay with this with this PR and then try to change it to concrete fields and see what's the performance speedup in another PR. This separate PR could be a nice story to report to ProtoBuf.jl if we get a significant speedup
| if !isassigned(__meta_Blob) | ||
| __meta_Blob[] = target = ProtoMeta(Blob) | ||
| fnum = Int[2,1,3,4,5,6,7] | ||
| allflds = Pair{Symbol,Union{Type,String}}[:raw_size => Int32, :raw => Vector{UInt8}, :zlib_data => Vector{UInt8}, :lzma_data => Vector{UInt8}, :OBSOLETE_bzip2_data => Vector{UInt8}, :lz4_data => Vector{UInt8}, :zstd_data => Vector{UInt8}] |
There was a problem hiding this comment.
Again I do not use ProtoBuf, but id we control it Union{Type,Symbol} would look better to me.
There was a problem hiding this comment.
You mean Pair{Symbol,Type} ?
|
The PR is now ready for review. It should be working. The performance may still be improved, e.g. using the suggestions in your comments, but that be done in separate PRs. |
See #43