Generally the "I'm going to lump all my complaints into one big bug" is a good way to get them ignored.
I'll skip "the design is wrong and it should change because I don't like it" and cite "it's used everywhere with lots of implementations so you can't change it in an incompatible way". I'll skip obvious typos and suggest you catch more flies with honey than vinegar, and you can work a git repository and make a pull request for those and they'd be fixed fast. (Or read 2 or 3 of the 8 implementations if the phrasing is confusing to you.) I'll make some suggestions on the technical details So, specifically: * [Opinion] [Line 737]. "The 16-byte, randomly-generated sync marker for this file". I don't like this point. It implies that container files are usually not equal. Thus it is not possible to compare them bitwise to determine equality. So, in my Avro implementation I write null bytes instead of this marker (yes, this possibly means that my implementation is non-conforming) * [Opinion] [Line 717]. There is no any marker for end of container file. Thus there is no way to determine whether all data was written If you use the sync marker, you don't need an end of container marker. (Flush the sync and container block map with new data after the new block is written, if you have the metadata and block list you know that much is complete and written for you to read, if you read the metadata and your sync byte marker is wrong, go re-read/continue read.) * [Very theoretical bug, possible even security-related] [Line 435]. Since you have a test case that proves it doesn't crash systems, it's sort of not a bug right? You could at the test case to the test suite. * [Bug] [Line 572]. "Currently for C/C++ implementations, the positions are practically an int, but theoretically a long". Wat? So, other implementations use int (as per spec), but C++ uses long, right? So, go fix C++ implementation to match spec and other implementations This is not a bug, but an acknowledgement that the C/C++ offsets are internally implemented via pointer math to be efficient but if you try to read in enough data that a long offset makes sense, you will be sad/run out of memory. That the internal implementation for C/C++ supports the minimum required by the specification. * [Bug] [Line 385]. "For example, int and long are always serialized the same way". What this means? You probably mean that *same* int and long (i. e. int and long, which are numerically identical) serialized the same way. That rewrite is wrong. Your wording would allow serialization to be altered by value (e.g. it would be allowable to use big endian storage for odd numbers and little endian for even as each same int and long would be serialized the same way.) * [Opinion] [Line 596]. "if its type is null, then it is encoded as a JSON null". There is no reasons to special-case nulls. This is additional requirement, which adds complexity to implementations without any reasons You are making assumptions about implementation encoding of null. A C developer would say writing 0x00 to the file that you will read back later is fine for null or false or 0. * [Bug] [Line 417]. - can't take action, the request breaks compatibility. * [Bug] [Line 292]. "The null namespace may not be used in a dot-separated sequence of names". You defined previously null namespace as a empty string instead of *whole* namespace. I. e. null namespace is lack of namespace (i. e. lack of whole dot-separated sequence). So there is no sense in speaking on using null namespace in dot-separated sequence. You probably mean that one should not use empty string in dot-separated sequence That doesn't read as a bug at all to me, you have introduce a "whole namespace" and only then does it not make sense a.b.c - a is the top level namespace, b is a namespace in a, c is a namespace in b. Probably easier to read up on C++ namespaces and nesting and full specification to see how your introduction of "whole namespace" where it doesn't exist is what is causing your confusion. I look forward to more bug reports and pull requests to iron out typos and standardize existing practice (you might want to read through other implementations to make sure your suggestions are useful to the community at large.) On Sat, Feb 12, 2022 at 5:22 PM Askar Safin <safinas...@mail.ru> wrote: > > Hi. I'm writing my own Avro implementation in Rust for personal use. During > this work I found a lot of issues in Avro > spec (the list follows). > > I send this mail not only to user and dev mailing lists of Avro, but also to > Apache community list, Kafka list and 3 > semi-randomly chosen Materialize employees. Because I want to draw attention > to this problems. I hope this wider > community helps Avro fix their issues and possible will give necessary > resources. > > As well as I understand Avro is used in Kafka. And Kafka, according to their > site, is used in "80% of all Fortune 100 > companies". So Avro is critical piece of infrastructure of humanity. It > should be absolutely perfect (and so I list > even very small bugs). But it is not perfect. > > Some of items in this list are (small and big) bugs, some are typos, some are > my objections to the design. Some are > fixable while keeping compatibility, some are not. I don't want to spend my > time to report them as separate bugs, but > you can try to convince me to do so. > > I created this list simply by reading the spec from end to end (I skipped > sections on RPC and logical types). And I > even didn't look at implementations! > > I write this is hope to help Avro. > > I think big audit of spec and its implementations should be done. > > All line numbers apply to spec.xml from tag release-1.11.0 (i. e. > https://github.com/apache/avro/blob/release-1.11.0/doc/src/content/xdocs/spec.xml > ). As well as I understand this tag > corresponds to currently published version at > https://avro.apache.org/docs/current/spec.html . > > So, here we go. > > * [Opinion] [No line]. In Avro one have to define named records inside each > other like so: > > { "type": "record", "name": "a", "fields": > [{"name":"b","type":{"type":"record","name":"c",...}}] } > > This is very unnatural. In popular programming languages one usually define > named record next to each other, not one > inside other. Such representation is not handy to deal programmatically. In > my implementation I have to convert this > representation to usual form "root type + list of named types" right after > reading JSON and convert back just before > writing. > > * [Opinion] [No line]. In this list you will see a lot of questions on Avro > schema (encoded as JSON). Good JSON schema > ( https://json-schema.org/ ) would resolve many of them > > * [Seems to be bug] [Line 49]. "derived type name" is vague term. In fact, in > whole spec phrase "type name" is used > very vaguely. Sometimes it means strings like "record" and sometimes it means > names of named types. I propose to define > in very beginning of the spec terms for primitive types, terms for strings > like "record" and terms for names of defined > types. Here is one possible way to do this: name strings like "record" and > "fixed" "type kinds" and never name them > type names, thus reserving term "type name" to named types only (and possibly > to primitive types). > > This issue already caused problems: look, for example, to this problems with > {"type":"record","name":"record",...}: > https://lists.apache.org/thread/0wmgyx6z69gy07lvj9ndko75752b8cn2 . > > * [Opinion] [Line 58]. There is no primitive type for unsigned 64 bit > integers. Such type is present in languages such > as C and Rust > > * [Very theoretical bug, possible even security-related] [Line 435]. "The > float is converted into a 32-bit integer > using a method equivalent to Java's floatToIntBits and then encoded in > little-endian format". If we click at provided > link, we will see that this Java function does NaN normalization. I think NaN > normalization is good thing. But I think > this quite possible spec implementers overlooked this NaN normalization > requirement. So I propose: write explicitly > directly in Avro spec that NaN are normalized. Audit all Avro > implementations: whether they actually implemented this > requirement. Create tests, which will actually test this requirement. > > Also I don't know whether bit pattern provided in that Java doc (0x7fc00000) > is quiet NaN or signaling. If it is > signaling, this is very bad. > > As well as I understand if you will configure your FPU particular way than > merely copying signaling NaN from one place > to another will abort your program. So, if your FPU is configured certain way > then feeding particular binary Avro data > to a program can crash it. I. e. this is security problem. So a reader should > be careful to check whether input data is > signaling NaN *before* storing it in floating point registers. > > I checked whether manipulating signaling NaN can actually crash a program in > default settings in Windows and Linux. And > it turned out that a program will not crash. Still I think signaling NaN > should be handled carefully. > > Write to spec that writers should normalize NaNs, that readers should reject > non-normalized NaNs and that readers > should be careful not to store incoming floating number to floating-point > variable before its sanitizing. Write that > this is security issue. > > * [Opinion] [Line 68]. "unicode character sequence". As well as I understand > Unicode character sequence means sequence > of Unicode scalar values. Note that scalar value is not same thing as code > point. Unfortunately, some people don't know > this, so please write explicitly: "this is sequence of scalar values, not > code points", to make sure implementations > will be correct > > * [Bug] [Line 71]. "Primitive types have no specified attributes". This is > lie. At line 1527 you specify logical type > based on primitive type int. Thus you specify particular meaning of attribute > "logicalType" for primitive type "int". > Be careful at your wording. The spec should be rock-solid > > * [Opinion] [Line 96]. "aliases: a JSON array of strings, providing alternate > names for this record (optional)". Is > empty array allowed? :) Are duplicate aliases allowed? :) Yes, you may say > this is nitpicking, but I don't think so. > Avro has important place in our infrastructure, so everything is important. > Think carefully whether empty list (and > duplicates) is allowed everywhere in the spec where you see some kind of > list. I think empty arrays (and duplicates) > should be disallowed in this particular case. Because the more things we > allow, the bigger is attack surface > > * [Bug] [Line 98]. "fields: a JSON array, listing fields". How many fields > allowed? Already reported by me at > https://issues.apache.org/jira/browse/AVRO-3279 > > * [Bug] [Line 235]. "Unions". How many variants in union allowed? Already > reported by me at > https://issues.apache.org/jira/browse/AVRO-3280 > > * [Bug] [Line 101]. "name: a JSON string providing the name of the field > (required), and". Word "and" usually placed > immediately before last item in sequence. The text here looks like item "doc" > was last, but then the text was edited > not carefully. This is very stupid typographic issue which shows that authors > are not careful about spec quality. Also, > the spec is not consistent on placing dots after items (this applies to whole > spec). Sometimes I see nothing in the end > of item, sometimes "." and sometimes ";" > > * [Opinion] [Line 106]. "default: A default value for..." What follows is > essentially description of JSON > representation of Avro datum (except for unions). So, you managed to put very > important part of your spec directly into > one paragraph into second level bullet point?! > > * [Opinion] [Line 112]. "Default values for union fields correspond to the > first schema in the union". This phrase is > difference between JSON encoding for Avro data and JSON encoding for default > field. And, of course, presence of this > difference is design bug > > * [Opinion] [Line 113]. "Default values for bytes and fixed fields are JSON > strings, where Unicode code points 0-255 > are mapped to unsigned 8-bit byte values 0-255". Wat? This is very unnatural > encoding. You misuse JSON string. They are > for strings, they are not for binary data. You should use array of numbers > instead. I. e. encode bytes 0x0f 0x02 as > [15, 2]. Moreover, how you will encode null bytes? "\u0000", right? C > programs have difficulties with such strings > > * [Bug] [Line 123]. Okey, so longs are encoded as JSON integers. But what if > given long is not a JSON-safe integer? As > we know from https://datatracker.ietf.org/doc/html/rfc7159 integers outside > of range [-(2**53)+1, (2**53)-1] are not > JSON-safe > > * [Bug] [Line 123]. Infinities and NaNs cannot be represented in JSON, > despite they seems to be allowed by Avro spec. > So, JSON representation of Avro data is incomplete > > * [Bug] [Line 128]. "enum". What is enum? :) This term is not yet defined by > spec. For unknown reasons you decided to > insert essentially whole description of JSON representation of Avro data into > one small paragraph even before type > system is fully described. Please use terms only after their definition > > * [Stupid bug] [Line 168]. "namespace". Namespace is not marked as optional > or required. Do you ever read your spec? > > * [Even more stupid bug] [Line 168]. "<em>namespace</em>". Word "namespace" > is marked using <em>, not <code>, and we > can see this in rendered version. This is very stupid typographic bug, which > is immediately obvious to anybody reading > this document, even to non-technical people > > * [Bug] [Line 200]. "a single attribute". As we see in provided example, > "default" is allowed, too. What is meaning of > this "default" attribute? And how its meaning differs from meaning of > "default" key in field description? (Same for > maps) > > * [Bug] [Line 238]. "declares a schema which may be either a null or string". > Lie. Schema is ["null", "string"]. > *Value* may be a null or string. Please check that you don't confuse types > (schemas) with value through whole your spec > > * [Very opinionated opinion :)] [Line 235]. I don't like your unions at all. > I'm coming from languages like Haskell and > Rust, where true sum types are supported. They are similar to your unions, > but their alternatives are named. > Alternatives are identified by their names, so there is no restriction on > duplicate types. So there is no need for very > unnatural restriction "Unions may not contain more than one schema with the > same type, except for the named types > record, fixed and enum" > > * [Absolutely stupid bug] [Line 261]. "aliases: a JSON array of strings, > providing alternate names for this enum". You > mean "fixed", right? So, you copy-pasted section on enums? Do you ever read > your spec from end to end at least one time? > > * [Bug] [Line 265]. "size: an integer, specifying the number of bytes per > value". Is zero allowed? > > * [Bug] [Line 292]. "The null namespace may not be used in a dot-separated > sequence of names". You defined previously > null namespace as a empty string instead of *whole* namespace. I. e. null > namespace is lack of namespace (i. e. lack of > whole dot-separated sequence). So there is no sense in speaking on using null > namespace in dot-separated sequence. You > probably mean that one should not use empty string in dot-separated sequence > > * [Bug] [Line 374]. "Deserializing data into a newer schema is accomplished > by specifying an additional schema, the > results of which are described in Schema Resolution". Term "additional > schema" is vague here. I would say so: > "Deserializing data into a newer schema is accomplished by using an algorithm > described in Schema Resolution" > > * [Bug] [Line 380]. "Therefore, it is possible, though not advisable, to read > Avro data with a schema that does not..." > The whole paragraph is very vague. At first reading I thought that it is > about schema resolution. After several > attempts to understand it I finally understood that the paragraph is about > reading attempts without original writer > schema available at all. I propose removing whole paragraph or rewriting it > completely > > * [Bug] [Line 385]. "For example, int and long are always serialized the same > way". What this means? You probably mean > that *same* int and long (i. e. int and long, which are numerically > identical) serialized the same way. > > * [Bug] [Line 413]. "null is written as zero bytes". The phrase is vague. Do > you mean no bytes at all? Or null bytes, > i. e. some undefined number of null bytes? (Of course, I understand that you > mean the first variant, but I still don't > like the phrase) > > * [Bug] [Line 417]. "int and long values are written..." Is canonical (i. e. > smallest) encoding of numbers required? > Already reported by me at https://issues.apache.org/jira/browse/AVRO-3307 > > I still think canonical representations should be required. The more forms of > encoding you allow the bigger is attack > surface. > > Also, it would be desirable property for binary representations be equal when > data is equal. It would be good if you > guarantee this property at least for some subset of schemas (and of course, > you should write explicitly for which > schemas the property is guaranteed). Non-canonical representations break this > property > > * [Bug] [Line 446]. "bytes of UTF-8". As well as I understand UTF-8 is > sequence of encoded scalar values (don't confuse > with code points). Unfortunately, not everybody knows this, and thus we see > WTF-8 (i. e. encoding similar to UTF-8, but > with standalone surrogates) available in places where proper UTF-8 should > reside. So everywhere where the spec says > "UTF-8" I propose to explicitly write that standalone surrogates are not > allowed and that readers should fail if they > find them (I prefer to place this sentence to introduction of the spec) > > * [Bug] [Line 572]. "Currently for C/C++ implementations, the positions are > practically an int, but theoretically a > long". Wat? So, other implementations use int (as per spec), but C++ uses > long, right? So, go fix C++ implementation to > match spec and other implementations > > * [Opinion] [Line 596]. "if its type is null, then it is encoded as a JSON > null". There is no reasons to special-case > nulls. This is additional requirement, which adds complexity to > implementations without any reasons > > * [*Real* bug] [Line 598]. "otherwise it is encoded as a JSON object..." It > seems I found a real bug. :) Consider this > schema: > > [{"type":"record","name":"map","fields":[{"name":"b","type":"int"}]},{"type":"map","values":"int"}] > > As well as I understand such schema fully allowed. Now consider this encoded > value: {"map":{"b":0}}. What is it? Map or > record named "map"? > > * [Bug] [Line 677]. "data is ordered by ascending numeric value". What about > NaNs? > > * [Bug] [Line 682]. "compared lexicographically by Unicode code point". > Replace with scalar values. UTF-8 consists of > scalar values > > * [Opinion] [Line 737]. "The 16-byte, randomly-generated sync marker for this > file". I don't like this point. It > implies that container files are usually not equal. Thus it is not possible > to compare them bitwise to determine > equality. So, in my Avro implementation I write null bytes instead of this > marker (yes, this possibly means that my > implementation is non-conforming) > > * [Opinion] [Line 717]. There is no any marker for end of container file. > Thus there is no way to determine whether all > data was written > > * [Bug] [Line 1186]. "string is promotable to bytes. bytes is promotable to > string". Wat? How these values are promoted? > > * [Bug] [Line 1153]. What implementation should do (when it does schema > resolution)? It should first check that schemas > match (and report any errors) and then read data? Or proceed straight to > reading data? This is important distinction. > For example, what happens when we attempt to read file container without data > elements using schema resolution > algorithm? (Are such container allowed, by the way?) In the first case scheme > check should be performed. In the second > such reading should always be successful. > > If you think the first case is correct, then the section should describe > algorithm for determining matching of schemas > separately from algorithm of actual reading data > > * [Bug] [Line 1308]. <<int instead of {"type":"int"}>>. You mean <<"int" > instead of {"type":"int"}>>, right? > > * [Bug] [Line 1331]. "replace any escaped characters". Any? What about > "a\"b"? It is impossible to replace :) > > ---- > > Some notes about my task. I want to implement this: > https://blogs.gnome.org/alexl/2012/08/10/rethinking-the-shell-pipeline/ . I. > e. I want to have shell utils in Linux, > which exchange some structured data. Here is how I chose format for > representing that data. > > * I want format to be binary, not textual, this rules out JSON, XML, etc > * I want format to be typed, this rules out CBOR etc > * I want format to have support for proper sum types (similar to Haskell's), > this rules out Microsoft Bond. As well as > I understand this also rules out using GVariants, proposed in above mentioned > article. And this rules out Protobuf: > Protobuf has support for sum types (they are named OneOf), but this OneOfs > are always optional (speaking in Avro > language: you always get ["null", "int", "bool"] instead of ["int", "bool"]) > * I want format to have support for recursive types, this rules out Bare ( > baremessages.org ) > > So, we have not so many formats left. Avro and possibly a few more. And I > chose Avro. And I really like it. Because: > > * It is very compact > * It has very elegant way to support schema evolution (as opposed to > Protobuf, where fields are tagged, i. e. you trade > space efficiency for schema evolution) > * It has container format with schema attached > * You don't need to write items count to container header (good for streaming) > > So, Avro is simply *best* for my task. But then I discovered its problems > (listed above). How it is happened that such > good format has so bad spec? How it is happened that *best* format for this > task happened to be so bad? What this says > about our industry? > > == > Askar Safin > http://safinaskar.com > https://sr.ht/~safinaskar > https://github.com/safinaskar
