Update ListAllWorkflowExecutions pinot endpoint #6160
Conversation
sankari165
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| } else { | ||
| val = fmt.Sprintf("%v", val) | ||
| s.string += fmt.Sprintf("%s = %v\n", obj, val) |
There was a problem hiding this comment.
A test for this case is missing (according to codecov).
| @@ -721,19 +721,17 @@ func (s *PinotQuerySearchField) resetSearchField() { | |||
| func (s *PinotQuerySearchField) addEqual(obj string, val interface{}) { | |||
| s.checkFirstSearchField() | |||
| if _, ok := val.(string); ok { | |||
| val = fmt.Sprintf("'%s'", val) | |||
| s.string += fmt.Sprintf("%s = '%s'\n", obj, val) | |||
There was a problem hiding this comment.
The whole thing with += of strings is extremely error-prone and inefficient.
I suggest revisiting this and building a set of filters to groups with "()" around them and then joining them together in one go.
But that would be a separate problem/PR.
cc @neil-xie
| } | ||
|
|
||
| func (s *PinotQuerySearchField) addMatch(obj string, val interface{}) { | ||
| s.checkFirstSearchField() | ||
|
|
||
| s.string += fmt.Sprintf("text_match(%s, '\"%s\"')\n", obj, val) | ||
| s.string += fmt.Sprintf("REGEXP_LIKE(%s, '^.*%s.*$')\n", obj, val) |
There was a problem hiding this comment.
Once we add the text index, we should use Text_Match for partial match.
Btw, for regexp like query, partial match can timeout for large cluster, it can work with prefix match which is REGEXP_LIKE(%s, '^%s.*$')
There was a problem hiding this comment.
We can update and test after Pinot team enable text index for us.
What changed?
partial match uses REGEXP_LIKE instead of text_match
Workflow search leverages new duplicate fields in pinot ( introduced in #6149 )
Workflow status match fixed to be int
Why?
Text index offers better performance and other changes are fixes
How did you test it?
unit tests and querybuilder to run the constructed queries
Potential risks
Release notes
Documentation Changes