Skip to content

fix: clarify StringViewBuilder.UnmarshalOne expected type#897

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix-stringview-unmarshalone-type
Open

fix: clarify StringViewBuilder.UnmarshalOne expected type#897
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix-stringview-unmarshalone-type

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

Problem

StringViewBuilder.UnmarshalOne reports a json.UnmarshalTypeError with an expected type of []byte{} for unsupported token types.

Change

  • Update StringViewBuilder.UnmarshalOne to report reflect.TypeOf("") when an unsupported JSON token is encountered.
  • Add regression test TestStringViewBuilderUnmarshalOneWrongType in arrow/array/string_test.go.

Impact

  • Aligns error diagnostics with StringBuilder behavior.
  • Makes JSON import debugging clearer for users expecting string inputs.

Testing

  • Added a focused regression test that asserts the reported expected type is string for unsupported JSON token input.
  • No full test run in this PR (single-path, message-type assertion).

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 4, 2026 01:02

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The production change (reflect.TypeOf([]byte{}) -> reflect.TypeOf(string(""))) is right — a StringViewBuilder should report string. But the test doesn't compile:

arrow/array/string_test.go:914:27: cannot use dec (variable of type *encoding/json.Decoder)
  as *github.com/apache/arrow-go/v18/internal/json.Decoder value in argument to bldr.UnmarshalOne

UnmarshalOne takes arrow's vendored *internal/json.Decoder, not the stdlib one. Import github.com/apache/arrow-go/v18/internal/json (e.g. import json "github.com/apache/arrow-go/v18/internal/json") in the test instead of encoding/json. With that it should pass. Verified against the PR head with go test.

@zeroshade

Copy link
Copy Markdown
Member

@fallintoplace nudge — the production change is correct; only the test is broken: it uses the stdlib encoding/json.Decoder, but UnmarshalOne takes arrow's internal/json.Decoder, so it won't compile. Swap the import and it should pass. Happy to re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants