-
Notifications
You must be signed in to change notification settings - Fork 12
feat: add toLower/toUpper support, fix type coercion error #82
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
feat: add toLower/toUpper support, fix type coercion error #82
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ChunxuTang
left a comment
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.
@youssef-tharwat Thanks so much for the fix!
I just left very minor comments for the tests. The fix generally looks good to me.
Meanwhile, could you fix the style check error of your PR? Feel free to tag me when you want me to review again.
|
@ChunxuTang all done :) |
ChunxuTang
left a comment
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.
LGTM! Thanks for the contribution.
|
@youssef-tharwat One minor thing: there's a style check error reported by the GitHub workflows. Could you run |
- Add support for toLower/toUpper (Cypher) and lower/upper (SQL) string functions - Change fallback for unsupported functions from lit(0) to ScalarValue::Null to prevent type coercion errors in any context (string or numeric) - Add unit tests for new string functions - Add integration tests including exact bug reproduction Fixes type coercion error when using toLower(s.name) CONTAINS 'x' with integer columns in RETURN clause.
Co-Authored-By: Claude <noreply@anthropic.com>
6984153 to
2ff66f0
Compare
Summary
toLower/toUpper(Cypher) andlower/upper(SQL) string functionslit(0)toScalarValue::Nullto prevent type coercion errorsProblem
When using
toLower(s.name) CONTAINS 'x'with integer columns in the RETURN clause, a type coercion error occurred:This happened because
toLowerwas not implemented and fell through to the default case which returnedlit(0)(Int32). When combined withCONTAINS(translated to LIKE), DataFusion couldn't coerce Int32 to Utf8.Solution
toLower/toUpperusing DataFusion'slower()/upper()functionslit(0)toScalarValue::Null, which coerces to any type in SQL semanticsTest plan
toLower,toUpper,lower,upperfunctions