Add support for dimension labels #259

shaunrd0 · 2023-05-01T19:42:37Z

This adds support for dimension labels. I needed this support to continue working on SC-25205 (See blocking SC-28383 for more detail).

I'm generally new to Go, so please point out any weirdness and I will address asap. Specifically looking for help or advice on this PR in the following areas:

The changes to query.go depend on new dimension label APIs, so builds without the experimental tag will fail. I'll do some digging for ideas online, but curious if there's already a preferred way to prevent this.
There doesn't seem to be a Subarray object in TileDB-Go so I could not implement dimension label APIs that operate on Subarray. Should we add one in this PR or create a story for follow up?
I struggled to implement tiledb_array_schema_set_dimension_label_tile_extent. I'll take another crack at it, but decided to move on for today so I could get working on adding tests for the other new APIs here.

shortcut-integration · 2023-05-01T19:42:40Z

This pull request has been linked to:

+ Add binding for SetDimensionLabelTileExtent

anastasop · 2023-09-22T08:42:53Z

Rebased to latest master to get TileDB 2.17 and the Subarray APIs

thetorpedodog · 2023-09-28T14:27:46Z

dimension_label.go

+// AttributeName returns the name of the attribute the label data is stored under.
+func (d *DimensionLabel) AttributeName() (string, error) {
+	var labelAttrName *C.char
+	ret := C.tiledb_dimension_label_get_label_attr_name(d.context.tiledbContext, d.tiledbDimensionLabel, &labelAttrName)


Who owns the memory pointed to by labelAttrName after this?

Memory is owned by core but GoString at return makes a safe copy in the go space

Can you add a comment saying this? Just so that future readers know why this is safe. (Similarly in other cases where we get a reference to C-owned memory that we copy out of.)

thetorpedodog · 2023-09-28T14:34:18Z

dimension_label.go

+func (a *ArraySchema) DimensionLabelsNum() (uint64, error) {
+	var labelNum uint64
+
+	ret := C.tiledb_array_schema_get_dimension_label_num(a.context.tiledbContext, a.tiledbArraySchema, (*C.uint64_t)(unsafe.Pointer(&labelNum)))


here would recommend doing the same thing as you've done in other places with

var labelNum C.uint64_t // ... return uint64(labelNum)

which I believe should avoid the need for an unsafe.Pointer.

thetorpedodog · 2023-09-28T15:22:50Z

dimension_label.go

+	switch dimType {
+	case TILEDB_INT8:
+		tmpExtent := extent.(int8)


There are a few things going on here which will be annoying to deal with:

Since Go is garbage-collected, memory becomes eligible for freeing (and thus should be considered as already freed) after the last time they are referred to.

unsafe.Pointer intentionally is opaque to the GC, so something referred to by an unsafe.Pointer does not count as actually there.

This means that in this case:

tmpExtent gets space allocated (on the stack, in this case), and the value of extent is copied into it.

We create an unsafe.Pointer to tmpExtent.

tmpExtent is no longer referred to, thus making it eligible for collection.

We pass that pointer out to TileDB.

(This might be something we do elsewhere in this package; if so we need to fix it there too.)

In this case we need to ensure tmpExtent stays around. So I think we need to do something like:

var extentPtr any defer runtime.KeepAlive(extentPtr) // Here we can also reduce the amount of repetition by using a type-switch. switch extent := extent.(type) { case int8: // This ensures that `extent` stays alive. extentPtr = &extent // and since cExtent is a pointer to the same thing as extentPtr, it's OK. cExtent := unsafe.Pointer(&extent) // ... }

(This is annoying and fiddly.)

Done, thanks! Similar pattern was also in dimension.go, I made this change there too.

thetorpedodog · 2023-09-28T15:24:20Z

dimension_label.go

+	return nil
+}
+
+// getDimensionLabelDataType Retrieve a dimension label Datatype from the schema using experimental APIs.


*retrieves a dimension label

thetorpedodog · 2023-09-28T15:29:55Z

enums.go

+}
+
+// FromString Converts from a string to the equivalent DataOrder enum
+func (d *DataOrder) FromString(name string) error {


This seems like an odd API:

var myDO DataOrder myDO.FromString("someStr")

I would expect something like:

order, err := tiledb.DataOrderFromString(someStr)

Correct. Made it a package method

thetorpedodog · 2023-09-29T15:06:03Z

dimension_label.go

+// AttributeName returns the name of the attribute the label data is stored under.
+func (d *DimensionLabel) AttributeName() (string, error) {
+	var labelAttrName *C.char
+	ret := C.tiledb_dimension_label_get_label_attr_name(d.context.tiledbContext, d.tiledbDimensionLabel, &labelAttrName)


Can you add a comment saying this? Just so that future readers know why this is safe. (Similarly in other cases where we get a reference to C-owned memory that we copy out of.)

thetorpedodog · 2023-09-29T15:15:09Z

examples_lib/utils.go

@@ -11,6 +11,11 @@ func checkError(err error) {
 	}
 }

+func checkedValue[T any](v T, err error) T {


could you add a docstring here? a one-liner should suffice.

thetorpedodog · 2023-10-05T14:45:11Z

dimension_label.go

+	case int8:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case int16:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case int32:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case int64:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case uint8:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case uint16:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case uint32:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case uint64:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case float32:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case float64:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)
+	case bool:
+		extentPtr = &tmpExtent
+		cExtent = unsafe.Pointer(&tmpExtent)


shaunrd0 requested review from anastasop and snagles May 1, 2023 19:42

shaunrd0 force-pushed the smr/sc-25205/support-dimlabels branch from 48ae1fd to 0d661fe Compare May 24, 2023 17:54

shaunrd0 force-pushed the smr/sc-25205/support-dimlabels branch from 0d661fe to 3f6857f Compare July 21, 2023 13:03

shaunrd0 force-pushed the smr/sc-25205/support-dimlabels branch from 1849736 to ce55d2d Compare August 1, 2023 20:43

shaunrd0 and others added 7 commits September 22, 2023 11:37

Implement dimension label APIs

4169545

Remove new C APIs for dimension labels

1a46582

+ Add binding for SetDimensionLabelTileExtent

Remove duplicate function

3cfabd9

Editing, naming

345158c

Add missing schema bindings for dimension labels

1a5943c

Use t.TempDir in dimension labels tests

79569db

Add range APIs bindings for dimension labels

0885fde

anastasop force-pushed the smr/sc-25205/support-dimlabels branch from ce55d2d to 0885fde Compare September 22, 2023 08:42

anastasop requested a review from thetorpedodog September 22, 2023 08:43

thetorpedodog reviewed Sep 28, 2023

View reviewed changes

Spyros Anastasopoulos added 3 commits September 29, 2023 11:36

Make DataOrder from string a package method

38e48c2

Remove some unnecessary casts

4ee4e7f

Edit docs

a37826c

thetorpedodog reviewed Sep 29, 2023

View reviewed changes

Spyros Anastasopoulos and others added 3 commits October 2, 2023 11:29

Add docs and comments

d9e96bd

Set query subarray after setting ranges in dimension label examples

58dccb4

Ensure extent pointer is not finalized early

44bd673

anastasop requested a review from thetorpedodog October 4, 2023 11:52

thetorpedodog approved these changes Oct 5, 2023

View reviewed changes

anastasop merged commit 1b1b761 into master Oct 9, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for dimension labels #259

Add support for dimension labels #259

shaunrd0 commented May 1, 2023 •

edited

Loading

shortcut-integration bot commented May 1, 2023

anastasop commented Sep 22, 2023

thetorpedodog Sep 28, 2023

anastasop Sep 29, 2023

thetorpedodog Sep 29, 2023

thetorpedodog Sep 28, 2023

anastasop Sep 29, 2023

thetorpedodog Sep 28, 2023

shaunrd0 Oct 3, 2023

thetorpedodog Sep 28, 2023

thetorpedodog Sep 28, 2023

anastasop Sep 29, 2023

thetorpedodog Sep 29, 2023

thetorpedodog Sep 29, 2023

thetorpedodog Oct 5, 2023

Add support for dimension labels #259

Add support for dimension labels #259

Conversation

shaunrd0 commented May 1, 2023 • edited Loading

shortcut-integration bot commented May 1, 2023

anastasop commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaunrd0 commented May 1, 2023 •

edited

Loading