GH-43624: [Go] Add JSON/UUID extension types, extend arrow -> parquet logical type mapping #43679

joellubi · 2024-08-13T21:10:11Z

Rationale for this change

Missing JSON extension type implementation.
Current precedent in C++ (and thereby PyArrow) is that canonical extension types do not require manual registration.
Issues like [Go] pqarrow - Provide a way to support Parquet Logical types that do not have or need Arrow equivalent types #43640 and [Go] pqarrow - Support for parquet Interval type #43624 suggest that we need to expose ways of configuring parquet types written from arrow records, but casting the underlying data presents challenges for a generalized approach.

What changes are included in this PR?

Move UUIDType from internal to arrow/extensions
Implement JSON canonical extension type
Automatically register all canonical extension types at initialization
- remove register/unregister from various locations these extension types are used
Add new CustomParquetType interface so extension types can specify their target LogicalType in Parquet
Refactor parquet fieldToNode to split up PrimitiveNode type mapping for leaves from GroupNode composition
Simplify parquet LogicalType to use only value receivers

Are these changes tested?

Yes

Are there any user-facing changes?

UUID and JSON extension types are available to end users.
Canonical extension types will automatically be recognized in IPC without registration.
Users with their own extension type implementations may use the CustomParquetType interface to control Parquet conversion without needing to fork or upstream the change.

GitHub Issue: [Go] pqarrow - Support for parquet Interval type #43624

…ters consistent

go/arrow/extensions/extensions.go

go/arrow/extensions/json.go

go/arrow/extensions/uuid.go

go/parquet/metadata/app_version.go

go/parquet/pqarrow/schema.go

joellubi · 2024-08-21T21:21:19Z

Thanks for the thorough review @pitrou. The UUID extension was moved from a previously internal part of the repo, but I clearly didn't do the diligence to confirm it did in fact meet the current specification. Let me know if you have any other thoughts.

pitrou

With my very basic knowledge of Go, this looks ok to me. I'd rather @zeroshade vet it though :-)

zeroshade · 2024-08-22T17:16:28Z

go/arrow/extensions/json.go

+
+// GetOneForMarshal implements arrow.Array.
+func (a *JSONArray) GetOneForMarshal(i int) interface{} {
+	return a.Value(i)


should this instead use ValueJSON and return the json.RawMessage instead?

Yes that makes more sense!

zeroshade · 2024-08-22T17:18:18Z

go/arrow/extensions/json_test.go

+			expectedJSON := strings.ReplaceAll(`
+				{"json":"foobar"}
+				{"json":null}
+				{"json":{"foo":"bar"}}
+				{"json":42}
+				{"json":true}
+				{"json":[1,true,"3",null,{"five":5}]}`,
+				"\t", "") + "\n" // strip indentation, add trailing newline
+			require.Equal(t, expectedJSON, buf.String())


you could probably use JSONEq instead of needing to do the ReplaceAll

RecordToJSON returns jsonlines rather than json itself, but it is still cleaner to do a line-by-line JSONEq comparison. I just added that.

zeroshade · 2024-08-22T17:18:35Z

go/arrow/extensions/uuid.go

+	*array.ExtensionBuilder
+}
+
+func NewUUIDBuilder(mem memory.Allocator) *UUIDBuilder {


should we add a docstring?

I must have missed that, just added one!

zeroshade

Just a couple nits to address first, otherwise I'm good with this

conbench-apache-arrow · 2024-08-23T03:26:31Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d47b305.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

…arquet logical type mapping (apache#43679) ### Rationale for this change - Missing `JSON` extension type implementation. - Current precedent in C++ (and thereby PyArrow) is that canonical extension types do not require manual registration. - Issues like apache#43640 and apache#43624 suggest that we need to expose ways of configuring parquet types written from arrow records, but casting the underlying data presents challenges for a generalized approach. ### What changes are included in this PR? - Move `UUIDType` from `internal` to `arrow/extensions` - Implement `JSON` canonical extension type - Automatically register all canonical extension types at initialization - remove register/unregister from various locations these extension types are used - Add new `CustomParquetType` interface so extension types can specify their target `LogicalType` in Parquet - Refactor parquet `fieldToNode` to split up `PrimitiveNode` type mapping for leaves from `GroupNode` composition - Simplify parquet `LogicalType` to use only value receivers ### Are these changes tested? Yes ### Are there any user-facing changes? - `UUID` and `JSON` extension types are available to end users. - Canonical extension types will automatically be recognized in IPC without registration. - Users with their own extension type implementations may use the `CustomParquetType` interface to control Parquet conversion without needing to fork or upstream the change. * GitHub Issue: apache#43624 Authored-by: Joel Lubinitsky <[email protected]> Signed-off-by: Joel Lubinitsky <[email protected]>

#### Summary Goes with cloudquery/plugin-pb-go#445 Interesting bit is https://arrow.apache.org/blog/2024/10/23/arrow-go-18.0.0-release/#canonical-extension-types that we might be able to drop our UUID and JSON own extension types, but probably better to do that separately Had to do 3ae4778 to get the tests working, followed apache/arrow#43679 as a reference ---

joellubi added 4 commits August 13, 2024 11:41

move uuid to extensions dir

7934998

cleanup exports and validate interface

91e9475

add failing test for custom logical type

476ae68

impl custom logical type, refactor fieldToNode, make logicaltype poin…

77a5289

…ters consistent

github-actions bot added Component: Go awaiting committer review Awaiting committer review labels Aug 13, 2024

joellubi added 6 commits August 14, 2024 10:02

auto-register canonical extension types

a3b4e1f

make sure extensions properly reregistered in tests

3015937

add json canonical extension type

25c7645

custom parquet logicaltype for json

f021841

add godoc

0ec706f

update extension type impl status

2b1389a

github-actions bot added the Component: Documentation label Aug 14, 2024

joellubi changed the title ~~GH-43624: [Go] Add Support for Custom Mapping b/w Arrow ExtensionType and Parquet LogicalType~~ GH-43624: [Go] Add JSON/UUID extension types, extend arrow -> parquet logical type mapping Aug 14, 2024

joellubi marked this pull request as ready for review August 14, 2024 19:58

joellubi requested a review from zeroshade as a code owner August 14, 2024 19:58

This was referenced Aug 14, 2024

[Go] pqarrow - Provide a way to support Parquet Logical types that do not have or need Arrow equivalent types #43640

Closed

GH-17682: [C++][Python] Bool8 Extension Type Implementation #43488

Merged

datbth mentioned this pull request Aug 15, 2024

GH-43640: [Go] allow setting Logical types for pqarrow file writer #43641

Closed