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

SNOW-966003: Fix Arrow return value for zero-row queries #1832

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

sfc-gh-aling
Copy link
Collaborator

@sfc-gh-aling sfc-gh-aling commented Dec 18, 2023

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-966003, dup PR: SNOW-966003: Fix Arrow return value for zero-row queries #1801 (comment)

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

@sfc-gh-aling sfc-gh-aling changed the title Snow 966003 SNOW-966003: Fix Arrow return value for zero-row queries Dec 18, 2023
@sfc-gh-aling sfc-gh-aling marked this pull request as ready for review December 18, 2023 21:31
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the overloading this is good to go by me.

🚢 🚢

src/snowflake/connector/cursor.py Outdated Show resolved Hide resolved
src/snowflake/connector/result_set.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 🚢

@thomasaarholt
Copy link
Contributor

The type signature came from me writing this in vscode with the pyright type checker.

Looks like mypy and pyright have slightly different behaviour. pyright allows my usage, while mypy does not. Here is a version that satisfies both . I believe the return type of None when Literal[False] is provided, is correct.

from typing import Literal, overload, reveal_type, NewType

Table = NewType("Table", object)


class Foo:
    @overload
    def fetch_arrow_all(self, force_return_table: Literal[False]) -> None:
        ...

    @overload
    def fetch_arrow_all(self, force_return_table: Literal[True]) -> Table:
        ...

    def fetch_arrow_all(self, force_return_table: bool = False) -> Table | None:
        return Table(1) if force_return_table else None


fetch_true = Foo().fetch_arrow_all(True)
fetch_false = Foo().fetch_arrow_all(False)
reveal_type(fetch_true)  # Table
reveal_type(fetch_false)  # None

@sfc-gh-aling
Copy link
Collaborator Author

sfc-gh-aling commented Dec 19, 2023

thanks @sfc-gh-mkeller @thomasaarholt , I have applied the change.
I do agree with Mark that when it's force_return_table is False, it should be the old Table | None as for non-zero row result, pyarrow.Table is returned.

@thomasaarholt
Copy link
Contributor

Apologies, I was blinded by what the correct return type. You're absolutely right.

@sfc-gh-aling sfc-gh-aling merged commit 990df44 into main Dec 19, 2023
70 of 75 checks passed
@sfc-gh-aling sfc-gh-aling deleted the SNOW-966003 branch December 19, 2023 21:06
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
@sfc-gh-aling
Copy link
Collaborator Author

no worries @thomasaarholt , you are all good!
I merged this PR and the change will be carried in our 2024 Jan release.
thanks for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants