-
Notifications
You must be signed in to change notification settings - Fork 690
Implement column-type-aware CSV import: defer type conversion to INSERT based on table definitions #2403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Co-authored-by: mathiasrw <[email protected]>
|
I recommend we keep csv data as string until we are inserting them. If there are col definitions, then we adhire to that (also with things like dates, int, string, float, varchar) If there are no definitions we behave like now when we do auto convert if we detect numbers (for example into tables provideda s reference data) |
…at INSERT based on table definitions Co-authored-by: mathiasrw <[email protected]>
Implemented as requested. CSV data now stays as strings until INSERT. Type conversion happens based on column definitions (STRING, INT, FLOAT, etc.) when available. Without definitions, the csvStringToNumber option controls auto-conversion for backward compatibility. Commit: 588303a |
…ECT INTO, add comprehensive edge case tests Co-authored-by: mathiasrw <[email protected]>
… complete object validation Co-authored-by: mathiasrw <[email protected]>
src/15utility.js
Outdated
| */ | ||
| var reTypeConverter = { | ||
| str: /string|char$|text/i, | ||
| int: /^int|int$|^smallint$|^bigint$|^tinyint$/i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small int, big int, tinyint are all coverd by int$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified int regex to /int$/i which covers all int types. Commit: 689a7ca
src/15utility.js
Outdated
| // If no type specified, try to auto-convert if it looks like a number | ||
| if (!dbtypeid) { | ||
| if (alasql.options.csvStringToNumber && typeof value === 'string' && value.length > 0) { | ||
| var trimmed = value.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid trim here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed trim() call, now using direct comparison value == +value. Commit: 689a7ca
src/15utility.js
Outdated
| var lower = value.toLowerCase(); | ||
| return lower === 'true' || lower === '1' || lower === 'yes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please use a regex here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced toLowerCase + string comparisons with single regex: /^(true|1|yes)$/i.test(value). Commit: 689a7ca
…or boolean matching Co-authored-by: mathiasrw <[email protected]>
Summary
Applied maintainer feedback to further optimize the type converter utility.
Changes Made
Type Converter Optimizations (src/15utility.js)
Simplified int regex (Line 1355)
/^int|int$|^smallint$|^bigint$|^tinyint$/i/int$/iint$patternRemoved trim() call (Line 1376)
var trimmed = value.trim(); if (trimmed == +trimmed)if (value == +value)Boolean conversion using regex (Lines 1396-1397)
var lower = value.toLowerCase(); return lower === 'true' || lower === '1' || lower === 'yes';return /^(true|1|yes)$/i.test(value);Testing Results
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.