amoeba commented on PR #46180:
URL: https://github.com/apache/arrow/pull/46180#issuecomment-2821937014
Thanks for the review @pitrou
> Some general comments:
>
> > * Moves `ArrayFromJSON`, `ChunkedArrayFromJSON`, `DictArrayFromJSON`,
`ScalarFromJSON`, `DictScalarFromJSON` from `arrow::ipc::internal` namespace to
`arrow::util` so it's clearer they're part of the public API
> > * Renames `json_simple.{h,cc}` to `from_json.{h,cc}` which is clearer IMO
>
> Perhaps we can find something more specific than `arrow/util/from_json.h`?
>
> I'm not sure what a good name can be, but those are basically parsing
Arrow data from a single JSON object or row (instead of a JSONL stream of
objects), so the name should reflect that. For example
`arrow/from_json_object.h` (probably too wordy?).
Adding an extra word wouldn't make the files longer than other files we
already have so I think that's fine. Since "object" has a specific meaning in
the context of JSON that differs from what these helpers are ingesting, maybe
`from_json_single` or `from_json_value`? Maybe `from_json_simple` like it was
before?
> > * Includes `from_json.{h,cc}` in `api.h`
>
> I don't think we should inflate `api.h`. It's enough that it includes the
most fundamental Arrow APIs (and it may already be including too much, but of
course we can't undo that :)).
Makes sense. I reverted the api.h change and updated the PR description.
--
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
