SNOW-950840 Parse new result metadata attributes for internal use #1806
Conversation
sfc-gh-rpanchapakesan
requested review from
sfc-gh-mloring and
sfc-gh-sfan
and removed request for
sfc-gh-mloring and
sfc-gh-sfan
sfc-gh-rpanchapakesan
force-pushed
the
rpanchapakesan/SNOW-950840-parse-new-result-metadata
branch
from
7d91f84 to
add7a7d
Compare
sfc-gh-rpanchapakesan
added
the
NO-CHANGELOG-UPDATES
sfc-gh-rpanchapakesan
requested review from
sfc-gh-sfan,
sfc-gh-mloring and
sfc-gh-aalam
sfc-gh-rpanchapakesan
changed the title
sfc-gh-rpanchapakesan
force-pushed
the
rpanchapakesan/SNOW-950840-parse-new-result-metadata
branch
from
deb0dad to
26729b7
Compare
sfc-gh-rpanchapakesan
force-pushed
the
rpanchapakesan/SNOW-950840-parse-new-result-metadata
branch
from
ef5dc65 to
3b7a22d
Compare
| is_nullable: bool, | ||
| display_size: int | None = None, | ||
| internal_size: int | None = None, | ||
| precision: int | None = None, | ||
| scale: int | None = None, |
There was a problem hiding this comment.
is there any particular reason why is_nullable moved up in order here compared to ResultMetadata. Does the ordering even matter?
There was a problem hiding this comment.
ordering does not matter. changes look good
There was a problem hiding this comment.
Yeah, I moved it up only since it doesn't have a default value. ResultMetadataV2 isn't a named tuple, so it shouldn't have any other user-visible effect.
sfc-gh-rpanchapakesan
deleted the
rpanchapakesan/SNOW-950840-parse-new-result-metadata
branch
| def describe(self, *args: Any, **kwargs: Any) -> list[ResultMetadata]: | ||
| def describe( | ||
| self, *args: Any, **kwargs: Any | ||
| ) -> list[ResultMetadata | ResultMetadataV2]: |
There was a problem hiding this comment.
Shouldn't this be just List[ResultMetadata]?
There was a problem hiding this comment.
Thanks for catching this: it should be just List[ResultMetadata] since describe only returns the old format. Same for _describe_internal(but for the new format) in your other comment.
|
|
||
| def _describe_internal( | ||
| self, *args: Any, **kwargs: Any | ||
| ) -> list[ResultMetadata | ResultMetadataV2]: |
There was a problem hiding this comment.
Is this returning a mix of ResultMetadata and ResultMetadataV2?
I added the
NO-CHANGELOG-UPDATESlabel since this change does not affect the public API of the connector. Let me know if I should instead update the changelog.Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-950840
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Created a new version of result metadata that contains new fields that have been added to the response (and are needed to support vector types in Snowpark). To avoid causing a breaking API change, this PR does not modify the existing
ResultMetadataclass (which is a named tuple so field additions could cause user-facing breakages). Instead, this change only adds parsing for a newResultMetadataV2class that internally replaces usages ofResultMetadata. The new metadata representation is only accessible via an internal attribute (for use by Snowpark).Longer-term, the old class should be deprecated, but, until that point, this serves as a stop-gap to unblock other work.