ARROW-5640: [Go] Implement Arrow Map Array #10106

zeroshade · 2021-04-20T02:25:33Z

Took it upon myself to implement the Map Array type for Golang and uncomment the tests appropriately.

github-actions · 2021-04-20T02:25:55Z

https://issues.apache.org/jira/browse/ARROW-5640

zeroshade · 2021-04-20T02:26:02Z

Tagging @emkornfield @sbinet for visibility

WilliamWhispell · 2021-04-20T12:20:25Z

go/arrow/array/builder.go

@@ -277,6 +277,8 @@ func NewBuilder(mem memory.Allocator, dtype arrow.DataType) Builder {
 	case arrow.UNION:
 	case arrow.DICTIONARY:
 	case arrow.MAP:
+		typ := dtype.(*arrow.MapType)


Looks like in this change, you're only adding map support, but here you are changing union and dictionary to use the map builder. I'm not sure in this context the difference between map and dictionary.

you're still thinking in C++ context. Go doesn't automatically fallthrough like C++, this only modifies the Map case, it doesn't change anything about Union or Dictionary.

go/arrow/array/map.go

WilliamWhispell · 2021-04-20T12:24:55Z

go/arrow/array/map.go

+
+func arrayEqualMap(left, right *Map) bool {
+	// since Map is implemented using a list, we can just use arrayEqualList
+	return arrayEqualList(left.List, right.List)


Would maps with the same key values, but in different orders be considered equal?

Currently they would not, and as far as I can tell that seems consistent with other implementations. Someone else can tell me if i'm wrong there.

Yeah, I don't see anywhere in the spec docs or cod that mentions keysSorted with regards to comparison

it isn't. To my knowledge the keySorted doesn't have strong semantics other than to indicate the keys follow some logical ordering.

go/arrow/array/map.go

go/arrow/array/map_test.go

ggodik · 2021-04-20T14:25:02Z

go/arrow/array/map.go

+	}
+
+	if len(data.childData[0].childData) != 2 {
+		panic("arrow/array: map array child array should have two fields")


are the panics necessary here ? is this common in arrow/go ?

yea, just following the pattern that is used elsewhere. Until the version tags are added properly as part of the release process here I can't change that because it would require adding "error" returns that would be breaking changes in order to get rid of the panics.

zeroshade · 2021-04-23T16:45:20Z

bump for reviews and hopefully merges

emkornfield · 2021-04-25T20:59:38Z

go/arrow/array/array_test.go

@@ -85,10 +85,16 @@ func TestMakeFromData(t *testing.T) {
 		}},
 		{name: "duration", d: &testDataType{arrow.DURATION}},

+		{name: "map", d: &testDataType{arrow.MAP}, child: []*array.Data{
+			array.NewData(&testDataType{arrow.STRUCT}, 0, make([]*memory.Buffer, 4), []*array.Data{


nit: this is probably consistent with other code here, but literal comments like

/*elementByteWidth=*/4 could make this more readable.

added comments for literals

emkornfield · 2021-04-25T21:01:09Z

go/arrow/array/map.go

+
+// KeysSorted checks the datatype that was used to construct this array and
+// returns the KeysSorted boolean value used to denote if the key array is
+// sorted for each list element.


nit: might be worth commenting on the somewhat lack of semantics of keySored and linking to the spec.

added comments expanding on the keysorted lack of semantics

emkornfield · 2021-04-25T21:03:18Z

go/arrow/array/map.go

+// the Value field (the second field) of the child struct.
+func (a *Map) Items() Interface { return a.items }
+
+func (a *Map) Retain() {


docs for Retain and release?

emkornfield · 2021-04-25T21:03:50Z

go/arrow/array/map.go

+// NewMapBuilder returns a builder, using the provided memory allocator.
+// The created Map builder will create a map array whose keys will be a non-nullable
+// array of type `keytype` and whose mapped items will be a nullable array of itemtype.
+func NewMapBuilder(mem memory.Allocator, keytype, itemtype arrow.DataType, keysSorted bool) *MapBuilder {


might be worth noting keysSorted is not enforced here?

emkornfield · 2021-04-25T21:04:29Z

go/arrow/array/map.go

+	}
+}
+
+func (b *MapBuilder) Retain() {


docs for Retain and Release.

emkornfield · 2021-04-25T21:04:46Z

go/arrow/array/map.go

+// Len returns the current number of Maps that are in the builder
+func (b *MapBuilder) Len() int { return b.listBuilder.Len() }
+
+func (b *MapBuilder) Cap() int   { return b.listBuilder.Cap() }


docs for Cap and NullN?

emkornfield · 2021-04-25T21:06:41Z

go/arrow/array/map.go

+	keysSorted              bool
+}
+
+// NewMapBuilder returns a builder, using the provided memory allocator.


since Maps are relatively complex to work with, it might pay to give a simple usage example here.

I used the example_test file to provide a simple usage example which would show up in the generated docs on pkg.go.dev, I figured that was more directly useful than putting one here, but i can also add one here too.

added an example.

thanks, I didn't realize the examples end up on the docs page, I think that is sufficient from now on.

emkornfield · 2021-04-25T21:10:53Z

go/arrow/datatype_nested.go

+		panic("arrow: nil key or item type for MapType")
+	}
+
+	return &MapType{value: ListOf(StructOf(Field{Name: "key", Type: key}, Field{Name: "value", Type: item, Nullable: true}))}


does StructOf provide a name?

the current implementation of StructOf does not provide a name for the resulting struct, it only creates a datatype, rather than a Field. The DataType can then be used to create a Field and thus Name the struct.

The same is true for the current implementation of ListOf here. The result is a DataType not a field and thus doesn't have a name.

emkornfield · 2021-04-25T21:11:34Z

go/arrow/datatype_nested.go

+		panic("arrow: nil key or item type for MapType")
+	}
+
+	return &MapType{value: ListOf(StructOf(Field{Name: "key", Type: key}, Field{Name: "value", Type: item, Nullable: true}))}


does "key" and "value" hardcoded here have any implications for reading maps that name this differentlcy.

I'll double check but I don't believe I enforce these names having to be named this way during reading. That said, naming them this way is how the spec describes it should be done.

Ok, I confirmed that the names hardcoded here do not affect reading maps that name it differently. The assumption, as per the spec, is always that the first field is the keys and the second field is the values.

+1, thanks.

emkornfield · 2021-04-25T21:12:57Z

go/arrow/example_test.go

@@ -593,3 +593,66 @@ func Example_table() {
 	// rec[3]["f1-i32"]: [16 17 18 19 20]
 	// rec[3]["f2-f64"]: [16 17 18 19 20]
 }
+
+// This example demonstrates how to create a Map Array.
+// The resulting array should be:


should the comment at the end of the method be moved up here? Maybe provide a reference here instead of documentation of usage I mentioned above.

By putting the comment at the end of the method here in that format this example actually gets run as a test when running tests and confirms that the output of running this method matches the output comment at the end of the method. moving the comment at the end of the method would disable that benefit.

ahh, very cool.

The other cool aspect of this is that it shows up in the go docs too as an example like the examples on the pkg.go.dev docs here 😄

emkornfield · 2021-04-25T21:13:58Z

go/arrow/internal/arrjson/arrjson.go

-	Unit      string `json:"unit,omitempty"`
-	TimeZone  string `json:"timezone,omitempty"`
-	Scale     int    `json:"scale,omitempty"` // for Decimal128
+	Name       string `json:"name"`


this is just whitespace adjustment?

KeysSorted was added to the struct which is why the whitespace got adjusted

emkornfield · 2021-04-25T21:15:14Z

go/arrow/internal/arrjson/arrjson_test.go

+
+func makeMapsWantJSONs() string {
+	return `{


for testing purposes would it make sense to use a shorter example?

because these tests are generated from the arrdata record batches that's why this ends up this large. The reason why those records are the size they are is to ensure that we're properly testing handling multiple chunks with a map and multiple records.

emkornfield

Mostly seems reasonable, mostly nits and some questions.

zeroshade · 2021-04-26T15:04:31Z

@emkornfield I've added comments and docs as requested and responded to the questions. Lemme know if there's anything else needed.

emkornfield · 2021-04-27T15:41:49Z

@zeroshade I think this needs a rebase since the merge of the Decimal128 PR.

zeroshade · 2021-04-27T15:51:47Z

@emkornfield yup, i was already doing the rebase when you commented haha. I've pushed the rebase

zeroshade · 2021-04-27T18:16:39Z

@emkornfield all set with the rebase here now and all tests passed still, huzzah

Took it upon myself to implement the Map Array type for Golang and uncomment the tests appropriately. Closes apache#10106 from zeroshade/maptype Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

github-actions bot added the Component: Go label Apr 20, 2021

WilliamWhispell reviewed Apr 20, 2021

View reviewed changes

go/arrow/array/map.go Outdated Show resolved Hide resolved

WilliamWhispell reviewed Apr 20, 2021

View reviewed changes

go/arrow/array/map.go Outdated Show resolved Hide resolved

WilliamWhispell reviewed Apr 20, 2021

View reviewed changes

go/arrow/array/map_test.go Outdated Show resolved Hide resolved

WilliamWhispell reviewed Apr 20, 2021

View reviewed changes

go/arrow/array/map_test.go Outdated Show resolved Hide resolved

ggodik reviewed Apr 20, 2021

View reviewed changes

emkornfield reviewed Apr 25, 2021

View reviewed changes

emkornfield requested changes Apr 25, 2021

View reviewed changes

zeroshade requested a review from emkornfield April 25, 2021 23:46

Matthew Topol added 3 commits April 27, 2021 11:48

implement map type and array

d52162c

uncomment tests and update docs

046c6be

add map array example

1a6688a

Matthew Topol added 5 commits April 27, 2021 11:48

add some comments and docs

186bf3b

re-disable go null test, something weird with java integration there.

a8f3fa0

respond to feedback

8d3131c

comments added for docs and improving examples / comments.

de7dc35

comments added for literals

33cf0ae

zeroshade force-pushed the maptype branch from c812466 to 33cf0ae Compare April 27, 2021 15:51

emkornfield closed this in 0d11014 Apr 28, 2021

zeroshade deleted the maptype branch September 12, 2021 19:35

asfimport mentioned this pull request Sep 29, 2021

[Go] implement Map array #22076

Closed

ARROW-5640: [Go] Implement Arrow Map Array #10106

ARROW-5640: [Go] Implement Arrow Map Array #10106

Conversation

zeroshade commented Apr 20, 2021

github-actions bot commented Apr 20, 2021

zeroshade commented Apr 20, 2021

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

Choose a reason for hiding this comment

zeroshade commented Apr 23, 2021

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

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

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

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

emkornfield left a comment

Choose a reason for hiding this comment

zeroshade commented Apr 26, 2021

emkornfield commented Apr 27, 2021

zeroshade commented Apr 27, 2021

zeroshade commented Apr 27, 2021