Add enumerations #269
Add enumerations #269
Conversation
anastasop
force-pushed
the
add-enumerations
branch
3 times, most recently
from
0af2936
to
9bad1c8
Compare
enumeration.go
Outdated
| } | ||
|
|
||
| // NewEnumeration creates an enumeration with name and ordered or not values. | ||
| func NewEnumeration[T EnumerationType](tdbCtx *Context, name string, ordered bool, values []T) (*Enumeration, error) { |
There was a problem hiding this comment.
I'm not a fan of boolean parameters, since it's not clear to the caller what the true in tiledb.NewEnumeration(someCtx, "myEnum", true, ...) might mean. It might be better to either split this into a NewEnumeration/NewOrderedEnumeration pair, or make the ordered parameter its own type so you can say tiledb.NewEnumeration(someCtx, "myEnum", tiledb.EnumOrdered, ...) or something like that.
| cData = reflect.ValueOf(data).UnsafePointer() | ||
| cDataLen = C.uint64_t(dataSize) | ||
| cOffsets = reflect.ValueOf(offsets).UnsafePointer() | ||
| cOffsetsLen = C.uint64_t(len(values) * int(reflect.TypeOf(uint64(0)).Size())) |
There was a problem hiding this comment.
since data and offsets are no longer referred to after these, they need a defer runtime.KeepAlive(...)
| cMode := C.CString("w") | ||
| defer C.free(unsafe.Pointer(cMode)) | ||
|
|
||
| cFile := C.fopen(cPath, cMode) |
There was a problem hiding this comment.
Up to you but might it be better to use file, err := os.Create(path); defer file.Close(); C.tiledb_enumeration_dump(..., file.Fd()) (or whatever the function to get the fd number from a File is)?
There was a problem hiding this comment.
Unfortunately the API requires a C.FILE not a file descriptor. You cannot get this from os package
There was a problem hiding this comment.
I see. I actually didn’t know that a FILE * was not the same as a fd until just now! Thanks for the learning opportunity.
enumeration.go
Outdated
| if typ != TILEDB_STRING_ASCII { | ||
| switch typ { | ||
| case TILEDB_BOOL: | ||
| return unsafe.Slice((*bool)(cData), cDataSize), nil |
There was a problem hiding this comment.
It looks like this is pointing to data inside of the Enumeration. So if I call
vals, _ := myEnum.Values()
// ... time passes, myEnum is GC’d
thing = vals[whatever]so vals is now unsafe.
There was a problem hiding this comment.
Make copies for safety
|
@thetorpedodog Setup github actions for 2.17.0-rc0 and works. Merge is pending until we test with a 2.17 release |
The TileDB core API return pointer to values which is in unsafe memory and also it can be changed by the user. This commit adds safety by copying the values to managed memory.
anastasop
force-pushed
the
add-enumerations
branch
from
be8f565
to
09ecf75
Compare
|
@thetorpedodog Rebased to use TileDB-2.17.0 release |
|
still looks good |
The TileDB enumerations PR can be found at TileDB-Inc/TileDB#4051
A usage example can be found at https://github.com/TileDB-Inc/TileDB/blob/dev/examples/cpp_api/enumerations.cc