chaisql / chai

Modern embedded SQL database
MIT License
1.56k stars 95 forks source link

Add LEN builtin function #482

Closed Darkclainer closed 2 years ago

Darkclainer commented 2 years ago

Fixes #474

I have few concerns about this PR:

codecov[bot] commented 2 years ago

Codecov Report

Merging #482 (a35d713) into main (487d4b4) will decrease coverage by 0.03%. The diff coverage is 61.76%.

@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
- Coverage   79.15%   79.12%   -0.04%     
==========================================
  Files         128      128              
  Lines       10748    10782      +34     
==========================================
+ Hits         8508     8531      +23     
- Misses       1536     1545       +9     
- Partials      704      706       +2     
Impacted Files Coverage Δ
internal/expr/functions/builtins.go 63.40% <61.76%> (-0.20%) :arrow_down:
internal/stringutil/stringutil.go 73.52% <0.00%> (+5.88%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Darkclainer commented 2 years ago

Thank you for review, @asdine!

Actually, you can add tests in the sqltests directory. It is used to test complete sql queries and declare the expected output. Here is an example.

Here is what I suggest:

  • add a file in sqltests/SELECT/len.sql
  • move your tests in that file and adapt them to use a complete SELECT statement
  • add the missing examples defined in the issue

Did it, now it seems more complete.

Maybe error message can be improved.

Do you have an example?

I do not. Actually, for me, any error in patched code seems like serious bug and should never be triggered, except for the first Eval that probably can fail. But may be you know situations when such cases can occur.