feat(partitioning): Add partition specs/fields and basic transform interface #2
Conversation
partitions_test.go
Outdated
| }) | ||
| } | ||
|
|
||
| errorTests := []struct { |
|
@nastra @coded9 @Fokko @rdblue @bitsondatadev Any further comments / reviews here? |
LGTM! |
| "fmt" | ||
| "strings" | ||
|
|
||
| "golang.org/x/exp/slices" |
There was a problem hiding this comment.
Generally I am really against using the experimental packages for anything that will be used likely in production. There recently has been a lot of people ignoring this and creating havoc in the release cycles of Go as seen in the following issues:
golang/go#51317
golang/go#61374
I think it would be better at this time to avoid using anything in the exp branch.
There was a problem hiding this comment.
@zeroshade thoughts on this one?
There was a problem hiding this comment.
Sorry I forgot to respond here on the PR because we ended up discussing this in the slack channel.
Tl;DR: the standard for Go in general is to target N-1 versions (ie. right now the latest is Go 1.21, so we want to target Go 1.20 + Go 1.21). This slices package was only added to the Go stdlib in Go 1.21, so we still need to point at golang.org/x/exp/slices until/unless we drop support for Go 1.20, at which point we can update this to just use the stdlib version of the package.
We're still about 6 months or so before Go 1.22 would get released, but this should be fine for now. Once Go 1.22 is released I'll replace these with the stdlib package, letting us maintain the N-1 compatibility.
| SourceID: 3, FieldID: 1001, Name: "id", Transform: bucket} | ||
| spec1 := iceberg.NewPartitionSpec(idField1) | ||
|
|
||
| assert.Zero(t, spec1.ID()) |
There was a problem hiding this comment.
independent from this PR, is there a good fluent assertion library available in the Golang ecosystem (similar to AssertJ)?
There was a problem hiding this comment.
I'm not familiar with AssertJ, but the most common assertion library I've seen for testing in the Golang ecosystem is the testify library that I'm using here.
| func (ps *PartitionSpec) NumFields() int { return len(ps.fields) } | ||
| func (ps *PartitionSpec) Field(i int) PartitionField { return ps.fields[i] } | ||
|
|
||
| func (ps *PartitionSpec) IsUnpartitioned() bool { |
There was a problem hiding this comment.
just to make sure: this will return true only if 1) fields are empty or 2) all transforms are void
There was a problem hiding this comment.
Correct, I've added tests accordingly.
| assert.Equal(t, 1002, spec3.LastAssignedFieldID()) | ||
| } | ||
|
|
||
| func TestUnpartitionedWithVoidField(t *testing.T) { |
There was a problem hiding this comment.
maybe also add a test where one field has a void transform and another field has some non-void transform, thus resulting spec.IsUnpartitioned() to be false
|
thanks for working on this @zeroshade! Should be good to go once the merge conflict is resolved |
zeroshade
force-pushed
the
partitioning-transforms
branch
from
a7dee3e to
beeaed2
Compare
|
@nastra resolved! 😄 |
Adds representations for Partition Specs, Partition Fields, and base transform interface + implementations. It does not add actual transformation functionality implementations yet. That will come later when implementing data reading. This adds only what is sufficient for reading a table definition / metadata.