This code is complicated and somewhat poorly exercised -- most users (that we know of) have been testing with up-to-date schemas that don't require resolution.
We do expect it to work, but we should add more robust load testing (chbench-style) to increase our confidence in those assumptions.
Also, there may be cases where decoding based on a resolved schema is slower. For example: even after the current crop of Avro performance improvements lands and we're doing (mostly) allocation-free decoding, when the writer's fields are in a different order from the reader's, it may be necessary to buffer the datums somewhere and reorder them (requiring an allocation for every String/Bytes datum), before writing them to the RowPacker. Will this cause a meaningful performance hit? Should we push to improve that case? Should we advise users for whom avro decoding is a bottleneck to periodically migrate their sources to up-to-date schemas? We need concrete measurements to answer these questions.
If we have a nice way to randomly generate avro schemas and objects -- this would be a neat fit for quickcheck style tests that can validate that avro encode -> decode is identity? We could also investigate pairing that with branch coverage info to see if there are any large regions of the codebase that don't get hit?
I've played around with quickcheck a little bit but not within materialize yet. I think the hardest bits for this would be generating a random schema / random objects matching that schema
There is also a very robust Avro library in Java. Presumably it has much better test cases. We should look into snagging those.
@ruchirK random schema generation seems pretty doable actually.
@awang this is the sort of 'important but not urgent' issue that I think we should prioritize in 0.4 to catch any unknown unknown technical debt. I'll let you make the prioritization decision, but I thought I'd call it out that this is my instinct.
A note that #3553 was encountered in the wild, so this piece of technical debt did in fact cause a painful interest payment to be incurred.
I have updated the name of the task, as the scope of potential issues is not just related to schema resolution.
@umanwizard @JLDLaughlin I've assigned this to the 0.5 milestone. What are specific goals that make sense for that release?
At least the following: Porting test suite from Apache libraries, make all logical types defined in the Spec work, fix any issues reported by users.
Maybe also a fuzz testing setup, but I think that is more of a nice-to-have / stretch goal than what is listed above.
@umanwizard @JLDLaughlin what 0.5 work remains for this issue?
Python and C tests have been ported from the Apache library, but we don't currently support all logical types.
Supporting all logical types is blocked on adding UUID support to SQL, which is not hard, someone just needs to go do it. I'm not sure how high-pri it is; @benesch might have opinions.
Upon discussion with Brennan, I'm going to mark this as done. I think the stability of mz-avro is in a much better place thanks to the new test coverage from the C/Python tests. I've filed #3818 to track the long-term goal of fuzz testing, but as I mentioned there I don't think it's particularly high priority. I've added the two UUID-related issues (#3577 and #3687) and the two-time related issues (#3629, #3630) to the 0.5 milestone so they don't get lost.
Most helpful comment
At least the following: Porting test suite from Apache libraries, make all logical types defined in the Spec work, fix any issues reported by users.
Maybe also a fuzz testing setup, but I think that is more of a nice-to-have / stretch goal than what is listed above.