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 LIKE operator support #241

Merged
merged 12 commits into from
Oct 25, 2020
Merged

Conversation

tdakkota
Copy link
Contributor

@tdakkota tdakkota commented Oct 8, 2020

Adds LIKE operator support.
Implementation is based on CockroachDB implementation.

Updates #52.

@tdakkota tdakkota changed the title WIP: add LIKE operator support Add LIKE operator support Oct 8, 2020
@tdakkota tdakkota marked this pull request as draft October 8, 2020 15:42
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #241 into master will increase coverage by 0.13%.
The diff coverage is 76.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
+ Coverage   61.34%   61.47%   +0.13%     
==========================================
  Files          68       70       +2     
  Lines        6327     6429     +102     
==========================================
+ Hits         3881     3952      +71     
- Misses       1933     1960      +27     
- Partials      513      517       +4     
Impacted Files Coverage Δ
sql/query/expr/like.go 0.00% <0.00%> (ø)
sql/scanner/token.go 22.22% <ø> (ø)
sql/parser/expr.go 75.79% <44.44%> (-2.78%) ⬇️
sql/query/expr/comparison.go 25.19% <50.00%> (-1.24%) ⬇️
sql/query/expr/expr.go 33.33% <77.77%> (+14.81%) ⬆️
sql/query/glob/like.go 100.00% <100.00%> (ø)
engine/memoryengine/store.go 91.96% <0.00%> (-1.79%) ⬇️
database/config.go 68.59% <0.00%> (ø)
... and 4 more

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 dfeafd1...ddcd088. Read the comment docs.

sql/query/expr/like.go Outdated Show resolved Hide resolved
database/database.go Outdated Show resolved Hide resolved
sql/query/expr/comparison.go Outdated Show resolved Hide resolved
sql/query/expr/like_test.go Outdated Show resolved Hide resolved
// Case 4.
var r rune
r, s = readRune(s)
if !equalFold(p, r) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use direct comparison?

I ran this query on different DBs.

SELECT 'abc' LIKE 'ABC';

Results:

  • PostgreSQL 12.3 returns false.
  • SQLite 3.27.2 returns 1 (true).
  • MySQL 5.7.12 returns 1 (true).
  • Oracle Database 11g returns false (Query: SELECT * FROM DUAL WHERE 'abc' like 'ABC', should be non-empty if true).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stick with what MySQL and SQLite do.

Copy link
Contributor

@tie tie Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, ideally we should be comparing grapheme clusters using e.g. github.com/clipperhouse/uax29/graphemes and golang.org/x/text/collate.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we work on collation support in a separate PR? This one is already pretty big and since Genji is not stable yet we can give ourselves time to improve before locking things up.
Unless you think adding it would not take long?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQLite performs (simple) case folding for character comparison with LIKE operator, and since that’s what our implementation is based on, I think it’s reasonable to follow this behavior too.

A proper Unicode support would definitely take some time to implement given the current state of Unicode support in Go (scattered across third-party libraries with different Unicode versions, and each embeds their own character database copy).

sql/query/glob/like.go Outdated Show resolved Hide resolved
sql/query/glob/like.go Outdated Show resolved Hide resolved
sql/query/glob/like.go Outdated Show resolved Hide resolved
* Increase test and comment coverage
* Add more test cases
* Add doc comment for glob package
* Fix escaping end with non-empty input
* Update like.go file header comment
Copy link
Collaborator

@asdine asdine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That's some awesome work 👏🏼
The PR is still in draft, what do you think is missing to finalize it?

sql/query/expr/like.go Outdated Show resolved Hide resolved
// May you share freely, never taking more than you give.
//
// This is an optimized Go port of the SQLite’s icuLikeCompare routine using backtracking.
// See https://sqlite.org/src/file?name=ext%2Ficu%2Ficu.c&ln=117-195&ci=54b54f02c66c5aea
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for quoting the source 🙏🏼

// Case 4.
var r rune
r, s = readRune(s)
if !equalFold(p, r) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we work on collation support in a separate PR? This one is already pretty big and since Genji is not stable yet we can give ourselves time to improve before locking things up.
Unless you think adding it would not take long?

@tdakkota tdakkota marked this pull request as ready for review October 18, 2020 00:08
@tie
Copy link
Contributor

tie commented Oct 22, 2020

@tdakkota can you please mark outdated discussions as resolved?

@asdine
Copy link
Collaborator

asdine commented Oct 25, 2020

@tdakkota Could you give us some input on what's missing?

@tie
Copy link
Contributor

tie commented Oct 25, 2020

@asdine I think it’s ready for review, otherwise the PR would be still a draft.

@tdakkota marked this pull request as ready for review 7 days ago

Copy link
Collaborator

@asdine asdine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@asdine asdine added this to the v0.9.0 milestone Oct 25, 2020
@asdine asdine merged commit a8b144b into chaisql:master Oct 25, 2020
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.

4 participants