SNOW-2465273: Add core library import #2657
Conversation
| # Load the core library - failures are captured in core_loader and don't prevent module loading | ||
| try: | ||
| core_loader.load() | ||
| except Exception: | ||
| # Silently continue if core loading fails - the error is already captured in core_loader | ||
| # This ensures the connector module loads even if the minicore library is unavailable | ||
| pass | ||
|
|
There was a problem hiding this comment.
This is probably redundant because load method is already big try / except
sfc-gh-pczajka
left a comment
sfc-gh-pczajka
left a comment
There was a problem hiding this comment.
Approved with small non-blocking comments
| if self._is_core_disabled(): | ||
| self._error = "mini-core-disabled" | ||
| return | ||
| self._error = "still-loading" |
There was a problem hiding this comment.
Could we wrap it into an error / change class definition to Exception | str | None?
There was a problem hiding this comment.
Can you add more context why? _is_core_disabled is imo safe on its own. All getters should be safe to use. The only one potentially causing the issue is get_core_version but bytes decoding is in try/except already.
There was a problem hiding this comment.
Just keeping types clean, it won't cause problems now as all reads are later casted to str anyway.
The field definition promises Error, not Error or str, I'd expect that content is raiseable if exists.
| def load(self): | ||
| """Spawn a separate thread to load the minicore library (non-blocking).""" | ||
| if self._is_core_disabled(): | ||
| self._error = "mini-core-disabled" |
There was a problem hiding this comment.
Could we wrap it into an error / change class definition to Exception | str | None?
sfc-gh-turbaszek
force-pushed
the
turbaszek-add-core-import
branch
from
027ac5a to
86bf408
Compare
sfc-gh-turbaszek
changed the title
| elif machine in ("aarch64", "arm64"): | ||
| return "aarch64" | ||
| elif machine in ("i686", "i386", "x86"): | ||
| return "i686" |
There was a problem hiding this comment.
do we support x86 minicore in python driver?
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 #NNNN
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.
(Optional) PR for stored-proc connector: