GH-34453: [Go] Support Builders for user defined extensions #34454

yevgenypats · 2023-03-04T19:45:11Z

This should serve as discussion as it's a medium change but this should Close #34453 and give users the ability to define custom Builder for their extensions just like they define ExtensionTypes and ExtensionArrays

Closes: [Go] Extension Builder Interface #34453

github-actions · 2023-03-04T19:45:32Z

Closes: [Go] Extension Builder Interface #34453

github-actions · 2023-03-04T19:45:35Z

⚠️ GitHub issue #34453 has been automatically assigned in GitHub to PR creator.

zeroshade

Took a look through, thoughts?

zeroshade · 2023-03-06T18:28:27Z

go/arrow/datatype_extension.go

+	// this should return array.Builder interface but we cannot import due to cycle import, so we use 
+	// interface{} instead. At least for 
+	NewBuilder(mem memory.Allocator, dt ExtensionType) interface{}


should we follow the same pattern as ArrayType and have a BuilderType method which returns a reflect.Type that we use to wrap the ExtensionBuilder with? This also avoids the import cycle.

Another thing to consider is that this is going to break any and all existing Extension types in other consumers' codebases. We should probably make a second interface type which contains the BuilderType method so that we can just use a type assertion test in NewBuilder rather than break existing consumers?

On the first part, we can keep the same pattern, sure. But actually I think this should 1) provide better performance, no reflection. 2) much better type safety, and better developer experience as it is clear what should be the type of this function (I also wanted to return Builder but had to fallback to interface due to some cyclic import that didn't want to fix part of this PR).

Re 2nd thing, not sure I followed, can you share an example of what this going to break? The only place that we use this function now is here and I fallback to the previous builder.

For the second point: Note that you had to add the NewBuilder method to the existing extension types. By adding a new method to the interface, anyone who has their own extension types defined that doesn't already have this method defined will have a compile error when they upgrade to this version of Arrow which adds the method because their structs will no longer meet the interface for ExtensionType.

But actually I think this should 1) provide better performance, no reflection. 2) much better type safety, and better developer experience as it is clear what should be the type of this function (I also wanted to return Builder but had to fallback to interface due to some cyclic import that didn't want to fix part of this PR).

In general, I wouldn't expect creating a builder to be a performance bottleneck as consumers shouldn't create builders repeatedly. That said, a couple ideas I've had so far:

Like was previously done for Arrays, we could shift the definition of the Builder interface to the arrow package directly and then add an alias in the array package to ensure we don't break any consumers (with a deprecated message telling people to point at arrow.Builder instead.

If we're going in this direction, rather than passing the allocator and the extension type, we should pass an ExtensionBuilder to the method and have this just wrap it and return the wrapped builder. The consumer can also retrieve the extension type and the allocator from the builder directly if they need to. So perhaps something like WrapBuilder(ExtensionBuilder) Builder.

Re breaking, I don't think this will breaking anything as there is a default in the ExtensionType that just returns nil to keep this backward compatible so if this is not implement I call the old NewExtensionBuilder in the NewBuilder function

@zeroshade can you please take a look at this comment ^ ? (This is why I didn't re-requested a re-review yet as need your guidance here)

Ah I missed that! Sorry! and you do check for the nil in NewBuilder, okay that's fair. So that avoids the breaking, you're completely correct.

So just need to address the other point in possibly changing the signature / solving the import cycle.

re using Builder in the signature I think it might be more complex than it seems (I tried it initially). For example newData *Data function which means we need also to take Data to arrow pacakge. In that case it might be even easier to move datatype_extension.go to array because we can import from array the arrow package but not the otherway around (Also, there are even more complications because the interface has private functions which would cause compilation error if we move it to a different package). Can't think of a much easier way without a big refactor which I think better to avoid atm and just do the runtime check. WDYT ?

Darn, that's really annoying..... I agree it's better to avoid a big refactor atm.

I still want to change the signature though instead of having consumers have to create both the underlying storage builder & wrap it.

Instead of adding this method to the ExtensionType interface, we could just go with an interface defined in the array package which would let us use Builder in the method definition. something like:

type ExtensionTypeCustomBuilder interface { NewBuilder(ExtensionBuilder) Builder }

Then in builder.go you can do:

case arrow.EXTENSION: typ := dtype.(arrow.ExtensionType) bldr := NewExtensionBuilder(mem, typ) if custom, ok := typ.(ExtensionTypeCustomBuilder); ok { return custom.NewBuilder(bldr) } return bldr

What do you think?

I like that! Updated.

go/arrow/internal/testing/types/extension_types.go

Blocked by cloudquery/plugin-sdk#724 but ready for initial review and discussion. I can do a short walkthrough for anyone up for review. Apache arrow fork is here (We use it until [this](apache/arrow#34454) is merged): https://github.com/cloudquery/arrow/tree/feat_extension_builder. Some more notes: - Right now we are just migrating the json writer/reader to use apache arrow so we can roll format by format and see if there are any real world issues before we roll this to everywhere instead of our own type system.

yevgenypats · 2023-03-07T07:02:52Z

Took a look through, thoughts?

@zeroshade Thanks for the initial review! Commented and also linked to a few example on how we use it.

zeroshade

Just request a re-review once this is ready for a new review, thanks.

yevgenypats · 2023-03-09T08:13:18Z

Ready now for another review, though looks like some workflows are failing. I think this is not connected though to this PR

zeroshade · 2023-03-09T15:58:20Z

go/arrow/internal/testing/types/extension_types.go

 	"golang.org/x/xerrors"
 )

+type UUIDBuilder struct {
+	*array.ExtensionBuilder
+	dtype *UUIDType


the failures are because this field is unused here

Oh ok. Fixed. I wasn't able to find it from the action. Where should I look for it next time? (Can I run those linters locally as well?)

So that was actually found by the Go build failing, and I saw it by looking at the log file for the failing Go workflows.

You can also run the linters (or most of the workflows) by following the instructions here: https://arrow.apache.org/docs/developers/continuous_integration/archery.html to install the archery utility in the repo and then running archery docker run <job> as described here.

Sounds good. Thanks, I'll try those for next PR! Tests passing now.

zeroshade

A few more nitpicks. We're almost there! Thanks again for all this!

go/arrow/array/extension_builder.go

go/arrow/datatype_extension.go

go/arrow/internal/testing/types/extension_test.go

go/arrow/internal/testing/types/extension_types.go

zeroshade · 2023-03-09T17:40:06Z

go/arrow/internal/testing/types/extension_types.go

+	data := make([][]byte, len(v))
+	for i, v := range v {
+		data[i] = v[:]
+	}
+	b.ExtensionBuilder.Builder.(*array.FixedSizeBinaryBuilder).AppendValues(data, valid)


this could be done more efficiently if desired via resize + UnsafeAppend or other methods. Not necessary for this PR but just something to think about.

go/arrow/internal/testing/types/extension_types.go

zeroshade

just a single whitespace nitpick haha. Fix that and i'll merge this 😄

go/arrow/datatype_extension.go

ursabot · 2023-03-11T00:04:00Z

Benchmark runs are scheduled for baseline = 71f3c56 and contender = 5219de3. 5219de3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.24% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.79% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.82% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 5219de33 ec2-t3-xlarge-us-east-2
[Failed] 5219de33 test-mac-arm
[Finished] 5219de33 ursa-i9-9960x
[Finished] 5219de33 ursa-thinkcentre-m75q
[Finished] 71f3c568 ec2-t3-xlarge-us-east-2
[Finished] 71f3c568 test-mac-arm
[Finished] 71f3c568 ursa-i9-9960x
[Finished] 71f3c568 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

yevgenypats requested a review from zeroshade as a code owner March 4, 2023 19:45

github-actions bot added Component: Go awaiting review Awaiting review labels Mar 4, 2023

kou changed the title ~~GH-34453: [GO] Support Builders for user defined extensions~~ GH-34453: [Go] Support Builders for user defined extensions Mar 4, 2023

yevgenypats mentioned this pull request Mar 6, 2023

feat: Arrow migration cloudquery/filetypes#87

Merged

zeroshade reviewed Mar 6, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 6, 2023

yevgenypats requested a review from zeroshade March 7, 2023 07:02

yevgenypats mentioned this pull request Mar 7, 2023

GH-34334: [Go][CSV] Support list fields #34343

Merged

yevgenypats force-pushed the feat/extension_builder branch from b6560ef to 2e0ae3d Compare March 8, 2023 05:24

zeroshade requested changes Mar 8, 2023

View reviewed changes

yevgenypats force-pushed the feat/extension_builder branch from 4a8a202 to 1d38d96 Compare March 9, 2023 07:54

yevgenypats added 3 commits March 9, 2023 09:57

apacheGH-34453: [GO] Support Builders for user defined extensions

b26606e

mod tidy

1a74b10

create better interface

8f8429a

yevgenypats force-pushed the feat/extension_builder branch from 1d38d96 to 8f8429a Compare March 9, 2023 07:58

mod tidy

5568a9b

yevgenypats requested a review from zeroshade March 9, 2023 08:00

zeroshade reviewed Mar 9, 2023

View reviewed changes

lint

3962d69

yevgenypats requested a review from zeroshade March 9, 2023 16:58

license fix

a7c58c3

zeroshade requested changes Mar 9, 2023

View reviewed changes

fix reviews

cbc15b3

yevgenypats requested a review from zeroshade March 10, 2023 07:21

zeroshade requested changes Mar 10, 2023

View reviewed changes

go/arrow/datatype_extension.go Outdated Show resolved Hide resolved

fmt

866360b

yevgenypats requested a review from zeroshade March 10, 2023 18:15

zeroshade approved these changes Mar 10, 2023

View reviewed changes

zeroshade merged commit 5219de3 into apache:main Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34453: [Go] Support Builders for user defined extensions #34454

GH-34453: [Go] Support Builders for user defined extensions #34454

yevgenypats commented Mar 4, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Mar 4, 2023

github-actions bot commented Mar 4, 2023

zeroshade left a comment

zeroshade Mar 6, 2023

yevgenypats Mar 7, 2023 •

edited

Loading

zeroshade Mar 7, 2023

yevgenypats Mar 7, 2023

yevgenypats Mar 8, 2023

zeroshade Mar 8, 2023

yevgenypats Mar 8, 2023

zeroshade Mar 8, 2023

yevgenypats Mar 9, 2023

yevgenypats commented Mar 7, 2023

zeroshade left a comment

yevgenypats commented Mar 9, 2023

zeroshade Mar 9, 2023

yevgenypats Mar 9, 2023

zeroshade Mar 9, 2023

yevgenypats Mar 9, 2023

zeroshade left a comment

zeroshade Mar 9, 2023

zeroshade left a comment

ursabot commented Mar 11, 2023

GH-34453: [Go] Support Builders for user defined extensions #34454

GH-34453: [Go] Support Builders for user defined extensions #34454

Conversation

yevgenypats commented Mar 4, 2023 • edited by github-actions bot Loading

github-actions bot commented Mar 4, 2023

github-actions bot commented Mar 4, 2023

zeroshade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yevgenypats Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yevgenypats commented Mar 7, 2023

zeroshade left a comment

Choose a reason for hiding this comment

yevgenypats commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeroshade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeroshade left a comment

Choose a reason for hiding this comment

ursabot commented Mar 11, 2023

yevgenypats commented Mar 4, 2023 •

edited by github-actions bot

Loading

yevgenypats Mar 7, 2023 •

edited

Loading