Skip to content

Merge development (with our current Datawarehouse code) into the AWS branch.#232

Open
jgrantr wants to merge 78 commits into
feature/aws-sdk-v3-againfrom
development
Open

Merge development (with our current Datawarehouse code) into the AWS branch.#232
jgrantr wants to merge 78 commits into
feature/aws-sdk-v3-againfrom
development

Conversation

@jgrantr

@jgrantr jgrantr commented Nov 3, 2025

Copy link
Copy Markdown

Note

High Risk
Large changes to fact/dimension ingest SQL, delete semantics, and Redshift COPY/S3 paths affect production warehouse data correctness; misconfiguration of new flags can call process.exit() at startup.

Overview
Merges current data warehouse work into the AWS branch: Redshift-oriented staging and load paths, corrected delete/update merging, and tooling to publish the postgres connector on GitHub release.

Postgres / Redshift DW (dwconnect.js, connect.js) adds a full streamToTableFromS3 path (CSV to S3, then COPY with optional IAM role and S3 cleanup). When config.version === 'redshift', fact/dimension imports use Redshift-style staging (DISTSTYLE ALL, sort keys from v_dist_sort_key), skip ANALYZE where inappropriate, and can use delete + reinsert instead of heavy updates. New config knobs include hashedSurrogateKeys (farmFingerPrint64, bigint SKs), bypassSlowlyChangingDimensions, and deleteFlushCount; incompatible combinations of hashed keys and SCD exit the process. Fact loads no longer push __leo_delete__ rows into staging (deletes go through the delete handler first). Temp table drops are IF EXISTS with errors logged instead of failing the run.

Common DW pipeline fixes combine.js so consecutive rows for the same key treat either side as a delete correctly (a later update cancels an earlier delete). load.js rewires the validation error sink to ls.process / ls.toLeo and records payload.source_id on failed events.

Also adds .github/workflows/publish-postgres.yml (release-triggered npm publish from ./postgres), .gitignore entries, and version bumps for common, entity-table, and postgres packages (lockfiles updated).

Reviewed by Cursor Bugbot for commit 9738116. Bugbot is set up for automated code reviews on this repo. Configure here.

@jgrantr jgrantr requested a review from czirker November 3, 2025 17:42
@ch-snyk-sa

ch-snyk-sa commented Nov 3, 2025

Copy link
Copy Markdown

Snyk checks have failed. 31 issues have been found so far.

Status Scan Engine Critical High Medium Low Total (31)
Open Source Security 0 24 6 1 31 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

* @param error {string}
*/
function handleFailedValidation (ID, source, eventObj, error) {
function handleFailedValidation(ID, source, eventObj, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Incorrect Case-Sensitive Property Check Breaks Error Stream Initialization

Same incorrect property check as above. The code checks !errorStream.Writable but should check !errorStream.writable (lowercase w). This will cause the error stream initialization logic to be executed every time handleFailedValidation is called, potentially creating multiple pipelines for the same error stream.

Fix in Cursor Fix in Web

feat: add source id to dim error event
Comment thread postgres/lib/dwconnect.js
useSurrogateDateKeys: true,
}, columnConfig || {});

console.log(`has delete_fix in-place`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Debug console.log left in production code

Medium Severity

A console.log(has delete_fix in-place) statement is left in the module initialization path of dwconnect.js. This will print to stdout every time the module is loaded, which is noisy in production and clearly debug/development code that wasn't intended to be committed.

Fix in Cursor Fix in Web

Comment thread postgres/lib/dwconnect.js
Comment thread postgres/lib/dwconnect.js
…atalog Delta

DPLAT-466: https://chb.atlassian.net/browse/DPLAT-466

## Summary

New `connectors/datalake/` library package — Deliverable 1 of the Redshift→Databricks migration. Adapts the Redshift connector (leo-connector-postgres) to write Delta tables via S3 staging + DML MERGE, using the same `dw_fields` JSON config as the existing pipeline.

**Scope:** library package only. The `offload_to_datalake.js` bot that calls it (Deliverable 1 runner) is a follow-on task. Deliverable 2 (VARIANT event tables) and Deliverable 3 (retabulation library) are out of scope.


## What was built

**Connection layer (`lib/connect.js`)**
- `DBSQLClient` + `generic-pool` of sessions with validate-on-borrow, destroy-on-error
- Memoized connect (single `connectPromise`) so parallel invocations don't each fetch OAuth
- `STATEMENT_TIMEOUT` session param (default 600s, floored 5s, capped 1800s) — runaway guard for serverless warehouse
- `isConnectionError` classifier drives pool destroy-on-error vs release-healthy
- S3 staging: `streamToTableFromS3` pipes enriched records through `fast-csv` → `leo-streams.toS3`; `buildStagingSelect` inlines `read_files(...)` directly into MIN and MERGE queries (no session-scoped temp views — pool sessions are independent acquires)

**Loader (`lib/dwconnect.js`)**
- `importFact`: stage → `SELECT MIN` prune → `MERGE INTO` Delta; surrogate keys computed in Node.js via `farmhash-modern`, not SQL
- `importDimension`: bypassSCD upsert (all production configs have `bypassSlowlyChangingDimensions=true`); sentinel values (`_current=true`, `_startdate='1900-01-01'`, `_enddate='9999-01-01'`) match postgres bypass path
- `linkDimensions`: FK surrogate-key values pre-computed in `importFact`/`importDimension` enrichFns (`buildFkEnrichers`); `linkDimensions` itself is a no-op — Databricks has no `FARMFINGERPRINT64` SQL function
- `insertMissingDimensions`: no-op under `hashedSurrogateKeys=true` — any FK reference computes the same hash and merges correctly when the dim row arrives
- `changeTableStructure`: CREATE TABLE for new tables, ALTER TABLE ADD COLUMN for new columns; `_rescued_data STRING` added to every table for malformed-record observability
- Bounded idempotent retry (`withRetry`) around MIN, MERGE, flushDeletes — connection-error-only; query-class errors propagate immediately
- `dropTempTables`: no-op (S3 files cleaned up inline after MERGE)

**Timestamp handling**
- All columns map to `TIMESTAMP_NTZ` to preserve legacy no-TZ wall-clock semantics bit-for-bit during coexistence
- Session params: `timezone=UTC`, `infer_timestamp_ntz_type=true`
- Audit timestamp and payload values strip `Z`/offset before staging CSV write (empirically confirmed by `test/integration/timezone.test.js`)

**SQL generation (`lib/sql.js`)**
- `createTable`, `changeTableStructure`, `mergeFact`, `mergeDim` — Databricks-dialect DDL and DML
- `varchar(n)` → `STRING`; `SORTKEY`/`DISTKEY` → `CLUSTER BY`

**CI / publish**
- `publish-datalake.yml` — release-triggered GitHub Actions workflow: `npm ci` → `npm run test:coverage` → coverage summary in step output → `npm publish`

**Key dependencies**
- `@databricks/sql@^1.16.0`, `leo-sdk@7.1.12` (AWS SDK v3 stack), `leo-connector-common@4.0.13-rc` (v5 was released from an unofficial awsv3 branch — held at stable rc), `leo-logger@1.0.7`

## Tests

**Unit (249 tests, `npm test`):**
- `connect.test.js` — connection pool, retry, `isConnectionError`, S3 staging contract
- `dwconnect.test.js` — `changeTableStructure`, `flushDeletes`, error propagation
- `importFact.test.js` — CSV serialization, staging orchestration, MIN/MERGE paths
- `importDimension.test.js` — dim upsert, sentinel values, delete-marker filtering
- `sql.test.js` — DDL/DML generation, type mapping
- `surrogate_key.test.js` — farmhash contract
- `load.smoke.test.js` — end-to-end wiring: 100 events through real `load.js` → real `dwconnect.js` with `connect.js`/S3 stubbed

**Coverage (`npm run test:coverage`):** 95% statements, 86% branches (nyc, scoped to `index.js` + `lib/**/*.js`)

**Integration (`npm run test:int`):**
- Round-trip, idempotency, schema evolution, timezone, session-reuse against `de_cup_dev_us` Databricks workspace

## What's not in this PR

- `offload_to_datalake.js` bot in `general/` — next task; library is ready
- CI catalog cloning workflows — blocked on OIDC/Secrets Manager grants
- Redshift parity validation script — blocked on prod fixture capture
- Deliverable 2 (VARIANT tables), Deliverable 3 (retabulation library)

## Test plan

- [x] `npm run lint` — clean
- [x] `npm test` — 249 passing
- [x] `npm run test:coverage` — 95% statements, 86% branches
- [x] Unit tests run fully offline
- [x] `npm run test:int` — requires `de_cup_dev_us` credentials (local dev only)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **High Risk**
> Adds a large new warehouse write path where MERGE semantics, surrogate-key parity, and TIMESTAMP_NTZ staging directly affect data correctness; mistakes would corrupt Delta tables during coexistence with Redshift even though the package is additive until a bot adopts it.
> 
> **Overview**
> Introduces **`connectors/datalake/`** (`leo-connector-datalake`) as a new npm library for the Redshift→Databricks migration: same `dw_fields` + `leo-connector-common/datawarehouse/load.js` contract as the postgres offload path, but targets Unity Catalog Delta via **S3 pipe-CSV staging** and **`MERGE INTO`**.
> 
> **Connection & staging (`lib/connect.js`)** wires `@databricks/sql` with a **bounded `generic-pool` session pool**, memoized OAuth connect, `STATEMENT_TIMEOUT`, and **`buildStagingSelect`** that inlines `read_files(...)` into MIN/MERGE (no session temp views). Staging uses `fast-csv` + `leo-sdk.streams.toS3`; **`TIMESTAMP_NTZ`** payload values strip `Z`/offsets before CSV write for Redshift wall-clock parity.
> 
> **Loader (`lib/dwconnect.js`)** implements **`importFact`** / **`importDimension`** (bypass-SCD dim sentinels), **`changeTableStructure`**, delete flushes, S3 cleanup, and **`withRetry`** on connection errors. Surrogate keys and dimension FKs are computed in Node (`farmhash-modern`, `buildFkEnrichers`); **`linkDimensions`** / **`insertMissingDimensions`** are intentional no-ops matching the hashed-SK model. **`lib/sql.js`** emits Delta DDL/DML (`CLUSTER BY`, `_rescued_data`); **`index.js`** extends `leo-connector-common/base` like sibling connectors.
> 
> Tooling adds Node 22, eslint, unit + integration test scripts and extensive **`docs/`** (build plan, porting decisions). Root **`.gitignore`** gains `archive` and `.DS_Store`. Production bot wiring (`offload_to_datalake.js`) and formal Redshift equivalence remain follow-ons per docs.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 0ce20c8. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread datalake/lib/dwconnect.js Outdated
…sts (#244)

Empty result sets produce fields=[] which crashes mapResults in the parent
(common/dol.js) when last is still null. processJoinQuery already has this
guard; this override adds the same check to processDomainQuery.

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Comment thread .github/workflows/publish-datalake.yml Outdated
pmogren and others added 4 commits June 26, 2026 13:43
… config (#245)

DATABRICKS_CONFIG_PROFILE is now required — there is no default profile name.
getConfig() throws when host+auth are missing (fail-fast, not skip).
Updated test file comments, README, CLAUDE.md, and statement_timeout_repro.sh.

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… process (#246)

* chore(datalake): remove publish workflow — no leoinsights npm access to use it

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* docs(datalake): document manual npm publish process and gate in CLAUDE.md

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…pping and storage clause (#247)

* refactor(datalake): make sql.js type mapping and storage clause injectable for multi-target support

createTable, alterAddColumn, alterColumnType now accept optional mapTypeFn
(default: Databricks mapType) and createTable accepts optional storageClause
(default: 'USING DELTA\nCLUSTER BY AUTO'). TYPE_MAP exported for reuse.
Existing callers unchanged — all params have defaults.

Also documents the multi-target design intent in project_principles.md (P0)
and adds a Never rule to CLAUDE.md against creating a separate connector
package for a new target.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* docs(datalake): document known target-coupling seams in P0

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* chore(datalake): drop leo-streams dependency; remove dead test stubs

leo-streams was listed as a dependency but never imported — connect.js and
dwconnect.js both use leo-sdk.streams. The proxyquire stubs in dwconnect.test.js
were stubbing a module that dwconnect.js does not require.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): align _rescued_data type token between createTable and alterAddColumn

dwconnect.js passes 'string' to alterAddColumn for _rescued_data; createTable
was using 'varchar'. Both map to STRING with the default mapType but diverge
with a custom mapTypeFn. Add 'string' to TYPE_MAP explicitly and align
createTable to use the same token.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): add varchar to TYPE_MAP explicitly

Plain 'varchar' already hit the startsWith branch, but a target extending
TYPE_MAP wouldn't inherit that behavior. Making it explicit keeps TYPE_MAP
self-contained for custom mapTypeFn implementations.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): add Redshift character type aliases to TYPE_MAP

char, character, nchar, nvarchar, bpchar, text all map to STRING.
Also adds startsWith coverage for char(n) and nvarchar(n) sized variants,
matching the existing varchar(n) branch.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): throw on TIME/TIMETZ in mapType — no native Databricks equivalent

Silently degrading to STRING would lose time semantics without any indication.
Covers time, timetz, and ANSI long-form aliases.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): add ANSI long-form timestamp aliases to TYPE_MAP

'timestamp without time zone' → TIMESTAMP_NTZ
'timestamp with time zone'    → TIMESTAMP

Exact TYPE_MAP entries are the clean solution — startsWith cannot distinguish
these since 'timestamp without time zone' contains 'with'.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): throw on any unrecognized or missing type in mapType

Unknown types silently becoming STRING could corrupt data — fail at
schema-change time instead. null/empty/whitespace also throw rather
than returning STRING. TIME/TIMETZ keep their specific error message.
TYPE_MAP is now the sole authoritative contract.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): complete Redshift type coverage in mapType

Numeric: smallint/int2→SMALLINT, int4→INT, int8→BIGINT,
  real/float4→FLOAT, float8/double precision→DOUBLE,
  numeric(p,s)→DECIMAL(p,s), numeric→DECIMAL(18,0)
Fix: float→DOUBLE (Redshift FLOAT = DOUBLE PRECISION, not REAL)
Binary: varbyte/varbinary/binary varying + sized variants → BINARY

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): throw on SUPER/VARIANT — staging pipeline changes required

CSV round-trip serializes nested JSON as STRING; read_files needs explicit
TRY_PARSE_JSON; MERGE COALESCE semantics differ. Points to Deliverable #2.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(datalake): remove cross-repo doc reference from SUPER/VARIANT error message

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…eering/rstreams-connector-datalake (#248)

The datalake connector is being extracted to a standalone chub-engineering
repo where it will be published via GitHub Package Registry as
@chub-engineering/leo-connector-datalake.

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.

There are 5 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9738116. Configure here.

Comment thread postgres/lib/dwconnect.js
tasks.push(done => {
connection.query(`UPDATE ${qualifiedStagingTable}
SET ${columnConfig._auditdate} = ${dwClient.auditdate},
${sk} = farmFingerPrint64(${nk.map(id => `${id}`).join(`|| '-' ||`)}); `, done);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bypass SCD wrong ingest path

High Severity

When bypassSlowlyChangingDimensions is true but hashedSurrogateKeys is false, importDimension still runs the delete/reinsert path that sets surrogate keys with farmFingerPrint64, which does not match the integer SCD path and can corrupt dimension history.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9738116. Configure here.

Comment thread postgres/lib/dwconnect.js
sortKey = results[0].sortkey;
sortKeyType = results[0].sortkeytype;
};
done();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing row guard on view

Medium Severity

Redshift sort-key setup reads results[0] without checking that v_dist_sort_key returned a row, so a missing view entry causes a TypeError and aborts the ingest for that table.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9738116. Configure here.

Comment thread postgres/lib/dwconnect.js
useSurrogateDateKeys: true,
}, columnConfig || {});

console.log(`has delete_fix in-place`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Debug log left in module

Low Severity

A console.log announcing delete-fix status runs every time the warehouse client module loads, adding noise in production logs and Lambda output.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9738116. Configure here.

Comment thread postgres/lib/connect.js
stream.once('drain', done);
} else {
done(null);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

S3 stream init race

High Severity

streamToTableFromS3 can process rows before the S3 upload stream exists: a single pending callback is overwritten if multiple rows arrive early, and the write step calls stream.write when stream is still undefined, causing lost rows, hangs, or runtime errors.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9738116. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants