SNOW-1488703: Add arrow support for structured types #1178
+591
−80
Conversation
This file contains hidden or 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
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types # Conflicts: # Snowflake.Data.Tests/IntegrationTests/StructuredArraysIT.cs # Snowflake.Data.Tests/IntegrationTests/StructuredMapsIT.cs # Snowflake.Data.Tests/IntegrationTests/StructuredObjectsIT.cs
Contributor
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
❌ 7 Tests Failed:
View the full list of 10 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
This reverts commit a034791.
This reverts commit 5add0a3.
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
sfc-gh-knozderko
suggested changes
Jul 18, 2025
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
…ps://github.com/snowflakedb/snowflake-connector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
sfc-gh-pszpakowski
requested review from
sfc-gh-mgemra
and removed request for
sfc-gh-dstempniak
| case Decimal128Array decimals: return decimals.GetValue(index); | ||
| case Int32Array ints: return ints.GetValue(index); | ||
| case Int64Array longs: return longs.GetValue(index); | ||
| case StringArray strArray: |
Collaborator
There was a problem hiding this comment.
should there be a boolean array?
Collaborator
Author
There was a problem hiding this comment.
Added case for boolean arrays
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
| case DoubleArray doubles: return doubles.GetValue(index); | ||
| case FloatArray floats: return floats.GetValue(index); | ||
| case Decimal128Array decimals: return decimals.GetValue(index); | ||
| case Int32Array ints: return ints.GetValue(index); |
Collaborator
There was a problem hiding this comment.
Is there any reason for not including Int8Array, Int16Array, BinaryArray, Date32Array and FixedSizeListArray? I see all of them are handled in ExtractCell method.
Collaborator
Author
There was a problem hiding this comment.
I missed them when going through the list of cases. They are added now
Collaborator
Author
There was a problem hiding this comment.
I was trying to make a test for FixedSizeListArray and found that ARRAY_CONSTRUCT and OBJECT_CONSTRUCT does not support VECTOR and that's why FixedSizeListArray wasn't initially included
Also just to let you know, currently the response always return number arrays in either Decimal128Array or DoubleArray regardless of the numeric data type they were assigned in ARRAY_CONSTRUCT
| var json = stringValue == null ? null : JObject.Parse(stringValue); | ||
| return JsonToStructuredTypeConverter.ConvertMap<TKey, TValue>(fields, json); | ||
| var val = GetValue(ordinal); | ||
| if (val is string stringValue) |
Collaborator
There was a problem hiding this comment.
nit: Isn't this more readable? Here and in GetArray
switch (val)
{
case string stringValue:
{
var json = JObject.Parse(stringValue);
return JsonToStructuredTypeConverter.ConvertObject<T>(fields, json);
}
case Dictionary<string, object> structArray:
return ArrowConverter.ConvertObject<T>(structArray);
default:
return null;
}
Collaborator
Author
There was a problem hiding this comment.
Replaced with switch case for all the Get methods
| return result; | ||
| } | ||
|
|
||
| private static object ConvertValue(object value, Type targetType) |
Collaborator
There was a problem hiding this comment.
Please use switch case for clarity
Comment on lines
44
to
69
| private static void MapPropertiesByNames(object obj, Dictionary<string, object> dict, Type type) | ||
| { | ||
| foreach (var kvp in dict) | ||
| { | ||
| var prop = type.GetProperty(kvp.Key, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance); | ||
| if (prop != null) | ||
| { | ||
| var converted = ConvertValue(kvp.Value, prop.PropertyType); | ||
| prop.SetValue(obj, converted); | ||
| } | ||
| else | ||
| { | ||
| foreach (var property in type.GetProperties()) | ||
| { | ||
| foreach (var attr in property.GetCustomAttributes().OfType<SnowflakeColumn>()) | ||
| { | ||
| if (attr?.Name == kvp.Key) | ||
| { | ||
| var converted = ConvertValue(kvp.Value, property.PropertyType); | ||
| property.SetValue(obj, converted); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| private static void MapPropertiesByNames(object obj, Dictionary<string, object> dict, Type type) | |
| { | |
| foreach (var kvp in dict) | |
| { | |
| var prop = type.GetProperty(kvp.Key, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance); | |
| if (prop != null) | |
| { | |
| var converted = ConvertValue(kvp.Value, prop.PropertyType); | |
| prop.SetValue(obj, converted); | |
| } | |
| else | |
| { | |
| foreach (var property in type.GetProperties()) | |
| { | |
| foreach (var attr in property.GetCustomAttributes().OfType<SnowflakeColumn>()) | |
| { | |
| if (attr?.Name == kvp.Key) | |
| { | |
| var converted = ConvertValue(kvp.Value, property.PropertyType); | |
| property.SetValue(obj, converted); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| private static void MapPropertiesByNames(object obj, Dictionary<string, object> dict, Type type) | |
| { | |
| foreach (var kvp in dict) | |
| { | |
| var prop = FindPropertyByName(type, kvp.Key); | |
| if (prop != null) | |
| { | |
| var converted = ConvertValue(kvp.Value, prop.PropertyType); | |
| prop.SetValue(obj, converted); | |
| } | |
| } | |
| } | |
| private static PropertyInfo FindPropertyByName(Type type, string name) | |
| { | |
| var prop = type.GetProperty(name, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance); | |
| if (prop != null) | |
| return prop; | |
| return type.GetProperties() | |
| .FirstOrDefault(p => p.GetCustomAttributes() | |
| .OfType<SnowflakeColumn>() | |
| .Any(attr => attr?.Name == name)); | |
| } |
IMO, it's more readable
| if (type.GetCustomAttributes(false).Any(attribute => attribute.GetType() == typeof(SnowflakeObject))) | ||
| { | ||
| var constructionMethod = JsonToStructuredTypeConverter.GetConstructionMethod(type); | ||
| if (constructionMethod == SnowflakeObjectConstructionMethod.PROPERTIES_NAMES) |
Collaborator
There was a problem hiding this comment.
Please use switch case for nested if statements to improve clarity
Collaborator
Author
There was a problem hiding this comment.
Refactored to switch case
| private static void MapPropertiesByOrder(object obj, Dictionary<string, object> dict, Type type) | ||
| { | ||
| var index = 0; | ||
| foreach (var property in type.GetProperties()) |
Collaborator
There was a problem hiding this comment.
Is the order of properties guaranteed in type.GetProperties()?
Collaborator
Author
There was a problem hiding this comment.
Based on the docs, the order is only guaranteed for .NET 7 and above
I checked its JSON counterpart and it also uses type.GetProperties(). One option could be ordering by metadata
| if (index >= dict.Count) | ||
| break; | ||
|
|
||
| var attributes = property.GetCustomAttributes().OfType<SnowflakeColumn>().ToList(); |
Collaborator
There was a problem hiding this comment.
Is the order of the attributes guaranteed in property.GetCustomAttributes()?
Collaborator
Author
There was a problem hiding this comment.
There doesn't seem to be a guarantee that GetCustomAttributes() will always list attributes in the same order on all .NET versions
It's replaced to get the first or default value instead of a list similar to JsonToStructuredTypeConverter
| var result = new T[list.Count]; | ||
| for (int i = 0; i < list.Count; i++) | ||
| { | ||
| if (targetType.IsGenericType && targetType.GetGenericTypeDefinition() == typeof(Nullable<>)) |
Collaborator
There was a problem hiding this comment.
Why is it checked inside the for loop? Let's move it outside
Collaborator
Author
There was a problem hiding this comment.
Done, it's now outside the for loop
| return CallMethod(targetType, objDict, nameof(ConvertObject)); | ||
| if (value is Dictionary<object, object> mapDict) | ||
| { | ||
| var genericArgs = targetType.GetGenericArguments(); |
Collaborator
There was a problem hiding this comment.
Shouldn't we do some kind of validation here? For example, should we check that the targetType is, in fact, a dictionary and has 2 generic arguments?
Collaborator
Author
There was a problem hiding this comment.
Added validation for targetType and the length of the generic arguments
Collaborator
|
Please also update the changelog |
Collaborator
Author
Updated |
…ps://github.com/snowflakedb/snowflake-connector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
…nector-net into SNOW-1488703-Arrow-Support-For-Structured-Types
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.
Description
Add arrow support for structured types
Checklist
dotnet test)