Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New delete fragments endpoints #279

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

anastasop
Copy link
Contributor

This implements the 2 new endpoints tiledb_array_delete_fragments_v2 and tiledb_array_delete_fragments_list for the upcoming TileDB core 2.18.0

@anastasop anastasop force-pushed the new-delete-fragments-endpoints branch from d9d6960 to 158ea63 Compare November 3, 2023 15:00
@KiterLuc KiterLuc closed this Nov 7, 2023
@KiterLuc KiterLuc reopened this Nov 7, 2023
@KiterLuc KiterLuc force-pushed the new-delete-fragments-endpoints branch from 1137117 to 70d03fb Compare November 7, 2023 13:33
@anastasop anastasop force-pushed the new-delete-fragments-endpoints branch from 348f516 to 8490cdf Compare November 8, 2023 16:13
@anastasop anastasop force-pushed the new-delete-fragments-endpoints branch 3 times, most recently from a453158 to 83cf0db Compare November 14, 2023 16:08
@anastasop
Copy link
Contributor Author

@snagles @thetorpedodog This is the final PR to support TileDB 2.18 and tag 0.24.0

Copy link
Contributor

@thetorpedodog thetorpedodog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! I have a few minor suggestions and questions left.

list = append(list, cfuri)
}

ret := C.tiledb_array_delete_fragments_list(tdbCtx.tiledbContext, curi, (**C.char)(unsafe.Pointer(&list[0])), C.size_t(len(list)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably something we should handle more broadly in the future, but you can use unsafe.SliceData to get the address of the start of a slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that was the initial code but needed to revert because there are some go1.19 installations and 1.19 does not have SliceData

array_test.go Outdated
err = array.Open(TILEDB_WRITE)
require.NoError(t, err)
for i := 1; i <= 10; i += 2 {
time.Sleep(100 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a note explaining why this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local disk is too fast and i needed fragments to have some time distance between them

array_test.go Outdated
Comment on lines 478 to 479
err = array.Open(TILEDB_WRITE)
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For situations where the function returns just an error, maybe require.NoError(t, array.Open(TILEDB_WRITE)) etc.?


// HandleArrayDeleteFragmentsTimestampsRequest is used by TileDB cloud to handle DeleteFragments with tiledb:// uris
func HandleArrayDeleteFragmentsTimestampsRequest(context *Context, array *Array, buffer *Buffer, serializationType SerializationType) error {
ret := C.tiledb_handle_array_delete_fragments_timestamps_request(context.tiledbContext, array.tiledbArray,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to runtime.KeepAlive the buffer or anything else here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added them for safety

@anastasop anastasop merged commit 33dbe7b into master Nov 27, 2023
11 checks passed
@anastasop anastasop deleted the new-delete-fragments-endpoints branch February 21, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants