feat: add support to DECIMAL duckdb column type - #3341
Conversation
There was a problem hiding this comment.
Pull request overview
Adds DuckDB DECIMAL column handling for Kepler ingestion by coercing DECIMAL-typed columns to DOUBLE in the generated SELECT statement, along with a unit test covering common DECIMAL type strings.
Changes:
- Update
castDuckDBTypesForKeplerto castDECIMAL*columns toDOUBLE. - Add a Tape test ensuring DECIMAL columns are cast as expected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/node/utils/duckdb-utils-test.js | Adds coverage for DECIMAL type casting behavior in generated SQL. |
| src/duckdb/src/table/duckdb-table-utils.ts | Extends type casting logic to include DECIMAL* alongside large integer types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type === 'UBIGINT' || | ||
| type === 'HUGEINT' || | ||
| type === 'UHUGEINT' || | ||
| type.startsWith('DECIMAL')) |
There was a problem hiding this comment.
type.startsWith('DECIMAL') can throw at runtime if type is undefined/non-string (e.g., getDuckDBColumnTypes assigns columnTypes?.get(i) with optional chaining). Consider normalizing/guarding first (e.g., const t = typeof type === 'string' ? type : '') before calling startsWith.
| type.startsWith('DECIMAL')) | |
| (typeof type === 'string' && type.startsWith('DECIMAL'))) |
| } else if ( | ||
| options.bigIntToDouble && | ||
| (type === 'BIGINT' || type === 'UBIGINT' || type === 'HUGEINT' || type === 'UHUGEINT') | ||
| (type === 'BIGINT' || | ||
| type === 'UBIGINT' || | ||
| type === 'HUGEINT' || | ||
| type === 'UHUGEINT' || | ||
| type.startsWith('DECIMAL')) | ||
| ) { | ||
| // Cast 64-bit and larger integer types to DOUBLE to avoid BigInt in JS | ||
| // Cast 64-bit and larger integer types and DECIMAL to DOUBLE to avoid BigInt in JS | ||
| return `CAST(${quotedColumnName} AS DOUBLE) as ${quotedColumnName}`; |
There was a problem hiding this comment.
bigIntToDouble now also controls DECIMAL casting, which is a behavioral/API mismatch with the option name (and could surprise callers who disable BigInt casting but still want DECIMAL coerced for Kepler compatibility). Consider introducing a separate option (e.g., decimalToDouble) or renaming/generalizing the option while keeping backward compatibility.
There was a problem hiding this comment.
Good point. But I just want to make a simple change
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ISSUE: DECIMAL duckdb columns have been parsed as
stringtype in kepler