Add binding for tiledb_array_consolidate_fragments. #322
Conversation
shaunrd0
force-pushed
the
smr/sc-49723/add-array-consolidate-fragments
branch
from
927bba4
to
c0aa2c1
Compare
+ Fix size_t where uint64 is expected.
shaunrd0
force-pushed
the
smr/sc-49723/add-array-consolidate-fragments
branch
from
c0aa2c1
to
0e1a439
Compare
|
I opened a story for follow up to enable the test added in this PR. The test fails on 2.24.X but passes on 2.25.0 with the fix in TileDB-Inc/TileDB#5135. This is ready for review, the test failure was the only thing keeping this PR in draft. |
shaunrd0
force-pushed
the
smr/sc-49723/add-array-consolidate-fragments
branch
from
b4165a6
to
2e35c1f
Compare
|
@NullHypothesis @thetorpedodog this is now ready for review. I don't feel yet confident in reviewing Go code :/ |
| // cStringArray takes an array of Go strings and converts it to an array of CStrings. | ||
| // The function returned should be deferred by the caller to free allocated memory. | ||
| func cStringArray(stringList []string) ([]*C.char, func()) { | ||
| list := make([]*C.char, len(stringList)) | ||
| for i, str := range stringList { | ||
| list[i] = C.CString(str) | ||
| } | ||
|
|
||
| return list, func() { | ||
| for _, str := range list { | ||
| C.free(unsafe.Pointer(str)) | ||
| } | ||
| } | ||
| } |
array_experimental.go
Outdated
| list, freeMemory := cStringArray(fragmentList) | ||
| defer freeMemory() | ||
|
|
||
| ret := C.tiledb_array_consolidate_fragments(a.context.tiledbContext, curi, (**C.char)(unsafe.Pointer(&list[0])), C.uint64_t(len(list)), config.tiledbConfig) |
There was a problem hiding this comment.
(unsafe.Pointer(&list[0])) can be slicePtr(list).
| func TestGetConsolidationPlan(t *testing.T) { | ||
| // Create an 1d array | ||
| func createTestArray(t *testing.T) *Array { | ||
| // Create a 1d array |
There was a problem hiding this comment.
since the function createTestArray would be visible to all tests in this package, it might be worth making the name more specific, since the array created may not be suitable for all tests. maybe create1DTestArray?
array_experimental_test.go
Outdated
|
|
||
| array := createTestArray(t) | ||
|
|
||
| numFrags := uint32(5) |
There was a problem hiding this comment.
It looks like you’re doing this for the assert.Equal below…
array_experimental_test.go
Outdated
| require.NoError(t, err) | ||
| fragToVacuumNum, err := fragmentInfo.GetToVacuumNum() | ||
| require.NoError(t, err) | ||
| require.Equal(t, numFrags, fragToVacuumNum) |
There was a problem hiding this comment.
…but instead, you can write require.EqualValues(t, numFrags, fragToVacuumNum) (or require.Equal(t, uint32(numFrags), ...)) to avoid having a bunch of uint32(n)s above.
There was a problem hiding this comment.
Much better, very nice 👍
This adds the
tiledb_array_consolidate_fragmentsbinding required for cloud fragment list consolidation.CI will fail on this PR until TileDB-Inc/TileDB#5135 is merged and backported to the core version in use by TileDB-Go.