Handle namespace division filter in SQL queries #3931
Conversation
rodrigozhou
changed the title
| @@ -179,7 +183,25 @@ func (c *QueryConverter) convertSelectStmt(sel *sqlparser.Select) error { | |||
| } | |||
|
|
|||
| if sel.Where != nil { | |||
There was a problem hiding this comment.
When might Where be nil? (I haven't really read any of this code yet so sorry if it's a dumb question)
Is this change enough to make things work for old-style ListOpenWorkflowExecutionsByType, etc. as well as string queries?
There was a problem hiding this comment.
If the user query string is empty.
Actually, I think I made a mistake... I think I have to add either way (ie, even if it's nil)
There was a problem hiding this comment.
Hm, but we do want to add the constraint in that case. Basically in all cases unless they specifically query for a different division
There was a problem hiding this comment.
@dnr Fixed the query converter, and visibility store to handle namespace division field.
There was a problem hiding this comment.
I don't have enough context to review the whole thing but that part looks good.
Unfortunately we don't have a standalone integration test for that, since only ES supported it at all before, but ScheduleIntegrationSuite exercises the NamespaceDivision stuff (as Alex discovered)
rodrigozhou
force-pushed
the
add-namespace-division-cmp
branch
from
c9069aa to
2055eb9
Compare
rodrigozhou
changed the title
| fmt.Sprintf( | ||
| "%s != %d", | ||
| searchattribute.ExecutionStatus, | ||
| int32(enumspb.WORKFLOW_EXECUTION_STATUS_RUNNING), |
There was a problem hiding this comment.
I didn't realized that ExecutionStatus is int. I changed in in ES, but we can't change here :-(
| searchattribute.StartTime, | ||
| request.EarliestStartTime.UTC().Format(time.RFC3339Nano), | ||
| request.LatestStartTime.UTC().Format(time.RFC3339Nano), | ||
| Query: s.buildQueryStringFromListRequest( |
There was a problem hiding this comment.
I hope we will deprecate and remove all these methods soon.
rodrigozhou
force-pushed
the
add-namespace-division-cmp
branch
from
2055eb9 to
a6bd463
Compare
What changed?
Handle namespace division filter in the new SQL visibility store.
Why?
Copying logic from Elasticsearch visibility store/query builder (namespace division was previously ignored).
How did you test it?
Existing tests.
Potential risks
No risks.
Is hotfix candidate?
No.