Remove RunId from visibility query sort for ES 7 #3140
Conversation
| @@ -65,7 +65,6 @@ const ( | |||
| var defaultSorter = []elastic.Sorter{ | |||
| elastic.NewFieldSort(searchattribute.CloseTime).Desc().Missing("_first"), | |||
| elastic.NewFieldSort(searchattribute.StartTime).Desc().Missing("_first"), | |||
| elastic.NewFieldSort(searchattribute.RunID).Desc().Missing("_first"), | |||
There was a problem hiding this comment.
You need to change setDefaultFieldSort too.
There was a problem hiding this comment.
I just read PR description. I believe it should be removed from setDefaultFieldSort in this PR.
There was a problem hiding this comment.
Tried and I feel we should not do it. So I reverted the change. There are two reasons:
(1) We are disabling user specified orderby by default. This is not an issue if that's not turned on.
(2) If user intentionally turn it on, then removing the RunId as tie-breaker can cause a behavior change, if user does not specify all tie-breakers in order by (very likely the case, e.g. if they order by some custom search attributes with low cardinality).
There was a problem hiding this comment.
Default value for disable ORDER BY is false. But I agree that if custom order is specified then query is already slow. Adding RunId won't hurt much.
alexshtin
changed the title
meiliang86
force-pushed
the
visibility-sort
branch
from
3ced517
to
689dae3
Compare
meiliang86
changed the title
meiliang86
force-pushed
the
visibility-sort
branch
from
2d6e714
to
6c4c509
Compare
What changed?
Remove
RunIdfrom visibility query sort for ES v7.Why?
Improve query performance
How did you test it?
Manually sending queries to ES
Potential risks
Duplicate or missing entries when doing pagination.
Although this is unlikely because both closed time and start time has nano seconds precision in ES v7.
For ES v6 this is not changed because date has only milliseconds precision and collisions are more likely.
Is hotfix candidate?
Could be.