alexandre-normand commented on PR #1654: URL: https://github.com/apache/iceberg-go/pull/1654#issuecomment-5256714421
> Thanks for this — the goal is a fair one and the swap is mechanically consistent across the tree, which I appreciate. > > I'd hold this before merging though, and the reason is the same thing three of us landed on independently: goccy v0.10.6 specifically isn't a safe pin, and the PR's own CI is red on it right now. > > The blocking one is the encoder. `metadataV3.MarshalJSON` hands an anonymous struct with an embedded `*Alias` and a sibling `*int64` to goccy, and v0.10.6 nil-derefs inside its encoder — that's `TestV3PartitionStatisticsRoundTrip` panicking with a SIGSEGV in this run, and it matches a cluster of open goccy panics on nested embedded omitempty structs. `RESTDataFile` has the same shape. On top of that goccy has an open data race in its compiler cache (their #566) whose fix isn't in any released version yet, and we decode from goroutines (errgroup fan-out, REST catalog under concurrency), so that race window is one we'd actually hit — silently, on non-`-race` builds. > > The thing that makes me most comfortable holding is that there's a clean, narrower version of this change. The perf target is REST decode CPU, and none of the marshal paths benefit — so keeping `Marshal`/`MarshalJSON` on `encoding/json` and routing only the decode hot paths through goccy sidesteps the panic entirely, avoids the UTF-8 normalization divergence on write, and drops the blast radius from ~90 files (CLI, SQL catalog, Puffin, every test) down to the few that get the win. > > A few things I'd want settled before merge: > > * pin a goccy release that carries both the encoder-panic and the #566 race fixes (or hold until one exists) > * scope the swap to the REST decode paths; leave marshaling and the non-hot-path packages on stdlib > * green up the gofumpt lint check (blank line before the goccy import in the six affected files) > * decouple the `udf/metadata_test.go` assertions from goccy's error strings > * add a benchmark in-repo (there's precedent with the `*_bench_test.go` files) so the perf win is reproducible, plus a large-int64 decode case > > Once those are addressed I'm happy to take another pass and approve. @laskoviymishka I think I addressed all the things you had on your list except this one: > * pin a goccy release that carries both the encoder-panic and the #566 race fixes (or hold until one exists) I could update to use the latest commit and not block on a new tagged version. I think it's likely that we might just want to go with `encoding/json/v2` instead of waiting for a tagged release but I would like to get better performance without having to wait for go 1.27. I wouldn't be offended if you just declined the PR and I could just use this change on our fork until go 1.27 lands and, hopefully, leads to better performance equal or better than goccy/go-json. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
