Skip to content
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

Add SQL compatible margin to SUBSTR's second argument. #10

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

rubyu
Copy link
Contributor

@rubyu rubyu commented Jul 5, 2019

I found an issue that csvq's SUBSTR function is not compatible with other SQL products. The second argument of SUBSTR indicates the start position of new string and it means take from the head both when it equals to zero and 1. Is this an intentional implementation?

Repro

csvq --no-header "SELECT SUBSTR('abcdefghijklmn', 1)"

Expected

+-----------------------------+
| SUBSTR('abcdefghijklmn', 1) |
+-----------------------------+
| abcdefghijklmn              |
+-----------------------------+

Result in BigQuery

スクリーンショット 2019-07-05 14 59 18

Actual

+-----------------------------+
| SUBSTR('abcdefghijklmn', 1) |
+-----------------------------+
| bcdefghijklmn               |
+-----------------------------+

References

@codecov
Copy link

codecov bot commented Jul 5, 2019

Codecov Report

Merging #10 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   89.86%   89.87%   +<.01%     
==========================================
  Files          74       74              
  Lines       15745    15747       +2     
==========================================
+ Hits        14150    14152       +2     
  Misses       1225     1225              
  Partials      370      370
Impacted Files Coverage Δ
lib/query/function.go 99.15% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 588d222...73b6d46. Read the comment docs.

@mithrandie
Copy link
Owner

Thanks.

This program is not intended to be a full standard SQL implementation, and the behavior is intended to be easy to use for programmers of other languages.

However, I am not certain about which behavior is better.
Please wait for a few days, I will think about it.

@rubyu
Copy link
Contributor Author

rubyu commented Jul 5, 2019

I think the standard SQL spec about SUBSTR, the position=1 is indicating the head of the string, is misleading too. But csvq is good introduction to new BI users aiming to learn SQL, and they will use the other product if they develop their skills. If so, compatibility of the functions might be important to them.

Lastly,
Thank you for your great product! I introduced csvq to my colleagues yesterday and it attracted them👍

@mithrandie mithrandie changed the base branch from master to develop July 5, 2019 19:37
@mithrandie mithrandie merged commit f5b5076 into mithrandie:develop Jul 5, 2019
mithrandie added a commit that referenced this pull request Jul 6, 2019
- Add a built-in function. ([Github #10](#10))
  - String Function
      - SUBSTRING
@mithrandie
Copy link
Owner

This request was merged, but SUBSTRING function has been newly implemented as an ANSI-Compliant function, and SUBSTR function has been restored for compatibility.

SUBSTRING function is included in the version 1.11.5.
Thank you.

@rubyu rubyu deleted the fix-substr-2nd-argument branch July 6, 2019 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants