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

adds string dimension support #108

Conversation

antalakas
Copy link
Contributor

@antalakas antalakas commented Apr 10, 2020

@antalakas antalakas force-pushed the an/ch1815/support_string_dimensions branch from 3a3c95b to 4a81187 Compare April 12, 2020 19:41
@antalakas antalakas changed the base branch from master to an/ch1832/support_non_empty_domain_var April 13, 2020 13:38
@antalakas antalakas force-pushed the an/ch1815/support_string_dimensions branch 2 times, most recently from 899dd6b to 7de5bb5 Compare April 13, 2020 14:34
@antalakas antalakas force-pushed the an/ch1832/support_non_empty_domain_var branch 4 times, most recently from b3409cd to 83eee25 Compare April 14, 2020 13:22
@antalakas antalakas force-pushed the an/ch1815/support_string_dimensions branch 4 times, most recently from b65efa3 to 1053599 Compare April 14, 2020 20:11
array.go Outdated
return nil, false, err
}

nonEmptyDomain, err := getNonEmptyDomainForDim(dimension, tmpDomain)
Copy link
Member

Choose a reason for hiding this comment

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

I believe you'll have to iterate through tmpDomain here, else you'll end up with the same values. It might be easier to drop tiledb_array_get_non_empty_domain and switch to tiledb_array_get_non_empty_domain_from_index inside the for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tmpDomain is just a slice here. Initially it was used in a call to makeNonEmptyDomain which was receiving a domain and did a loop in dimensions. Here we just use getNonEmptyDomainForDim which accepts a diimension and we are performing the loop through domain dimensions around this function. I don't think i changed the intention of the initial code. I will add a test for tiledb_array_get_non_empty_domain to make sure anyway

Copy link
Member

Choose a reason for hiding this comment

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

The tmpDomain is created with tmpDomain, tmpDomainPtr, err := domainType.MakeSlice(uint64(2 * ndims)). So it is the size of 2*ndims. In the new getNonEmptyDomainForDim you access index 0 and 1 of the passed slice. The problem I see is you always pass the same slice, tmpDomain so for every dimension you are just accessing tmpDomain[0] and tmpDomain[1]. You need to subslice tmpDomain based on the index dimension, however that would require you to add switch cases for all the datatypes. I think it might be easier to switch to tiledb_array_get_non_empty_domain_by_index to just fetch the single dimension's nonEmptyDomain to pass to getNonEmptyDomainForDim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. For this case, using the indexed version works well. In the case of tiledb_deserialize_array_nonempty_domain there is no indexed version so there is no choice but using the switch

serialize.go Outdated
return nil, false, err
}

nonEmptyDomain, err := getNonEmptyDomainForDim(dimension, tmpDomain)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above you will need to iterate through tmpDomain here, else you'll end up with the same values.

@antalakas antalakas force-pushed the an/ch1815/support_string_dimensions branch from 1053599 to d285c33 Compare April 15, 2020 11:59
@Shelnutt2
Copy link
Member

Thanks for the changes, looks great!

@antalakas antalakas merged commit 63e9dc1 into an/ch1832/support_non_empty_domain_var Apr 15, 2020
@Shelnutt2 Shelnutt2 deleted the an/ch1815/support_string_dimensions branch November 25, 2020 12:35
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.

2 participants