feat: add toLower/toUpper support, fix type coercion error #82
Conversation
- 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.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ChunxuTang
left a comment
ChunxuTang
left a comment
There was a problem hiding this comment.
@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.
| assert_eq!(name, "expr", "Arithmetic should use generic name"); | ||
| } | ||
|
|
||
| // ======================================================================== |
There was a problem hiding this comment.
We have a section "Unit tests for to_df_value_expr()" in the unit tests. Could you append your string function tests directly to that section? You don't need to create a new section here.
| // ============================================================================ | ||
| // String Function Tests (toLower, toUpper) | ||
| // ============================================================================ |
There was a problem hiding this comment.
Similar to the previous comment, there has been a section called "String Operator Tests", so you don't need to create a new section. Actually, the new tests have already been in the "String Operator Tests" section.
|
|
||
| // ============================================================================ | ||
| // String Function Tests (toLower, toUpper) | ||
| // ============================================================================ |
There was a problem hiding this comment.
| // ============================================================================ | |
| // String Function Tests (toLower, toUpper) | |
| // ============================================================================ |
3 participants
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