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

Update enumerations exception message. #2096

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

shaunrd0
Copy link
Contributor

Unless I missed some testing done on earlier core versions I think we can just remove the version check here since TileDB dev has corrected the error message and it will not be present in 2.27.

For context, I had changed this message in TileDB-Inc/TileDB#5291 by adding a check earlier in the code with a different message. We essentially decided to revert those changes to fix a performance regression in TileDB-Inc/TileDB#5349 so the exception message returned to the previous format.

@shaunrd0 shaunrd0 requested a review from kounelisagis October 23, 2024 21:19
@kounelisagis
Copy link
Member

kounelisagis commented Oct 23, 2024

Thanks for opening the PR, @shaunrd0! It looks good.

My only concern -and if I remember correctly, it was also @ihnorton's concern- is the error message. 'Unable to check if unknown enumeration is loaded' doesn't seem to make much sense in that context. But it may be unrelated to this PR.

@kounelisagis
Copy link
Member

Should fix the related error in TileDB-Inc/centralized-tiledb-nightlies#24
cc @jdblischak

@ihnorton
Copy link
Member

As long as this works against 2.26 and 2.27 I'm ok with it.

@shaunrd0
Copy link
Contributor Author

As long as this works against 2.26 and 2.27 I'm ok with it.

Yep the change was reverted in 2.26 as well.

@kounelisagis kounelisagis merged commit e63fe29 into dev Oct 24, 2024
15 checks passed
@kounelisagis kounelisagis deleted the smr/update-enumerations-test branch October 24, 2024 14:54
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.

3 participants