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

Wrap tiledb_handle_load_array_schema_request #277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davisp
Copy link

@davisp davisp commented Oct 20, 2023

This wraps the new tiledb_handle_load_array_schema_request function in TileDB.

I was a bit unsure of where to put this. But based on the prior art for DeserializeLoadEnumerationsRequest, I stuck it in array_schema.go.

There's also at least one more of these coming for another handler though. So if you want me to create a new handlers.go or cloud_handlers.go or some such, I could also move the Enumeration handler and add the third all in one PR if that's preferable.

Also, I have no idea on Go style guides. I just know that this compiles. When I run go fmt locally it changes a lot of things that do not appear as they should changed so I've not run it and am just crossing my fingers I aped the DeserializeLoadEnumerationsRequest code closely enough.

@shortcut-integration
Copy link

@davisp
Copy link
Author

davisp commented Oct 20, 2023

Some of those test failures look like version incompatibilities and I can't decipher the others. I'm no Go expert so feel free to poke me with a clue stick if I've missed anything obvious.

Also, I believe this is the first ever Go PR I've ever submitted which is very exciting.

Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

LGTM but I'll leave for @anastasop to get a second set of eyes on the Go code, and merge.

array_schema.go Outdated Show resolved Hide resolved
array_schema.go Outdated Show resolved Hide resolved
@anastasop
Copy link
Contributor

anastasop commented Nov 14, 2023

@davisp the PR is OK. The tests fail because it is build against a TileDB 2.17.4 build. To eliminate the test failures you can cherry-pick the yaml commits of #279 or wait for a TileDB 2.18 release before resubmitting. No code changes are needed but cannot be merged until TileDB 2.18 is released which will lead to TileDB-Go 0.24

This wraps the new tiledb_handle_load_array_schema_request function in
TileDB.
@davisp davisp force-pushed the pd/sc-35285/wrap-handle-load-array-schema-request branch from 68869dc to bd45631 Compare January 24, 2024 15:32
@anastasop anastasop removed their assignment Apr 15, 2024
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