jecsand838 commented on PR #8006: URL: https://github.com/apache/arrow-rs/pull/8006#issuecomment-3152727580
> > To move forward, would it be best to simply focus on getting the `schema.rs` changes in first (~500-600 LOC) and then follow-up with a `arrow-avro/src/reader/mod.rs` PR? > > Interesting. By "schema.rs changes" you mean adding the new `SchemaStore` and fingerprinting infrastructure? With unit tests but not yet integrated into the actual decoder? That does seem like a good idea. I don't think there's any outstanding controversy in that part of the code (just a couple of follow-on items)? Thats correct and that's my understanding as well. > > I do apologize for both the size of this PR and the quality issues. > > This is _NOT_ the kind of run of the mill logic that I would normally associate with "quality issues." For me, at least, this has been an invigorating PR to review -- not a frustrating one. Some hard core software engineering here with depth and subtlety to work through. I really appreciate that! I just thought I should have caught some of that subtlety upfront. > > Let me know what I should do and I'll get on it immediately. > > Now that we know more, the split you propose seems quite reasonable; I'm not sure it was obvious two weeks ago tho? It crossed my mind honestly. I just thought that: 1. It would be helpful to see the context of how the `SchemaStore` would be used. 2. I saw the functionality as coupled (even if loosely) and wanted to ensure there was test coverage in `mod.rs` I'll get that PR up right now and link it here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
