-
Notifications
You must be signed in to change notification settings - Fork 690
Fix NULL comparison in IN and NOT IN operators to follow SQL semantics #2315
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?
Fix NULL comparison in IN and NOT IN operators to follow SQL semantics #2315
Conversation
|
@copilot |
Co-authored-by: mathiasrw <[email protected]>
Co-authored-by: mathiasrw <[email protected]>
I've completed the work on fixing NULL comparison in IN and NOT IN operators to follow SQL semantics. Changes implemented:
SQL semantics now enforced:
Testing:
Commits: 13261b4 (context fix), 06a51a1 (main implementation) |
…0b-4cd2-bdb8-314a0de54479
|
Found 1 test failure on Blacksmith runners: Failure
|
|
325 passing (458ms)
[ should loosely deep-equal [] |
…g logic Co-authored-by: mathiasrw <[email protected]>
|
... The test failure was caused by a merge conflict. The develop branch introduced caching logic that was overwriting the SQL-compliant NULL handling. I've now integrated the NULL handling with both the cached and uncached code paths. Fixed in commit 7f5f328:
Test results:
|
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 seems like way way too much code to make sure we return something correct for in and not in operatoins. Please rethink how this could be implemented in a much less invasive way.
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.
I've simplified the implementation significantly. The new approach:
Changes made (commit 386672b):
- Added two simple utility functions:
sqlInCheckandsqlNotInCheck(37 lines in src/15utility.js) - Replaced complex inline arrow functions with calls to these utilities
- Net reduction of 95 lines of code (added 37, removed 132)
Result:
- Much cleaner and less invasive
- Integrates well with existing caching logic
- All 1896 tests passing
- Original issue verified fixed
The utilities handle SQL NULL semantics in a centralized, maintainable way rather than duplicating complex logic inline.
Co-authored-by: mathiasrw <[email protected]>
Thank you for the time you are putting into AlaSQL!
Description
This PR fixes issue #1414 where AlaSQL's IN and NOT IN operators were not following SQL semantics when handling NULL values. The implementation uses utility functions to correctly enforce SQL three-valued logic (TRUE, FALSE, UNKNOWN) for NULL comparisons, providing a clean and maintainable solution.
Problem
Previously, when a subquery or list contained NULL values:
Solution
Added two utility functions and modified
src/50expression.jsto use them for SQL-compliant NULL handling:Changes Made
sqlInCheck: Handles IN operator with SQL-compliant NULL semanticssqlNotInCheck: Handles NOT IN operator with SQL-compliant NULL semanticsTesting
The implementation follows SQL standard behavior where NULL comparisons produce UNKNOWN, which is treated as false in WHERE clause filtering. The utility-based approach provides a clean, maintainable solution that integrates seamlessly with existing caching optimization while ensuring SQL compliance.
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.