Fix generic type validation to support datetimes and non-ASCII strings. #291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
And also document which type is compatible with which `DataType`.
…ing. If the generic type is string, we check ourselves that the `DataType` is a string one. This ensures the exception thrown is `ArgumentException` instead of `TileDBException`. The exception message also is more descriptive.
teo-tsirpanis
force-pushed
the
type-validation
branch
from
019ff81 to
5ccc03f
Compare
|
The failing examples in #258 succeed after this. |
Is there any way that external users could be relying on this behavior? |
|
Besides the validation we do, one other use case I could find for this method is to dynamically construct an array of that type using reflection, as part of processing TileDB arrays of unknown schemata, but there are better ways of doing this. Another thing we could do is make an internal edition of this method with the updated behavior and deprecate the public |
ihnorton
reviewed
Oct 16, 2023
| @@ -55,5 +56,39 @@ public static void ThrowIfManagedType<T>() | |||
| ThrowHelpers.ThrowManagedType(); | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Returns whether values of type <typeparamref name="T"/> can be stored or | |||
There was a problem hiding this comment.
The obsoletions in question have not yet shipped.
ihnorton
approved these changes
Oct 17, 2023
|
Thanks |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SC-24612
The C# API's rules around type validation in its generic methods have some defects that prevent datetime types and non-ASCII strings from being used. This PR improves the type validation to fix these cases.
Summary of changes
Typeto a TileDBDataTypeand compare them, we do the opposite. This happens because (with few exceptions), the mapping between .NET types and TileDB data types is one-to-many, and so by "narrowing down" we can validate that datetime data types match withlong, whilelongmaps toDataType.Int64.ErrorHandling.CheckDataType. It accepts a generic type parameter and aDataTyperegular parameter, and fails if they are incompatible.DataType.BooleanandStringUtf16. Besidesbyteandushort, they can also match withboolandchar(in .NET it's 2 bytes long) respectively.StringUtf32andSystem.Text.Rune, but the Core must validate that UTF-32 strings contain valid Unicode codepoints (if it already does, please let me know).EnumUtil.DataTypeToTypemethod mapped all stringDataTypes tosbyte, which is an obviously wrong behavior, and inconsistent with what the C++ API does. This PR changes it to map ASCII and UTF-8 strings tobyte, UTF-16 strings toushort, and UTF-32 strings touint.DataTypes with multiple valid types (likeStringUtf16andBoolean),DataTypeToTypereturns the matching numeric type.ArgumentExceptionwhich is more appropriate thanInvalidOperationException. Not commonly expected to hit, but worth mentioning.