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.

Sign up for GitHub

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

Jump to bottom

Support contains operator for MySQL #2042

Merged
merged 4 commits into from
Jun 7, 2021
Merged

Support contains operator for MySQL #2042

merged 4 commits into from
Jun 7, 2021

Conversation

nakabonne
Copy link
Member

@nakabonne nakabonne commented Jun 3, 2021 •
edited
Loading

What this PR does / why we need it:
Indexing on a JSON column didn't go well. Though I'm in the middle of trying to do it, the query works fine without the index.
Let me merge it to support the environment deletion feature in the next version. It's no problem to add index after that version.

Which issue(s) this PR fixes:

Fixes #2033

Does this PR introduce a user-facing change?:

NONE

@pipecd-bot pipecd-bot requested review from khanhtc1202 and nghialv June 3, 2021 07:06
@nakabonne nakabonne mentioned this pull request Jun 3, 2021
Closed
@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.32%. This pull request decreases coverage by -0.00%.

File Function Base Head Diff
pkg/datastore/mysql/query.go buildWhereClause 100.00% 90.00% -10.00%

khanhtc1202
khanhtc1202 reviewed Jun 3, 2021
View reviewed changes
pkg/datastore/mysql/query.go Outdated
@@ -74,6 +74,8 @@ func buildWhereClause(filters []datastore.ListFilter) string {
// Make string of (?,...) which contains the number of `?` equal to the element number of filter.Value
valLength := reflect.ValueOf(filter.Value).Len()
conds[i] = fmt.Sprintf("%s %s (?%s)", filter.Field, filter.Operator, strings.Repeat(",?", valLength-1))
case "JSON_CONTAINS":
conds[i] = fmt.Sprintf("%s(%s, '%q', '$')", filter.Operator, filter.Field, filter.Value)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't pass the filter.Value here, it will automatically be passed by here, what we do here is to make the string query that mark every value on the query as ? character. 😄

@nakabonne
Copy link
Member Author

nakabonne commented Jun 4, 2021 •
edited
Loading

hmm, it's killing me that it takes a bit long time to reflect on the local environment every time I change the code, and plus i wonder if it really reflects the changes. i'm in the middle of looking up what's going on.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.32%. This pull request decreases coverage by -0.00%.

File Function Base Head Diff
pkg/datastore/mysql/query.go buildWhereClause 100.00% 90.00% -10.00%

@nakabonne
Copy link
Member Author

and plus i wonder if it really reflects the changes.

Okay, first of all, for embed files, it turns out they don't get updated with make push, it requires make build to re-generate code.
Besides, to apply source code changes, we seem to have to make kind-down every time before make push, which takes more than 20min or more on my laptop (Intel(R) Core(TM) i7-8559U CPU @ 2.70GHz with 16GB of RAM on macOS 10.15.7) 😭

@khanhtc1202
Copy link
Member

Besides, to apply source code changes, we seem to have to make kind-down every time before make push

I didn't know that 👀 I'm using docker4mac k8s cluster instead of kind on my local 😂

@nakabonne nakabonne marked this pull request as ready for review June 7, 2021 02:38
@nakabonne
Copy link
Member Author

I didn't know that 👀 I'm using docker4mac k8s cluster instead of kind on my local 😂

I see. Let me talk about it in the near future.

@nakabonne
Copy link
Member Author

Indexing on a JSON column didn't go well. Though I'm in the middle of trying to do it, the query works fine without the index.
Let me merge it to support the environment deletion feature in the next version. It's no problem to add index after that version.

@nakabonne
Copy link
Member Author

Sorry, hold on
/hold

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.32%. This pull request decreases coverage by -0.00%.

File Function Base Head Diff
pkg/datastore/mysql/query.go buildWhereClause 100.00% 90.00% -10.00%

@nakabonne
Copy link
Member Author

/hold cancel

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.32%. This pull request decreases coverage by -0.00%.

File Function Base Head Diff
pkg/datastore/mysql/query.go buildWhereClause 100.00% 90.00% -10.00%

khanhtc1202
khanhtc1202 reviewed Jun 7, 2021
View reviewed changes
pkg/datastore/mysql/query.go
@@ -74,6 +74,8 @@ func buildWhereClause(filters []datastore.ListFilter) string {
// Make string of (?,...) which contains the number of `?` equal to the element number of filter.Value
valLength := reflect.ValueOf(filter.Value).Len()
conds[i] = fmt.Sprintf("%s %s (?%s)", filter.Field, filter.Operator, strings.Repeat(",?", valLength-1))
case "MEMBER OF":
conds[i] = fmt.Sprintf("? %s (%s)", filter.Operator, filter.Field)
Copy link
Member

Choose a reason for hiding this comment

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

Please add test for this in query_test.go/TestBuildFindQuery 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Right right

@nakabonne nakabonne mentioned this pull request Jun 7, 2021
Merged
pipecd-bot pushed a commit that referenced this pull request Jun 7, 2021
@nakabonne
Add notes on how to apply changes on the dev cluster (#2046)
618bc14 
**What this PR does / why we need it**:
To prevent further casualties...
Ref: #2042 (comment)

**Which issue(s) this PR fixes**:


**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
-->
```release-note
NONE
```

This PR was merged by Kapetanios.
@pipecd-bot
Copy link
Collaborator

TODO

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Create index on ProjectId and EnvIds

https://github.com/pipe-cd/pipe/blob/c01c30d61e7fff356cddb752e8861c77dfbf4342/pkg/datastore/mysql/ensurer/indexes.sql#L92-L93

This was created by todo plugin since "TODO:" was found in c01c30d when #2042 was merged. cc: @nakabonne.

@nakabonne
Copy link
Member Author

PTAL ;)

@khanhtc1202
Copy link
Member

Way to go 🙌
/lgtm

@pipecd-bot pipecd-bot added the lgtm label Jun 7, 2021
@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.34%. This pull request increases coverage by 0.02%.

File Function Base Head Diff
pkg/datastore/mysql/query.go refineFiltersOperator 72.73% 81.82% +9.09%
pkg/datastore/mysql/query.go buildWhereClause 100.00% 100.00% +0.00%

@nghialv
Copy link
Member

nghialv commented Jun 7, 2021

/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@pipecd-bot pipecd-bot merged commit 16bb0c8 into master Jun 7, 2021
@pipecd-bot pipecd-bot deleted the contains-json branch June 7, 2021 07:54
This was referenced Jun 7, 2021
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khanhtc1202 khanhtc1202 khanhtc1202 left review comments

@nghialv nghialv Awaiting requested review from nghialv

Assignees
No one assigned
Labels
approved lgtm release-note-none size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert contains operator into one dedicated to MySQL
4 participants
@nakabonne @pipecd-bot @khanhtc1202 @nghialv