Skip to content

feat(snowflake-driver): Upgrade snowflake-sdk to ^2.4.0#10765

Merged
ovr merged 2 commits intomasterfrom
feat/snowflake-sdk-upgrade
Apr 28, 2026
Merged

feat(snowflake-driver): Upgrade snowflake-sdk to ^2.4.0#10765
ovr merged 2 commits intomasterfrom
feat/snowflake-sdk-upgrade

Conversation

@ovr
Copy link
Copy Markdown
Member

@ovr ovr commented Apr 28, 2026

Brings idle-socket pruning and session token renewal fix on long-lived connections (relevant for clientSessionKeepAlive), uniform HTTP retry behavior, faster default JSON variant parser, and CVE bumps. No API changes; getColumns() is now typed as possibly undefined, fall back to [].

ovr added 2 commits April 28, 2026 18:09
Brings idle-socket pruning and session token renewal fix on long-lived
connections (relevant for clientSessionKeepAlive), uniform HTTP retry
behavior, faster default JSON variant parser, and CVE bumps. No API
changes; getColumns() is now typed as possibly undefined, fall back to [].
@ovr ovr requested a review from a team as a code owner April 28, 2026 16:13
@github-actions github-actions Bot added driver:snowflake Issues relating to the Snowflake driver javascript Pull requests that update Javascript code data source driver labels Apr 28, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 28, 2026

Claude finished @ovr's task in 1m 57s —— View job


Code Review: Upgrade snowflake-sdk to ^2.4.0

  • Gather context and review PR diff
  • Review package.json changes
  • Review SnowflakeDriver.ts changes
  • Review yarn.lock changes
  • Check for security, performance, and testing concerns
  • Post final review

Overall: This is a clean, well-scoped upgrade. The changes are minimal and defensive. Two commits — one for the SDK bump + code adaptations, one for lockfile dedup. I have one bug-level concern and a couple of minor observations.


Bug fix: Missing break in generateHydrationMap — good catch

The added break at SnowflakeDriver.ts:935 is a correctness fix. Without it, once a matching hydrator was found for a column, the inner for loop continued iterating over subsequent hydrators. This could cause a later hydrator to overwrite the correct one if that hydrator's types array also matched (unlikely given current hydrators, but still a latent bug). Good to fix this alongside the SDK bump since it touches the same code paths.

Defensive ?? [] on getColumns() — appropriate

All four call sites now use stmt.getColumns() ?? [] (line 833, line 881, line 903, line 965). This is the correct defensive adaptation — snowflake-sdk@2.4.0 updated the return type to Column[] | undefined, and the nullish coalescing avoids a runtime crash on .map() or iteration.

Node engine bump >=20.0.0 — reasonable

Matches the rest of the ecosystem (Node 14/16 are EOL). Worth confirming this aligns with Cube's documented minimum Node version, but since many modern packages already require >=18 or >=20, this is expected.

Lockfile churn

The diff is ~2,200 lines, but that's entirely from transitive dependency updates (AWS SDK packages moved to newer minor versions when snowflake-sdk dropped its older @aws-crypto/crc32@3.0.0 dependency). The second commit (chore: align deps) correctly deduplicates residual entries. This is expected for a semver-minor bump of a package with deep AWS SDK dependencies.


Suggestions

  1. Minor — getColumns() called twice in some paths: In getTypes() and callers like memory(), stream(), and execute(), stmt.getColumns() is called once for the hydration map and once for getTypes(), which also calls stmt.getColumns() internally. This means getColumns() is invoked 2× per query execution in those paths. This is a pre-existing pattern (not introduced by this PR), so not blocking — but if you wanted to tighten it, you could pass the columns array into getTypes() instead of re-fetching. Not necessary for this PR.

  2. Testing: The existing integration tests in test/SnowflakeDriver.test.ts cover query and stream with hydration, which exercises the exact code paths modified here (getColumns() calls, generateHydrationMap, getTypes). These tests require a live Snowflake connection, so they'll validate the upgrade when run against a real instance. No new unit test is needed for the ?? [] fallback since it's a type-level defense, not a behavioral change.


Verdict

Looks good to merge. The SDK bump is well-motivated (CVE fixes, idle-socket pruning, session token renewal), the code changes are minimal and defensive, and the break fix in generateHydrationMap is a nice bonus correctness improvement. No API surface changes.

@ovr ovr merged commit d9fa353 into master Apr 28, 2026
73 of 76 checks passed
@ovr ovr deleted the feat/snowflake-sdk-upgrade branch April 28, 2026 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data source driver driver:snowflake Issues relating to the Snowflake driver javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant