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

Update ListAllWorkflowExecutions pinot endpoint #6160

Merged
merged 2 commits into from
Jul 8, 2024

Conversation

sankari165
Copy link
Contributor

@sankari165 sankari165 commented Jul 5, 2024

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

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.66%. Comparing base (731cec6) to head (f9ec265).
Report is 3 commits behind head on master.

Additional details and impacted files
Files Coverage Δ
common/persistence/pinot/pinot_visibility_store.go 93.19% <86.66%> (+<0.01%) ⬆️

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc05a78...f9ec265. Read the comment docs.

common/persistence/pinot/pinot_visibility_store.go Outdated Show resolved Hide resolved
} else {
val = fmt.Sprintf("%v", val)
s.string += fmt.Sprintf("%s = %v\n", obj, val)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@sankari165 sankari165 merged commit 1e2ef3d into uber:master Jul 8, 2024
20 checks passed
@sankari165 sankari165 deleted the CDNC-9747 branch July 8, 2024 10:25
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.*$')

Copy link
Contributor

Choose a reason for hiding this comment

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

We can update and test after Pinot team enable text index for us.

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.

3 participants