-
Notifications
You must be signed in to change notification settings - Fork 116
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
SNOW-1803811: Allow mixed-case field names for struct type columns #2640
SNOW-1803811: Allow mixed-case field names for struct type columns #2640
Conversation
125f1df
to
ed051f5
Compare
ed051f5
to
f05c058
Compare
@sfc-gh-aling @sfc-gh-aalam @sfc-gh-lspiegelberg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. left 2 minor comments
src/snowflake/snowpark/types.py
Outdated
return self.column_identifier.name if self.is_column else self._name | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does is_column=True
enforce type of self.column_identifier
is ColumnIdentifier
? can we add an assert for that in __init__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__init__
uses the setter for self.name
which ensures that self.column_identifier
is always ColumnIdentifier
.
# Global flag that determines if structured type semantics should be used | ||
_should_use_structured_type_semantics = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
who's responsible for setting this parameter, also do we need server side parameter for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be enabled until structured type support is rolled out. There will eventually be a server side parameter that we can track instead of this flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, if this is not rolled out yet then I'm good with this module var
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1803811
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This PR addresses an issue with StructType columns where names for StructFields are always uppercased. Snowflake always uppercases column names, but fields that are internal to StructType columns do not have casing enforced. Ideally we would not treat StructFields as column objects, but separating out that responsibility would require a BCR.
This change gets around the BCR requirement by using a feature flag. When enabled it shouldn't be much of a change as it assumes that only the top level of StructFields in a struct object are columns. Any structfields that are nested deeper in the schema are instead treated a OBJECT field names. This hierarchy should only exist in structured columns.