feat(go): support native fory row format - #3901
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Go and Java toolchains not installed on this machine; CI passed all Go checks. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
— Qwen3-235B-A22B via Qwen Code /review (v0.21.2)
| } else if strings.HasPrefix(part, "ignore=") { | ||
| switch strings.TrimPrefix(part, "ignore=") { | ||
| case "true": |
There was a problem hiding this comment.
[Suggestion] hasIgnoreTag accepts only literal "true"/"false" while the core tag parser parseFieldTag delegates to parseBoolStrict, which accepts true/1/yes/false/0/no case-insensitively with trimming. The doc comment claims this function mirrors the core parser. — Failure scenario: a struct tagged fory:"ignore=1" serializes correctly through the core fory serializer, but InferSchema returns an error for the same tag.
| } else if strings.HasPrefix(part, "ignore=") { | |
| switch strings.TrimPrefix(part, "ignore=") { | |
| case "true": | |
| } else if strings.HasPrefix(part, "ignore=") { | |
| switch strings.ToLower(strings.TrimSpace(strings.TrimPrefix(part, "ignore="))) { | |
| case "true", "1", "yes": |
— Qwen3-235B-A22B via Qwen Code /review
| end := offset + size | ||
| if offset < 0 || size < 0 || end > len(data) { |
There was a problem hiding this comment.
[Suggestion] boundedSlice's bounds check is defeated by signed integer overflow: end := offset + size wraps negative when size is near MaxInt64 (reachable via NewMapData's attacker-controlled uint64→int cast). All three guards pass on the negative end, and the function reaches a generic runtime panic instead of the intended descriptive one. Go's runtime prevents memory-safety issues, but the defensive boundary is silently bypassed. — Failure scenario: a crafted keysSize = MaxInt64 makes end = 8 + MaxInt64 overflow to negative; data[8:negative] triggers an uninformative panic.
| end := offset + size | |
| if offset < 0 || size < 0 || end > len(data) { | |
| end := offset + size | |
| if offset < 0 || size < 0 || end < offset || end > len(data) { |
— Qwen3-235B-A22B via Qwen Code /review
| dataBytes := int64(numElements) * int64(w.elemSize) | ||
| if dataBytes > maxArrayDataBytes { | ||
| return fmt.Errorf("row: array of %d elements exceeds maximum size", numElements) | ||
| } |
There was a problem hiding this comment.
[Suggestion] The int64 multiplication int64(numElements) * int64(w.elemSize) overflows for extreme numElements, wrapping dataBytes negative and bypassing the > maxArrayDataBytes guard. The trigger is physically unallocatable, but the explicit guard is provably defeated. — Failure scenario: numElements > MaxInt64/8 wraps dataBytes negative, producing an obscure runtime panic instead of the intended clean error.
| dataBytes := int64(numElements) * int64(w.elemSize) | |
| if dataBytes > maxArrayDataBytes { | |
| return fmt.Errorf("row: array of %d elements exceeds maximum size", numElements) | |
| } | |
| if int64(numElements) > maxArrayDataBytes/int64(w.elemSize) { | |
| return fmt.Errorf("row: array of %d elements exceeds maximum size", numElements) | |
| } | |
| dataBytes := int64(numElements) * int64(w.elemSize) |
— Qwen3-235B-A22B via Qwen Code /review
| elem, err := inferField(listItemName, t.Elem(), path) | ||
| if err != nil { | ||
| return Field{}, err | ||
| } | ||
| return Field{Name: name, Type: &ListType{Elem: elem}, Nullable: true}, nil |
There was a problem hiding this comment.
[Suggestion] inferField leaves the list-element Nullable flag at its zero value false for value-type elements (e.g. []int32), diverging from Java's TypeInference (which produces Nullable: true for List<Integer>) and Go's own List()/Map() factories (which always set Nullable: true). The nullable flag is encoded as bit 6 of the field header in SchemaToBytes, so schema bytes differ. The cross-language test fixture uses only pointer elements ([]*string), masking this divergence. — Failure scenario: adding a []int32 field to the xlang fixture would fail both parsed.Equal(encoder.Schema()) and bytes.Equal(reencoded, schemaBytes).
| elem, err := inferField(listItemName, t.Elem(), path) | |
| if err != nil { | |
| return Field{}, err | |
| } | |
| return Field{Name: name, Type: &ListType{Elem: elem}, Nullable: true}, nil | |
| elem, err := inferField(listItemName, t.Elem(), path) | |
| if err != nil { | |
| return Field{}, err | |
| } | |
| elem.Nullable = true | |
| return Field{Name: name, Type: &ListType{Elem: elem}, Nullable: true}, nil |
— Qwen3-235B-A22B via Qwen Code /review
| key := reflect.New(goType.Key()).Elem() | ||
| if err := keyCodec.read(keys, j, key); err != nil { | ||
| return err | ||
| } | ||
| value := reflect.New(goType.Elem()).Elem() |
There was a problem hiding this comment.
[Suggestion] newMapCodec's read loop allocates two heap objects per map entry via reflect.New(...).Elem() inside the loop. Since SetMapIndex copies the key and value, and the read functions fully overwrite their targets (per the valueCodec doc contract), both allocations can be hoisted before the loop. — Concrete cost: deserializing a map with N entries performs 2N short-lived heap allocations that immediately become garbage, adding GC pressure proportional to map size.
| key := reflect.New(goType.Key()).Elem() | |
| if err := keyCodec.read(keys, j, key); err != nil { | |
| return err | |
| } | |
| value := reflect.New(goType.Elem()).Elem() | |
| key := reflect.New(goType.Key()).Elem() | |
| value := reflect.New(goType.Elem()).Elem() | |
| for j := 0; j < n; j++ { | |
| if err := keyCodec.read(keys, j, key); err != nil { | |
| return err | |
| } |
— Qwen3-235B-A22B via Qwen Code /review
2 participants
Why?
What does this PR do?
Related issues
AI Contribution Checklist
yes/noyes, I included a completed AI Contribution Checklist in this PR description and the requiredAI Usage Disclosure.yes, my PR description includes the requiredai_reviewsummary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described inAI_POLICY.md, the Fory-guided reviewer and the independent general reviewer, on the current PR diff or current HEAD after the latest code changes.Does this PR introduce any user-facing change?
Benchmark