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-592647 consolidate definitions and resolve circular dependency issues #1158

Merged
merged 6 commits into from
Jun 8, 2022

Conversation

sfc-gh-mkeller
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller commented May 27, 2022

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-592647: IndexError: list index out of range in snowflake/connector/result_batch.py:698: IndexError #1145

  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 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.

    In this PR I fix an issue that was introduced in SNOW-499804 Add Geography Types #966 where a new type was added to snowflake.connector.constants.FIELD_TYPES, but this type was not added to FIELD_TYPE_TO_PA_TYPE defined in snowflake.connector.result_batch.ArrowResultBatch._create_empty_table.
    In this PR I move the later definition into the former to prevent them drifting apart in the future.
    I had to then move around definitions and import statements to prevent circular import issues and I also rewrote a couple of tests.

@sfc-gh-mkeller sfc-gh-mkeller self-assigned this May 27, 2022
@sfc-gh-mkeller sfc-gh-mkeller force-pushed the mkeller/SNOW-592647-geography_arrow branch from 7a3a790 to bf61e05 Compare May 27, 2022 23:32
Copy link
Contributor

@sfc-gh-sfan sfc-gh-sfan left a comment

Choose a reason for hiding this comment

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

The change itself looks good to me. I'm wondering if there is a test or linter to make sure we don't accidentally introduce circular dependencies in the future?

DESCRIPTION.md Outdated Show resolved Hide resolved
src/snowflake/connector/errors.py Outdated Show resolved Hide resolved
src/snowflake/connector/constants.py Show resolved Hide resolved
@sfc-gh-mkeller
Copy link
Collaborator Author

The change itself looks good to me. I'm wondering if there is a test or linter to make sure we don't accidentally introduce circular dependencies in the future?

Python itself will do this for you, just importing snowflake.connector caused Python to stop before I moved the import statements into the functions that needed them.

@sfc-gh-mkeller sfc-gh-mkeller force-pushed the mkeller/SNOW-592647-geography_arrow branch from dadd484 to 1c3fb18 Compare June 1, 2022 22:10
@sfc-gh-mkeller
Copy link
Collaborator Author

Thanks to #1158 (comment) I found that in the case of from . import subpackage the imported module acts lazy, so importing modules this way allows us to import everything at module level only and resolve them only when we need them only.
Good find @sfc-gh-aling , thank you!

@sfc-gh-mkeller sfc-gh-mkeller merged commit 2ccd3c8 into main Jun 8, 2022
@sfc-gh-mkeller sfc-gh-mkeller deleted the mkeller/SNOW-592647-geography_arrow branch June 8, 2022 21:41
@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2022
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.

SNOW-592647: IndexError: list index out of range in snowflake/connector/result_batch.py:698: IndexError
5 participants