Add IS_SUBSTRING operator for use in API resource filtering. #645

neuromage · 2019-01-08T01:25:33Z

This should allow substring matches on fields like names and labels and
so on.

Also bump the version of Mastermind/squirrel so we get the new 'like'
operator for use when building the SQL query.

Additionally, I also had to fix the generate_api.sh script which had a
bug (it modified the wrong file permissions before), and add a dummy
service to generate Swagger definitions for the Filter itself (this was
a hack in the previous Makefile that I lost when we moved to Bazel).

This change is

This should allow substring matches on fields like names and labels and so on. Also bump the version of Mastermind/squirrel so we get the new 'like' operator for use when building the SQL query. Additionally, I also had to fix the generate_api.sh script which had a bug (it modified the wrong file permissions before), and add a dummy service to generate Swagger definitions for the Filter itself (this was a hack in the previous Makefile that I lost when we moved to Bazel).

neuromage · 2019-01-08T01:26:00Z

/assign @rileyjbauer
/assign @IronPan

rileyjbauer · 2019-01-08T01:55:39Z

backend/src/apiserver/filter/filter.go

@@ -107,6 +110,14 @@ func (f *Filter) AddToSelect(sb squirrel.SelectBuilder) squirrel.SelectBuilder {
 		sb = sb.Where(squirrel.Eq(f.in))
 	}

+	if len(f.substring) > 0 {
+		like := make(squirrel.Like)
+		for k, v := range f.substring {


why is this loop needed?

To prepend/append the % character to the string being matched.

Maybe go is just strange... are you looping over each letter? there can't be more than one substring in a filter, right?
Is there no better way to access the key/value in substring?

Actually, I'm looping over the map named substring, which holds key/values for substring matches in the filter. So

for k, v := range f.substring

means loop over the map f.substring, and let k be the map key and v be the map value.

do you mind add more comments explaining it?

Ah, I see... so the squirrel operations take a map, and the other operations are already formatted correctly, but LIKE has its special syntax.

Out of curiosity, if a filter has multiple substring predicates, are those combined into a single f.substring map?

Added more comments.

rileyjbauer · 2019-01-08T17:20:22Z

/lgtm

IronPan · 2019-01-08T18:40:08Z

backend/api/filter.proto

@@ -115,4 +119,6 @@ message Filter {
  repeated Predicate predicates = 1;
 }

-
+service DummyFilterService {


do we need this?

Yes, otherwise we don't get the swagger definitions being generated for Filter message. Added clarifying comments

IronPan · 2019-01-08T18:43:51Z

backend/src/apiserver/filter/filter.go

+		case *api.Predicate_StringValue:
+			return nil
+		default:
+			return fmt.Errorf("cannot use non string value type %T with operator %v", p.Op, t)


i don't think we should return fmt.errorf, as we dissused before.

same applies to other places

IronPan · 2019-01-08T18:45:33Z

go.sum

@@ -4,6 +4,8 @@ github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/Masterminds/squirrel v0.0.0-20170825200431-a6b93000bd21 h1:7+Xpo0TxPIBqSHVktrR1VYU2LSNTrJ3B79nVzE74nPA=
 github.com/Masterminds/squirrel v0.0.0-20170825200431-a6b93000bd21/go.mod h1:xnKTFzjGUiZtiOagBsfnvomW+nJg2usB1ZpordQWqNM=
+github.com/Masterminds/squirrel v0.0.0-20190107164353-fa735ea14f09 h1:enWVS77aJkLWVIUExiqF6A8eWTVzCXUKUvkST3/wyKI=


are changes in go.sum needed? if version update is needed, should old version entry be removed?

I didn't touch this file. This is updated by Go itself when one does go get <library>@<version>. There are multiple such instances of duplicates in this file. See also golang/go#28456

IronPan · 2019-01-08T20:34:51Z

/lgtm
/approve

k8s-ci-robot · 2019-01-08T20:34:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-01-08T20:34:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yebrahim · 2019-01-09T00:43:11Z

/lgtm

…ubeflow#645) * Management cluster is a cluster running Cloud Config connector which can be used to create GCP resources. * This PR checks in the config for cluster kf-ci-management. We also setup a namespace to administer resources in project kubeflow-ci-deployment Fix kubeflow#644

k8s-ci-robot requested review from IronPan, qimingj and vicaire January 8, 2019 01:25

k8s-ci-robot added size/L labels Jan 8, 2019

k8s-ci-robot assigned IronPan and rileyjbauer Jan 8, 2019

rileyjbauer reviewed Jan 8, 2019

View reviewed changes

k8s-ci-robot added lgtm labels Jan 8, 2019

IronPan reviewed Jan 8, 2019

View reviewed changes

Add comments for DummyFilterService

d768428

k8s-ci-robot removed lgtm labels Jan 8, 2019

neuromage added 3 commits January 8, 2019 10:57

Add more comments

25c36e1

change errors returned

fdd94b8

fix import

83c6177

k8s-ci-robot added the lgtm label Jan 8, 2019

k8s-ci-robot added the approved label Jan 8, 2019

Merge branch 'master' into filter-like

dd22cd1

k8s-ci-robot removed lgtm labels Jan 9, 2019

k8s-ci-robot assigned yebrahim Jan 9, 2019

k8s-ci-robot added lgtm labels Jan 9, 2019

k8s-ci-robot merged commit 5a9e3ff into kubeflow:master Jan 9, 2019

rileyjbauer mentioned this pull request Jan 9, 2019

Support filtering in list APIs #270

Closed

HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024

Add Gang Pu as approver (kubeflow#645)

2fc23fa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IS_SUBSTRING operator for use in API resource filtering. #645

Add IS_SUBSTRING operator for use in API resource filtering. #645

neuromage commented Jan 8, 2019 •

edited by jlewi

Loading

neuromage commented Jan 8, 2019

rileyjbauer Jan 8, 2019

neuromage Jan 8, 2019

rileyjbauer Jan 8, 2019

neuromage Jan 8, 2019

IronPan Jan 8, 2019

rileyjbauer Jan 8, 2019

neuromage Jan 8, 2019

rileyjbauer commented Jan 8, 2019

IronPan Jan 8, 2019

neuromage Jan 8, 2019

IronPan Jan 8, 2019

IronPan Jan 8, 2019

neuromage Jan 8, 2019

IronPan Jan 8, 2019

neuromage Jan 8, 2019

IronPan commented Jan 8, 2019

k8s-ci-robot commented Jan 8, 2019

k8s-ci-robot commented Jan 8, 2019

yebrahim commented Jan 9, 2019

Add IS_SUBSTRING operator for use in API resource filtering. #645

Add IS_SUBSTRING operator for use in API resource filtering. #645

Conversation

neuromage commented Jan 8, 2019 • edited by jlewi Loading

neuromage commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyjbauer commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IronPan commented Jan 8, 2019

k8s-ci-robot commented Jan 8, 2019

k8s-ci-robot commented Jan 8, 2019

yebrahim commented Jan 9, 2019

neuromage commented Jan 8, 2019 •

edited by jlewi

Loading