Support STARTS_WITH in visibility queries #5094
Conversation
rodrigozhou
force-pushed
the
vis-support-starts-with
branch
4 times, most recently
from
52d4557 to
5ebdd3c
Compare
| if !ok { | ||
| return nil, NewConverterError("right-hand side of '%v' must be a string", comparisonExpr.Operator) | ||
| } | ||
| query = elastic.NewBoolQuery().MustNot(elastic.NewPrefixQuery(colName, v)) |
There was a problem hiding this comment.
Don't you need to enable index_prefixes for this to work?
There was a problem hiding this comment.
No. index_prefixes would make search faster, but not required.
rodrigozhou
force-pushed
the
vis-support-starts-with
branch
2 times, most recently
from
c578ab2 to
fb28633
Compare
| } | ||
| resp, err := s.engine.ListWorkflowExecutions(NewContext(), listRequest) | ||
| s.NoError(err) | ||
| s.Len(resp.GetExecutions(), 1) |
There was a problem hiding this comment.
Same here. Please ensure we get the expected record, not just any record
There was a problem hiding this comment.
These are old tests, just moved around, and also soon to be deleted.
There was a problem hiding this comment.
I don't see new tests that verify this functionality works as expected, are you planning to add them?
rodrigozhou
force-pushed
the
vis-support-starts-with
branch
from
fb28633 to
d4938fb
Compare
| { | ||
| name: "starts_with expression", | ||
| input: "AliasForKeyword01 starts_with 'foo_bar%'", | ||
| output: `Keyword01 like 'foo!_bar!%%' escape '!'`, | ||
| err: nil, | ||
| }, | ||
| { | ||
| name: "not starts_with expression", | ||
| input: "AliasForKeyword01 not starts_with 'foo_bar%'", | ||
| output: `Keyword01 not like 'foo!_bar!%%' escape '!'`, | ||
| err: nil, | ||
| }, | ||
| { | ||
| name: "starts_with expression error", | ||
| input: "AliasForKeyword01 starts_with 123", | ||
| output: "", | ||
| err: query.NewConverterError( | ||
| "%s: right-hand side of '%s' must be a literal string (got: 123)", | ||
| query.InvalidExpressionErrMessage, | ||
| sqlparser.StartsWithStr, | ||
| ), | ||
| }, | ||
| { | ||
| name: "not starts_with expression error", | ||
| input: "AliasForKeyword01 not starts_with 123", | ||
| output: "", | ||
| err: query.NewConverterError( | ||
| "%s: right-hand side of '%s' must be a literal string (got: 123)", | ||
| query.InvalidExpressionErrMessage, | ||
| sqlparser.NotStartsWithStr, | ||
| ), | ||
| }, |
There was a problem hiding this comment.
@tdeebswihart New tests.
There was a problem hiding this comment.
I'd seen the unit tests. So we don't have useful functional tests for this functionality? The only ones are what I called out earlier which don't bother to check whether the records we expect are the ones returned, and I've seen a test like that hide bugs in the past.
| "value starts_with 'prefix'": `{"bool":{"filter":{"prefix":{"value":"prefix"}}}}`, | ||
| "value not starts_with 'prefix'": `{"bool":{"must_not":{"prefix":{"value":"prefix"}}}}`, |
There was a problem hiding this comment.
@tdeebswihart New tests.
rodrigozhou
force-pushed
the
vis-support-starts-with
branch
from
d4938fb to
51abcc0
Compare
tests/advanced_visibility_test.go
Outdated
| // Prefix search | ||
| listRequest = &workflowservice.ListWorkflowExecutionsRequest{ | ||
| Namespace: s.namespace, | ||
| PageSize: defaultPageSize, | ||
| Query: `CustomKeywordField STARTS_WITH "justice"`, | ||
| } | ||
| resp, err = s.engine.ListWorkflowExecutions(NewContext(), listRequest) | ||
| s.NoError(err) | ||
| s.Len(resp.GetExecutions(), 1) | ||
| s.Equal(id, resp.Executions[0].GetExecution().GetWorkflowId()) | ||
| s.Equal(wt, resp.Executions[0].GetType().GetName()) | ||
| s.Equal(searchAttr, resp.Executions[0].GetSearchAttributes()) | ||
|
|
||
| // LIKE exact match on Keyword (supported) | ||
| listRequest = &workflowservice.ListWorkflowExecutionsRequest{ | ||
| Namespace: s.namespace, | ||
| PageSize: defaultPageSize, | ||
| Query: `CustomKeywordField LIKE "%justice for all%"`, | ||
| } | ||
| resp, err = s.engine.ListWorkflowExecutions(NewContext(), listRequest) | ||
| s.NoError(err) | ||
| s.Len(resp.GetExecutions(), 1) | ||
| listRequest = &workflowservice.ListWorkflowExecutionsRequest{ | ||
| Namespace: s.namespace, | ||
| PageSize: defaultPageSize, | ||
| Query: `CustomKeywordField NOT STARTS_WITH "justice"`, | ||
| } | ||
| resp, err = s.engine.ListWorkflowExecutions(NewContext(), listRequest) | ||
| s.NoError(err) | ||
| s.Len(resp.GetExecutions(), 0) |
There was a problem hiding this comment.
@tdeebswihart These are functional tests.
There was a problem hiding this comment.
These are the ones I raise an issue about earlier. These tests look for the presence of a record (or no records) but do not verify that the returned record is what we expect.
Can you update these to ensure the returned execution actually matches the filter?I've seen tests like this (that look for the presence of a record) hide bugs where the incorrect record was returned
There was a problem hiding this comment.
I already added checks. See L555-557.
rodrigozhou
force-pushed
the
vis-support-starts-with
branch
from
51abcc0 to
dd72876
Compare
What changed?
Parse
STARTS_WITHqueries to visibility, and build queries to Elasticsearch and SQL.Why?
Support prefix search in visibility queries.
How did you test it?
Added unit tests, and integration tests.
Potential risks
No.
Is hotfix candidate?
No.