Go: Schema and Types #8122

zeroshade · 2023-07-20T22:05:13Z

Initial PR of a Golang implementation of the Iceberg spec. This PR contains:

Representation of Schema and Types
GHA workflow to run unit tests and linting on the Go code
basic README/docs

zeroshade · 2023-07-20T22:05:59Z

CC @coded9 @Fokko @nastra @bitsondatadev @rdblue

.github/workflows/go-ci.yml

Fokko

This looks good, what else can I say since it is very much a copy of the PyIceberg code :D Thanks for working on this! Curious what @nastra thinks of this since he's much more experienced in Go than me.

Fokko · 2023-07-22T06:28:41Z

go/iceberg/schema.go

+		return false
+	}
+
+	return slices.EqualFunc(s.fields, other.fields, func(a, b NestedField) bool {


Just curious. In Python we use a generator that will break the loop once it encounters the first False:

schema_is_equal = all(lhs == rhs for lhs, rhs in zip(self.columns, other.columns))

Is there something similar in Go?

slices.EqualFunc will break its loop when it encounters the first false.

Fokko · 2023-07-22T06:37:35Z

go/iceberg/schema.go

+	if bl, ok := visitor.(BeforeListElementVisitor); ok {
+		bl.BeforeListElement(elemField)
+	} else if bf, ok := visitor.(BeforeFieldVisitor); ok {
+		bf.BeforeField(elemField)


I think we only want to visit the first branch of this if:

@visit.register(ListType) def _(obj: ListType, visitor: SchemaVisitor[T]) -> T: """Visit a ListType with a concrete SchemaVisitor.""" visitor.before_list_element(obj.element_field) result = visit(obj.element_type, visitor) visitor.after_list_element(obj.element_field) return visitor.list(obj, result)

The default implementation of before_list_element calls before_field (https://github.com/apache/iceberg/blob/c27c1fb1f279c8be96847fc41ce637b3eff19661/python/pyiceberg/schema.py#L282-284)

So it does require the second branch

Fokko · 2023-07-22T06:39:18Z

go/iceberg/schema_test.go

+)
+
+var (
+	tableSchemaNested = iceberg.NewSchemaWithIdentifiers(1,


Nice, thanks for using the same tests :)

go/iceberg/schema.go

Fokko · 2023-07-22T06:59:05Z

go/iceberg/schema_test.go

+		}, Required: true},
+		9:  {ID: 9, Name: "key", Type: iceberg.PrimitiveTypes.String, Required: true},
+		10: {ID: 10, Name: "value", Type: iceberg.PrimitiveTypes.Int32, Required: true},
+		11: tableSchemaNested.Field(5),


coded9 · 2023-07-22T07:22:48Z

go/iceberg/go.mod

+
+require (
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect


This is no longer maintained, do we still want to continue with this ?

They are indirect dependencies of github.com/stretchr/testify/assert which is very well maintained currently so I don't think it's an issue.

nastra · 2023-07-22T07:32:21Z

I'll review this early next week

go/iceberg/doc.go

nastra · 2023-07-24T10:48:15Z

go/iceberg/README.md

+
+# Get in Touch
+
+- [Iceberg community](https://iceberg.apache.org/community/)


I think it would be good to have a contributing section or file for GO so that people know how to set up Intellij and how to run tests and such (can be a follow-up)

yea, I'll follow up with that after I get the existing stuff merged.

go/iceberg/types.go

nastra · 2023-07-24T11:07:25Z

go/iceberg/types.go

+			t.Type = BinaryType{}
+		default:
+			switch {
+			case strings.HasPrefix(typename, "fixed"):


should we use the same regex that we're using on the Java side?

same comment as above, I was following the way that pyiceberg works, which regex is essentially just doing the same thing as I'm doing here by checking for the prefix and then parsing the number out of the brackets. If we're gonna switch this to the same regex as the Java side, we should probably do the same in pyiceberg so that all three are aligned, yes?

nastra · 2023-07-24T12:07:21Z

go/iceberg/schema_test.go

+
+package iceberg_test
+
+import (


I think all types should be tested, similar to TestSchemaConversions

Added tests for all the remaining types.

go/iceberg/LICENSE

nastra · 2023-07-28T13:39:20Z

go/.pre-commit-config.yaml

+    rev: v4.4.0
+    hooks:
+      - id: golangci-lint
+        entry: bash -c 'cd iceberg && golangci-lint run --fix --timeout 5m'


does this require any installation?

As with pyiceberg, you need to pip install pre-commit first and then you can run pre-commit install to get what you need.

I think for someone who isn't actively working on pyiceberg (like me) this isn't obvious what exactly is required for installation, so would be good to add this somewhere in the docs. This can be a follow-up and can be part of the Docs that describe how to set up IntelliJ with Go and such, so that non-Go committers can review code and know how to run tests

That makes sense, I can add stuff to the README for it. Though I have no idea how to set up IntelliJ with Go since I use VSCode lol, so i welcome any contributions on that 😄

So the thing is that most reviewers will be using Intellij and having some docs on how to set up stuff for Go would be great and also improves review cycles

Goland is the recommended IDE for go from Jetbrains, but it is paid. VScode has good support for Go compared to IntelliJ.
Core contributors of open source projects can get Goland for free

go/iceberg/types.go

nastra · 2023-07-28T13:54:15Z

go/iceberg/schema_test.go

+	assert.Truef(t, tableSchemaNested.Equals(&sc), "expected: %s\ngot: %s", tableSchemaNested, &sc)
+}
+
+func TestRemainingTypes(t *testing.T) {


I think there should be a types_test.go where this should live and where we can test all types with the JSON parsing, similar to how it's done on the Python side in test_types.py

zeroshade · 2023-08-01T21:15:57Z

@nastra I'll probably get to the remaining comments tomorrow, sorry for the delay here

zeroshade · 2023-08-09T16:46:01Z

@nastra just to confirm, is increasing the tests in types_test.go the only remaining thing here? or am i overlooking a comment somewhere?

nastra · 2023-08-09T16:50:30Z

@nastra just to confirm, is increasing the tests in types_test.go the only remaining thing here? or am i overlooking a comment somewhere?

I believe so, but I'd have to re-review the PR tomorrow. Sorry, for the review delay here.

nastra · 2023-08-10T09:46:10Z

go/iceberg/types_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestRemainingTypes(t *testing.T) {


Remaining is rather a confusing name. The goal here is to test all types, including lists/maps/nested structs.

I think test_types.py does a great job in testing JSON & string representations of all types, and I think we should do the same here as well

@zeroshade I think that the only item that is currently outstanding. We just need to make sure that all types are extensively tested

Updated and tests are updated too. Let me know if there's any other tests you think I should add. Confirmed that all JSON and string representations are covered in the tests (according to the test coverage reports)

thanks, I think this should be fine now. We should probably re-open this against https://github.com/apache/iceberg-go

i'll go do that

@nastra Looks like the iceberg-go repo is completely empty, the github UI doesn't allow forking an empty repository so that I can submit a PR. Could someone please commit a LICENSE file or empty README.md or something so that I can fork it to create the PR?

I added a README to iceberg-go, so hopefully things should work now

@nastra I've created apache/iceberg-go#1 😄 in addition to the existing code here I've added general dev script stuff there.

zeroshade · 2023-08-16T15:28:54Z

Closing this PR in favor of it having been merged / added to github.com/apache/iceberg-go

Go: Schema and Types

47e2123

github-actions bot added INFRA docs labels Jul 20, 2023

zeroshade added 6 commits July 20, 2023 18:07

add rat excludes

34678df

fix rat excludes

bca1e15

maybe this is correct?

5a47189

forgot i don't have a go.work in the CI

7683b28

exclude go.sum at top level?

43e87bb

don't need check-license at go dev level

011e81e

nastra reviewed Jul 21, 2023

View reviewed changes

.github/workflows/go-ci.yml Show resolved Hide resolved

Fokko approved these changes Jul 22, 2023

View reviewed changes

coded9 reviewed Jul 22, 2023

View reviewed changes

nastra reviewed Jul 24, 2023

View reviewed changes

zeroshade added 3 commits July 24, 2023 11:29

add paths-ignore to ci yamls

c5eb859

updates from feedback

59c0ab2

adding tests for other types

f816078

dramaticlly reviewed Jul 24, 2023

View reviewed changes

go/iceberg/LICENSE Show resolved Hide resolved

Add golang label to labeler

e314b3b

nastra reviewed Jul 28, 2023

View reviewed changes

updates from feedback

b2ec016

nastra reviewed Aug 10, 2023

View reviewed changes

update with more tests and feedback

3b0772a

zeroshade closed this Aug 16, 2023


		# Get in Touch

		- [Iceberg community](https://iceberg.apache.org/community/)


		package iceberg_test

		import (

Go: Schema and Types #8122

Go: Schema and Types #8122

Conversation

zeroshade commented Jul 20, 2023

zeroshade commented Jul 20, 2023 • edited Loading

Fokko left a 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

nastra commented Jul 22, 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

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

coded9 Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeroshade commented Aug 1, 2023

zeroshade commented Aug 9, 2023

nastra commented Aug 9, 2023

nastra Aug 10, 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

zeroshade commented Aug 16, 2023

zeroshade commented Jul 20, 2023 •

edited

Loading

coded9 Aug 15, 2023 •

edited

Loading

nastra Aug 10, 2023 •

edited

Loading