-
Notifications
You must be signed in to change notification settings - Fork 690
Fix NULL value handling in SELECT * INTO SQL() #2196
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 <1063454+mathiasrw@users.noreply.github.com>
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
|
@copilot Im not sure we are meant to return null. I think alasql is having a "NULL is represented by undefined" policy. |
|
@copilot continue |
The fix is complete and working correctly. The implementation properly handles alasql's "NULL is undefined" policy:
All tests pass (493/494, 1 unrelated HTTPS timeout). Commit cc04534. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Run yarn test-format-all |
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
| } else { | ||
| if (typeof val == 'string') { | ||
| val = "'" + escapeqq(val) + "'"; | ||
| if (col.typeid) { |
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.
This look like an ideal situation for a switch statement
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.
Refactored to use switch statement. Commit 9ecbe42.
test/test042_null_issue.js
Outdated
| // null and undefined should become NULL | ||
| assert(res.indexOf('NULL') > -1, 'null and undefined should become NULL'); | ||
|
|
||
| done(); |
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 only have function (done) and use the done() if we are doing async work. If not we just do ()=>{
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.
Changed to arrow functions () => {} and removed done() calls since tests are synchronous. Commit 9ecbe42.
|
@copilot Please see the comments |
Both code review comments addressed in commit 9ecbe42:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
test/test042_null_issue.js
Outdated
| ]; | ||
| var res = alasql('SELECT * INTO SQL({tableid:"test_table"}) FROM ?', [data]); | ||
|
|
||
| console.log('Generated SQL:'); |
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 never write things from the test files.
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 all console.log statements from the test file. Commit ec64f1c.
test/test042_null_issue.js
Outdated
| 'Third row should have NULL for a, c, and d' | ||
| ); | ||
|
|
||
| done(); |
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 get rid of done() for things that are not async
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.
Changed to arrow functions and removed done() for synchronous tests. Commit ec64f1c.
test/test042_null_issue.js
Outdated
| assert(res.indexOf('(2,NULL,4,NULL)') > -1, 'Second row should have NULL for b and d'); | ||
|
|
||
| // Third row: a, c, and d should be NULL | ||
| assert( |
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 just do a deepEqual with the expected value instead of all this indexOf
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 indexOf checks with assert.deepEqual comparing against expected SQL output. Commit ec64f1c.
|
@copilot did you see the comments? |
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
All three comments addressed in commit ec64f1c:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Fix NULL values in SELECT * INTO SQL() statement
Problem: When using
SELECT * INTO SQL(), null or undefined values are output as empty strings (resulting in consecutive commas like,,) instead of the SQLNULLkeyword.Solution: Modified the SQL generation logic in
src/830into.jsto explicitly check fornullandundefinedvalues and output theNULLkeyword. Refactored type checking to use a switch statement for better code maintainability.Changes Made:
,,instead ofNULLTest Results:
Before Fix:
Notice the
,,which is invalid SQL.After Fix:
Now properly outputs
NULLinstead of,,.Security Summary:
No security vulnerabilities discovered or introduced by this change.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.