laskoviymishka commented on code in PR #1654:
URL: https://github.com/apache/iceberg-go/pull/1654#discussion_r3740296235
##########
go.mod:
##########
@@ -39,6 +39,7 @@ require (
github.com/docker/docker v28.5.2+incompatible
github.com/geoarrow/geoarrow-go v0.0.0-20260403143023-f54751c3e3a1
github.com/go-sql-driver/mysql v1.10.0
+ github.com/goccy/go-json v0.10.6
Review Comment:
I'd hold on this pin specifically. goccy v0.10.6 has an open data race in
its encode/decode compiler cache (goccy #566) that isn't fixed in any released
version — the fix landed on master in July but there's no v0.10.7 to pin. We
fan decode out across goroutines (errgroup in `arrow_scanner`, plus the REST
catalog from concurrent contexts), so the first-use-of-a-type window is exactly
the shape that race hits, and it stays invisible on non-`-race` production
builds.
That's on top of the marshal panic I flagged on `metadata.go`, which the
PR's own CI already hits (`TestV3PartitionStatisticsRoundTrip` SIGSEGVs inside
goccy's encoder). So we've got two independent v0.10.6 defects and nothing
stable to upgrade to.
I'd wait for a release that carries both fixes before pinning, or narrow the
swap so we aren't exposed. wdyt?
##########
table/metadata.go:
##########
@@ -31,6 +30,8 @@ import (
"strconv"
"time"
+ "github.com/goccy/go-json"
Review Comment:
This is where the CI failure comes from. `metadataV3.MarshalJSON` builds an
anonymous struct with an embedded `*Alias` and a sibling `*int64`, hands it to
`Marshal`, and goccy v0.10.6 nil-derefs inside `encoder/vm.Run` — that's
`TestV3PartitionStatisticsRoundTrip` panicking in this run, and it lines up
with the open goccy nested-embedded-omitempty panics (#576/#581/#587, #554).
`RESTDataFile`'s six pointer-omitempty fields are the same shape.
The perf goal here is REST decode CPU, and none of the marshal paths benefit
from the swap. I'd keep `Marshal`/`MarshalJSON` on `encoding/json` and only
route the decode hot paths (`rest.go`, `scan_planning`, `scan_task_decoder`,
`catalog/internal/utils`) through goccy. That sidesteps the panic entirely and
shrinks the blast radius from ~90 files to the handful that actually get the
win.
Separately, goccy normalizes invalid UTF-8 on marshal (replaces with the
replacement char) where stdlib passes bytes through — a legacy table with a
stray byte in a column name or property would round-trip differently, which is
another reason I'd leave marshaling alone. Thoughts?
##########
catalog/internal/utils.go:
##########
@@ -20,9 +20,9 @@ package internal
import (
"compress/gzip"
"context"
- "encoding/json"
"errors"
"fmt"
+ "github.com/goccy/go-json"
Review Comment:
goccy went into the stdlib import group here, so gofumpt fails — this is the
red "Audit and Verify" / macOS lint check (it names this file plus
`planfake/server.go` and `server_test.go`). Same pattern in a few more
(`codec/file_scan_task.go`, the gcs and parquet_files tests).
A blank line before the goccy import to start the third-party group fixes
it; running `golangci-lint fmt` at v2.11.4 will do it everywhere at once.
##########
catalog/rest/scan_task_decoder.go:
##########
@@ -20,12 +20,13 @@ package rest
import (
"bytes"
"encoding/hex"
- "encoding/json"
"errors"
"fmt"
"math"
"slices"
+ "github.com/goccy/go-json"
Review Comment:
While we're in the decode paths — `decodeJSONInteger` below calls
`UseNumber()`, and goccy v0.10.6 runs `ParseFloat` on every number literal even
under `UseNumber` (goccy #555/#599). Within int64 range `Int64()` still parses
exact, so I don't think it's a data-loss path, but it's untested. Could we add
a case at `MaxInt64` and one past 2^53 to lock the behavior down?
##########
udf/metadata_test.go:
##########
@@ -549,12 +550,12 @@ func TestMetadataValidation(t *testing.T) {
{
"malformed parameter",
func(t *testing.T, m map[string]any) { definition(t, m,
0)["parameters"] = []any{42} },
- nil, "cannot unmarshal number",
+ nil, "invalid character",
Review Comment:
I'd not couple the assertion to goccy's wording here. "invalid character" is
goccy's internal syntax-layer phrasing (stdlib classified this as a type error,
"cannot unmarshal number"), so it's both weaker — any syntax error now matches
— and liable to break on a goccy bump.
I'd assert on the error type instead, `errors.As` into the JSON error or
`errors.Is` on `ErrInvalidUDFMetadata`, so we're testing behavior rather than a
third-party string. Same applies to the other three rows changed in this file.
--
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]
