feat(snowflake): add pure-Python Snowflake SQL API adapter#25
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a new ChangesSnowflake SQL API Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
204-219: ⚡ Quick winMake overrideability explicit in the Snowflake example.
The section mentions NUI defaults, but the snippet doesn’t show how to override them. Adding one explicit override example keeps this feature clearly generic for non-NUI users.
Proposed README tweak
-Pure-Python Snowflake client (no `snowflake-connector-python`), keypair auth via -Secrets Manager, with NUI session defaults (`TIMEZONE=Pacific/Auckland`, role -`NUI_LAMBDA`) and a redacting query-logging hook. +Pure-Python Snowflake client (no `snowflake-connector-python`), keypair auth via +Secrets Manager, with NUI session defaults (`TIMEZONE=Pacific/Auckland`, role +`NUI_LAMBDA`) that you can override via `timezone=` and `role=`, plus a +redacting query-logging hook. @@ -client = create_snowflake_client(warehouse="COMPUTE_WH", database="ANALYTICS") +client = create_snowflake_client( + warehouse="COMPUTE_WH", + database="ANALYTICS", + timezone="UTC", + role="MY_APP_ROLE", +)Based on learnings: "Before adding new features, verify they are generic and useful for any Lambda project, not NUI-specific; if NUI-specific, document clearly and provide configuration options."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 204 - 219, The Snowflake example code snippet shows NUI-specific defaults like TIMEZONE and role but does not demonstrate how to override them, making the example appear NUI-specific rather than generic. Add an explicit example to the code snippet showing how to override at least one of the default settings (such as timezone or role) when calling create_snowflake_client, so that non-NUI users can clearly see these defaults are configurable and the client is suitable for generic Lambda projects. This demonstrates the overrideability of the NUI defaults and keeps the feature generic.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@README.md`:
- Around line 204-219: The Snowflake example code snippet shows NUI-specific
defaults like TIMEZONE and role but does not demonstrate how to override them,
making the example appear NUI-specific rather than generic. Add an explicit
example to the code snippet showing how to override at least one of the default
settings (such as timezone or role) when calling create_snowflake_client, so
that non-NUI users can clearly see these defaults are configurable and the
client is suitable for generic Lambda projects. This demonstrates the
overrideability of the NUI defaults and keeps the feature generic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b45ffee0-6dda-4f40-8f86-ffe9d968aba3
📒 Files selected for processing (9)
AGENTS.mdREADME.mdnui_lambda_shared_utils/snowflake_client.pynui_shared_utils/__init__.pynui_shared_utils/snowflake_client.pypyproject.tomlrequirements-test.txtsetup.pytests/test_snowflake_client.py
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
hampsterx
force-pushed
the
feat/snowflake-extra
branch
from
e00f28c to
79f21df
Compare
Contributor
Author
|
Applied the README nitpick: the Snowflake example now overrides |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
68-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify that
[all]does not include Snowflake.The install block currently says
[all]is “All integrations”, but this PR intentionally keeps Snowflake out of[all]. Please update wording to prevent confusion andImportErrorwhen users expect Snowflake to be included.Suggested docs tweak
-pip install nui-python-shared-utils[all] # All integrations +pip install nui-python-shared-utils[all] # Core optional integrations (excludes Snowflake) pip install nui-python-shared-utils[powertools] # AWS Powertools only ... pip install nui-python-shared-utils[snowflake] # Snowflake SQL API clientBased on learnings: Use specific extras (
[slack],[elasticsearch]) rather than[all]when installing the package to optimize Lambda bundle size.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 68 - 75, The documentation currently describes the [all] installation option as "All integrations" without clarifying that Snowflake is intentionally excluded from this bundle. Update the comment next to the [all] pip install command to explicitly state that it includes all integrations except Snowflake, and optionally add a note recommending users choose specific extras like [slack], [elasticsearch], [database], [jwt], and [powertools] individually instead of [all] to optimize Lambda bundle size, with [snowflake] installed separately if needed.Source: Learnings
tests/test_snowflake_client.py (1)
84-560:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd pytest markers to categorize this suite (
unit/integration/slow).The tests are unmarked right now. Please apply the required pytest markers so selective runs (
-m ...) and CI partitioning remain consistent.As per coding guidelines:
**/tests/test_*.py: Use@pytest.mark.unit,@pytest.mark.integration, and@pytest.mark.slowpytest markers to categorize tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_snowflake_client.py` around lines 84 - 560, Add pytest markers to categorize tests in the test_snowflake_client.py file. Apply `@pytest.mark.unit` to the majority of tests in TestGetCredentials, TestRedactingLogger, TestCreateClient (except test_real_sync_client_constructs and test_real_async_client_constructs), TestMissingExtra, and TestPackageWiring classes since they mock dependencies and test isolated functionality. Apply `@pytest.mark.integration` to tests like test_secrets_manager in TestGetCredentials, test_real_sync_client_constructs and test_real_async_client_constructs in TestCreateClient, and the entire TestRedactionEndToEnd class as these construct real objects or test end-to-end behavior. Add markers as class-level decorators using `@pytest.mark.unit` or `@pytest.mark.integration` on each test class, or as method-level decorators on individual test methods if different tests within a class require different categorizations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nui_shared_utils/snowflake_client.py`:
- Around line 132-139: The `private_key` parameter in the three public functions
`get_snowflake_credentials()`, `create_snowflake_client()`, and
`create_async_snowflake_client()` is incorrectly typed as `Optional[bytes]` when
it should accept both bytes and str (PEM text format) to match the
implementation in the `_as_key_bytes()` helper. Update the type annotation for
the `private_key` parameter in all three functions from `Optional[bytes]` to
`Optional[Union[bytes, str]]`. Ensure that Union is imported from the typing
module if it is not already present at the top of the file.
In `@README.md`:
- Line 225: The markdown linting error MD031 indicates missing blank lines
around a fenced code block. Locate the fenced code block in README.md that
contains the text "already on an event loop. Requires the `[snowflake]` extra."
and ensure there is a blank line immediately before the opening fence and a
blank line immediately after the closing fence to comply with markdownlint
formatting requirements.
---
Outside diff comments:
In `@README.md`:
- Around line 68-75: The documentation currently describes the [all]
installation option as "All integrations" without clarifying that Snowflake is
intentionally excluded from this bundle. Update the comment next to the [all]
pip install command to explicitly state that it includes all integrations except
Snowflake, and optionally add a note recommending users choose specific extras
like [slack], [elasticsearch], [database], [jwt], and [powertools] individually
instead of [all] to optimize Lambda bundle size, with [snowflake] installed
separately if needed.
In `@tests/test_snowflake_client.py`:
- Around line 84-560: Add pytest markers to categorize tests in the
test_snowflake_client.py file. Apply `@pytest.mark.unit` to the majority of tests
in TestGetCredentials, TestRedactingLogger, TestCreateClient (except
test_real_sync_client_constructs and test_real_async_client_constructs),
TestMissingExtra, and TestPackageWiring classes since they mock dependencies and
test isolated functionality. Apply `@pytest.mark.integration` to tests like
test_secrets_manager in TestGetCredentials, test_real_sync_client_constructs and
test_real_async_client_constructs in TestCreateClient, and the entire
TestRedactionEndToEnd class as these construct real objects or test end-to-end
behavior. Add markers as class-level decorators using `@pytest.mark.unit` or
`@pytest.mark.integration` on each test class, or as method-level decorators on
individual test methods if different tests within a class require different
categorizations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cc747eb-1568-4309-aafe-174f9b5c27de
📒 Files selected for processing (9)
AGENTS.mdREADME.mdnui_lambda_shared_utils/snowflake_client.pynui_shared_utils/__init__.pynui_shared_utils/snowflake_client.pypyproject.tomlrequirements-test.txtsetup.pytests/test_snowflake_client.py
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (4)
- requirements-test.txt
- pyproject.toml
- nui_lambda_shared_utils/snowflake_client.py
- nui_shared_utils/init.py
- Add [snowflake] extra + create_snowflake_client / create_async_snowflake_client so Lambdas query Snowflake without the 75MB snowflake-connector-python (pure-Python snowflake-sql-api over httpx) - Load keypair from Secrets Manager by default (env + explicit overrides), strict explicit > env > secret precedence with source-aware passphrase pairing - Apply NUI session defaults (TIMEZONE=Pacific/Auckland, role NUI_LAMBDA, both overridable); redacting query-log hook never logs bind values, JWTs, or key material - Sync client is the default; an async factory mirrors it for event-loop Lambdas - Dependency is a pre-release git-SHA pin of the public snowflake-sql-api repo, kept out of [all] so nothing pulls it implicitly WEEK_START is intentionally not a session default: the SQL API rejects it in the request parameters (HTTP 400) and is stateless per statement, so it cannot be set per call. Set it on the Snowflake user/role default if needed. Backwards compatible: additive only (new extra + new module). No change to existing consumers; nui_lambda_shared_utils shim re-exports the new surface.
hampsterx
force-pushed
the
feat/snowflake-extra
branch
from
79f21df to
7220955
Compare
Contributor
Author
|
Addressed the review:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[snowflake]extra andcreate_snowflake_client/create_async_snowflake_clientso Lambdas can query Snowflake without the 75MBsnowflake-connector-python. The adapter wraps the pure-Pythonsnowflake-sql-apiclient (httpx), which cold-starts in ~1.2s vs the connector's 8-20s.explicit > env > Secrets Managerprecedence and source-aware passphrase pairing (a key is only ever paired with a passphrase from its own source).TIMEZONE=Pacific/Auckland, roleNUI_LAMBDA), both overridable, and wires a redacting query-logging hook that logs the statement text plus bind-parameter count, never the bind values, JWTs, key material, or the passphrase.snowflake-sql-apirepo, kept out of[all]so nothing pulls the git dependency implicitly. It will be repinned to a PyPI release once that package ships.WEEK_STARTis intentionally not a session default: the Snowflake SQL API rejects it in the requestparametersfield (HTTP 400, verified live) and is stateless per statement, so it cannot be applied per call. Set it on the Snowflake user/role default if a consumer needs Monday-first weeks.Backwards compatibility
Additive only: a new optional extra and a new module. No behavior change for existing consumers; nothing installs the extra implicitly. The
nui_lambda_shared_utilsdeprecation shim re-exports the new surface.Review
Internal: Pass 1 (bugs) clean, Pass 2 (polish) 5 items auto-fixed. External (cross-family Codex + Kimi, two rounds): no major defects; an explicit-key-vs-env precedence bug and an empty-key edge case were caught and fixed with regression tests; both reviewers confirmed credential precedence, redaction, sync/async parity, kwargs assembly, the missing-extra guard, and lazy-export wiring are correct.
Changes
Test plan
python -m build+twine checkpass with the git-pin extraSummary by CodeRabbit