From d1c5a0070c6ff4b172e5fab3b0ab76527c330b58 Mon Sep 17 00:00:00 2001 From: Saman Barghi Date: Fri, 31 Mar 2023 14:39:56 -0700 Subject: [PATCH 1/4] Change Scan API to have no Order(ES) --- .../store/elasticsearch/visibility_store.go | 18 +++++-- .../visibility_store_read_test.go | 49 ++++++++++++++++--- 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/common/persistence/visibility/store/elasticsearch/visibility_store.go b/common/persistence/visibility/store/elasticsearch/visibility_store.go index dab68fec65d..76a5800eb68 100644 --- a/common/persistence/visibility/store/elasticsearch/visibility_store.go +++ b/common/persistence/visibility/store/elasticsearch/visibility_store.go @@ -66,6 +66,10 @@ var defaultSorter = []elastic.Sorter{ elastic.NewFieldSort(searchattribute.StartTime).Desc().Missing("_first"), } +var docSorter = []elastic.Sorter{ + elastic.SortByDoc{}, +} + type ( visibilityStore struct { esClient client.Client @@ -439,7 +443,7 @@ func (s *visibilityStore) ListWorkflowExecutions( ctx context.Context, request *manager.ListWorkflowExecutionsRequestV2, ) (*store.InternalListWorkflowExecutionsResponse, error) { - p, err := s.buildSearchParametersV2(request) + p, err := s.buildSearchParametersV2(request, false) if err != nil { return nil, err } @@ -478,7 +482,7 @@ func (s *visibilityStore) scanWorkflowExecutionsWithPit( ctx context.Context, request *manager.ListWorkflowExecutionsRequestV2, ) (*store.InternalListWorkflowExecutionsResponse, error) { - p, err := s.buildSearchParametersV2(request) + p, err := s.buildSearchParametersV2(request, true) if err != nil { return nil, err } @@ -527,7 +531,7 @@ func (s *visibilityStore) scanWorkflowExecutionsWithScroll(ctx context.Context, // First call doesn't have token with ScrollID. if len(request.NextPageToken) == 0 { // First page. - p, err := s.buildSearchParametersV2(request) + p, err := s.buildSearchParametersV2(request, true) if err != nil { return nil, err } @@ -663,6 +667,7 @@ func (s *visibilityStore) buildSearchParameters( func (s *visibilityStore) buildSearchParametersV2( request *manager.ListWorkflowExecutionsRequestV2, + isScan bool, ) (*client.SearchParameters, error) { boolQuery, fieldSorts, err := s.convertQuery( @@ -687,7 +692,7 @@ func (s *visibilityStore) buildSearchParametersV2( Index: s.index, Query: boolQuery, PageSize: request.PageSize, - Sorter: s.setDefaultFieldSort(fieldSorts), + Sorter: s.setDefaultFieldSort(fieldSorts, isScan), } return params, nil @@ -730,8 +735,11 @@ func (s *visibilityStore) convertQuery( return namespaceFilterQuery, fieldSorts, nil } -func (s *visibilityStore) setDefaultFieldSort(fieldSorts []*elastic.FieldSort) []elastic.Sorter { +func (s *visibilityStore) setDefaultFieldSort(fieldSorts []*elastic.FieldSort, isScan bool) []elastic.Sorter { if len(fieldSorts) == 0 { + if isScan { + return docSorter + } return defaultSorter } diff --git a/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go b/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go index 4b874247f43..fc6db92db3f 100644 --- a/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go +++ b/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go @@ -414,6 +414,27 @@ func (s *ESVisibilitySuite) TestBuildSearchParameters() { }, p) } +func (s *ESVisibilitySuite) TestSetDefaultFieldSort() { + // test defaultSorter is retunred when not isScan + fieldSorts := make([]*elastic.FieldSort, 0) + sorter := s.visibilityStore.setDefaultFieldSort(fieldSorts, false) + s.Equal(defaultSorter, sorter) + + // test docSorter is returned when isScan + sorter = s.visibilityStore.setDefaultFieldSort(fieldSorts, true) + s.Equal(docSorter, sorter) + + // test passing non-empty fieldSorts + testFieldSorts := [2]*elastic.FieldSort{elastic.NewFieldSort("_test"), elastic.NewFieldSort("_second_tes")} + s.mockMetricsHandler.EXPECT().Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Return(metrics.NoopCounterMetricFunc) + sorter = s.visibilityStore.setDefaultFieldSort(testFieldSorts[:], true) + expectedSorter := make([]elastic.Sorter, len(testFieldSorts)+1) + expectedSorter[0] = testFieldSorts[0] + expectedSorter[1] = testFieldSorts[1] + expectedSorter[2] = elastic.NewFieldSort(searchattribute.RunID).Desc() + s.Equal(expectedSorter, sorter) +} + func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { request := &manager.ListWorkflowExecutionsRequestV2{ NamespaceID: testNamespaceID, @@ -428,7 +449,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { request.Query = `WorkflowId="guid-2208"` filterQuery := elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208")) boolQuery := elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery).MustNot(namespaceDivisionExists) - p, err := s.visibilityStore.buildSearchParametersV2(request) + p, err := s.visibilityStore.buildSearchParametersV2(request, false) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -445,7 +466,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { // note namespace division appears in the filterQuery, not the boolQuery like the negative version filterQuery = elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208"), matchNSDivision) boolQuery = elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery) - p, err = s.visibilityStore.buildSearchParametersV2(request) + p, err = s.visibilityStore.buildSearchParametersV2(request, false) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -461,7 +482,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { request.Query = `Order bY WorkflowId` boolQuery = elastic.NewBoolQuery().Filter(matchNamespaceQuery).MustNot(namespaceDivisionExists) s.mockMetricsHandler.EXPECT().Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Return(metrics.NoopCounterMetricFunc) - p, err = s.visibilityStore.buildSearchParametersV2(request) + p, err = s.visibilityStore.buildSearchParametersV2(request, false) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -476,9 +497,25 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { }, p) request.Query = "" + // test with Scan API + request.Query = `WorkflowId="guid-2208"` + filterQuery = elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208")) + boolQuery = elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery).MustNot(namespaceDivisionExists) + p, err = s.visibilityStore.buildSearchParametersV2(request, true) + s.NoError(err) + s.Equal(&client.SearchParameters{ + Index: testIndex, + Query: boolQuery, + SearchAfter: nil, + PointInTime: nil, + PageSize: testPageSize, + Sorter: docSorter, + }, p) + request.Query = "" + // test for wrong query request.Query = "invalid query" - p, err = s.visibilityStore.buildSearchParametersV2(request) + p, err = s.visibilityStore.buildSearchParametersV2(request, false) s.Nil(p) s.Error(err) request.Query = "" @@ -500,7 +537,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2DisableOrderByClause() { request.Query = `WorkflowId="guid-2208"` filterQuery := elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208")) boolQuery := elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery).MustNot(namespaceDivisionExists) - p, err := s.visibilityStore.buildSearchParametersV2(request) + p, err := s.visibilityStore.buildSearchParametersV2(request, false) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -514,7 +551,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2DisableOrderByClause() { // test invalid query with ORDER BY request.Query = `ORDER BY WorkflowId` - p, err = s.visibilityStore.buildSearchParametersV2(request) + p, err = s.visibilityStore.buildSearchParametersV2(request, false) s.Nil(p) s.Error(err) var invalidArgumentErr *serviceerror.InvalidArgument From 081689a8cc006ed12273217092cfa9ef8c3c411a Mon Sep 17 00:00:00 2001 From: Saman Barghi Date: Fri, 31 Mar 2023 16:33:58 -0700 Subject: [PATCH 2/4] Refactoring based on PR comments --- .../store/elasticsearch/visibility_store.go | 43 +++++++++---- .../visibility_store_read_test.go | 62 ++++++++++++++----- 2 files changed, 80 insertions(+), 25 deletions(-) diff --git a/common/persistence/visibility/store/elasticsearch/visibility_store.go b/common/persistence/visibility/store/elasticsearch/visibility_store.go index 76a5800eb68..f5ae1349dc0 100644 --- a/common/persistence/visibility/store/elasticsearch/visibility_store.go +++ b/common/persistence/visibility/store/elasticsearch/visibility_store.go @@ -91,6 +91,10 @@ type ( // For ES>=7.10.0 and "default" flavor. PointInTimeID string } + + searchParameterBuilder struct { + store *visibilityStore + } ) var _ store.VisibilityStore = (*visibilityStore)(nil) @@ -443,7 +447,7 @@ func (s *visibilityStore) ListWorkflowExecutions( ctx context.Context, request *manager.ListWorkflowExecutionsRequestV2, ) (*store.InternalListWorkflowExecutionsResponse, error) { - p, err := s.buildSearchParametersV2(request, false) + p, err := s.buildSearchParametersV2(request, s.getListFieldSorter) if err != nil { return nil, err } @@ -482,7 +486,7 @@ func (s *visibilityStore) scanWorkflowExecutionsWithPit( ctx context.Context, request *manager.ListWorkflowExecutionsRequestV2, ) (*store.InternalListWorkflowExecutionsResponse, error) { - p, err := s.buildSearchParametersV2(request, true) + p, err := s.buildSearchParametersV2(request, s.getScanFieldSorter) if err != nil { return nil, err } @@ -531,7 +535,7 @@ func (s *visibilityStore) scanWorkflowExecutionsWithScroll(ctx context.Context, // First call doesn't have token with ScrollID. if len(request.NextPageToken) == 0 { // First page. - p, err := s.buildSearchParametersV2(request, true) + p, err := s.buildSearchParametersV2(request, s.getScanFieldSorter) if err != nil { return nil, err } @@ -667,7 +671,7 @@ func (s *visibilityStore) buildSearchParameters( func (s *visibilityStore) buildSearchParametersV2( request *manager.ListWorkflowExecutionsRequestV2, - isScan bool, + getFieldSorter func([]*elastic.FieldSort) ([]elastic.Sorter, error), ) (*client.SearchParameters, error) { boolQuery, fieldSorts, err := s.convertQuery( @@ -688,11 +692,16 @@ func (s *visibilityStore) buildSearchParametersV2( return nil, serviceerror.NewInvalidArgument("ORDER BY clause is not supported") } + sorter, err := getFieldSorter(fieldSorts) + if err != nil { + return nil, err + } + params := &client.SearchParameters{ Index: s.index, Query: boolQuery, PageSize: request.PageSize, - Sorter: s.setDefaultFieldSort(fieldSorts, isScan), + Sorter: sorter, } return params, nil @@ -735,14 +744,26 @@ func (s *visibilityStore) convertQuery( return namespaceFilterQuery, fieldSorts, nil } -func (s *visibilityStore) setDefaultFieldSort(fieldSorts []*elastic.FieldSort, isScan bool) []elastic.Sorter { +func (s *visibilityStore) getScanFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) { + // if sortByDoc is set (which means ScanWorkflowExecutions is invoked), custom + // ORDER BY is not supported. + if len(fieldSorts) > 0 { + return nil, serviceerror.NewInvalidArgument("ORDER BY clause is not supported") + } + if len(fieldSorts) == 0 { + return docSorter, nil + } + return s.getFieldSorter(fieldSorts) +} + +func (s *visibilityStore) getListFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) { if len(fieldSorts) == 0 { - if isScan { - return docSorter - } - return defaultSorter + return defaultSorter, nil } + return s.getFieldSorter(fieldSorts) +} +func (s *visibilityStore) getFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) { s.metricsHandler.Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Record(1) res := make([]elastic.Sorter, len(fieldSorts)+1) for i, fs := range fieldSorts { @@ -751,7 +772,7 @@ func (s *visibilityStore) setDefaultFieldSort(fieldSorts []*elastic.FieldSort, i // RunID is explicit tiebreaker. res[len(res)-1] = elastic.NewFieldSort(searchattribute.RunID).Desc() - return res + return res, nil } func (s *visibilityStore) getListWorkflowExecutionsResponse( diff --git a/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go b/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go index fc6db92db3f..d777dfa3eed 100644 --- a/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go +++ b/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go @@ -414,24 +414,51 @@ func (s *ESVisibilitySuite) TestBuildSearchParameters() { }, p) } -func (s *ESVisibilitySuite) TestSetDefaultFieldSort() { - // test defaultSorter is retunred when not isScan +func (s *ESVisibilitySuite) TestGetListFieldSorter() { + + // test defaultSorter is returned when fieldSorts is empty fieldSorts := make([]*elastic.FieldSort, 0) - sorter := s.visibilityStore.setDefaultFieldSort(fieldSorts, false) + sorter, err := s.visibilityStore.getListFieldSorter(fieldSorts) + s.NoError(err) s.Equal(defaultSorter, sorter) - // test docSorter is returned when isScan - sorter = s.visibilityStore.setDefaultFieldSort(fieldSorts, true) + // test passing non-empty fieldSorts + testFieldSorts := [2]*elastic.FieldSort{elastic.NewFieldSort("_test"), elastic.NewFieldSort("_second_tes")} + s.mockMetricsHandler.EXPECT().Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Return(metrics.NoopCounterMetricFunc) + sorter, err = s.visibilityStore.getFieldSorter(testFieldSorts[:]) + expectedSorter := make([]elastic.Sorter, len(testFieldSorts)+1) + expectedSorter[0] = testFieldSorts[0] + expectedSorter[1] = testFieldSorts[1] + expectedSorter[2] = elastic.NewFieldSort(searchattribute.RunID).Desc() + s.NoError(err) + s.Equal(expectedSorter, sorter) + +} + +func (s *ESVisibilitySuite) TestGetScanFieldSorter() { + // test docSorter is returned when fieldSorts is empty + fieldSorts := make([]*elastic.FieldSort, 0) + sorter, err := s.visibilityStore.getScanFieldSorter(fieldSorts) + s.NoError(err) s.Equal(docSorter, sorter) - // test passing non-empty fieldSorts + // test error is returned if fieldSorts is not empty + testFieldSorts := [2]*elastic.FieldSort{elastic.NewFieldSort("_test"), elastic.NewFieldSort("_second_tes")} + sorter, err = s.visibilityStore.getScanFieldSorter(testFieldSorts[:]) + s.Error(err) + s.Nil(sorter) +} +func (s *ESVisibilitySuite) TestGetFieldSorter() { + + // test passing non-empty fieldSorts and not sortByDoc testFieldSorts := [2]*elastic.FieldSort{elastic.NewFieldSort("_test"), elastic.NewFieldSort("_second_tes")} s.mockMetricsHandler.EXPECT().Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Return(metrics.NoopCounterMetricFunc) - sorter = s.visibilityStore.setDefaultFieldSort(testFieldSorts[:], true) + sorter, err := s.visibilityStore.getFieldSorter(testFieldSorts[:]) expectedSorter := make([]elastic.Sorter, len(testFieldSorts)+1) expectedSorter[0] = testFieldSorts[0] expectedSorter[1] = testFieldSorts[1] expectedSorter[2] = elastic.NewFieldSort(searchattribute.RunID).Desc() + s.NoError(err) s.Equal(expectedSorter, sorter) } @@ -449,7 +476,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { request.Query = `WorkflowId="guid-2208"` filterQuery := elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208")) boolQuery := elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery).MustNot(namespaceDivisionExists) - p, err := s.visibilityStore.buildSearchParametersV2(request, false) + p, err := s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getListFieldSorter) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -466,7 +493,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { // note namespace division appears in the filterQuery, not the boolQuery like the negative version filterQuery = elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208"), matchNSDivision) boolQuery = elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery) - p, err = s.visibilityStore.buildSearchParametersV2(request, false) + p, err = s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getListFieldSorter) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -482,7 +509,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { request.Query = `Order bY WorkflowId` boolQuery = elastic.NewBoolQuery().Filter(matchNamespaceQuery).MustNot(namespaceDivisionExists) s.mockMetricsHandler.EXPECT().Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Return(metrics.NoopCounterMetricFunc) - p, err = s.visibilityStore.buildSearchParametersV2(request, false) + p, err = s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getListFieldSorter) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -501,7 +528,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { request.Query = `WorkflowId="guid-2208"` filterQuery = elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208")) boolQuery = elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery).MustNot(namespaceDivisionExists) - p, err = s.visibilityStore.buildSearchParametersV2(request, true) + p, err = s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getScanFieldSorter) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -513,9 +540,16 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2() { }, p) request.Query = "" + // test with Scan API with custom sort + request.Query = `Order bY WorkflowId` + p, err = s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getScanFieldSorter) + s.Error(err) + s.Nil(p) + request.Query = "" + // test for wrong query request.Query = "invalid query" - p, err = s.visibilityStore.buildSearchParametersV2(request, false) + p, err = s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getListFieldSorter) s.Nil(p) s.Error(err) request.Query = "" @@ -537,7 +571,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2DisableOrderByClause() { request.Query = `WorkflowId="guid-2208"` filterQuery := elastic.NewBoolQuery().Filter(elastic.NewMatchQuery(searchattribute.WorkflowID, "guid-2208")) boolQuery := elastic.NewBoolQuery().Filter(matchNamespaceQuery, filterQuery).MustNot(namespaceDivisionExists) - p, err := s.visibilityStore.buildSearchParametersV2(request, false) + p, err := s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getListFieldSorter) s.NoError(err) s.Equal(&client.SearchParameters{ Index: testIndex, @@ -551,7 +585,7 @@ func (s *ESVisibilitySuite) TestBuildSearchParametersV2DisableOrderByClause() { // test invalid query with ORDER BY request.Query = `ORDER BY WorkflowId` - p, err = s.visibilityStore.buildSearchParametersV2(request, false) + p, err = s.visibilityStore.buildSearchParametersV2(request, s.visibilityStore.getListFieldSorter) s.Nil(p) s.Error(err) var invalidArgumentErr *serviceerror.InvalidArgument From 97a6ceb43484b0ca6f53d549ba59bb18756db67c Mon Sep 17 00:00:00 2001 From: Saman Barghi Date: Fri, 31 Mar 2023 17:11:02 -0700 Subject: [PATCH 3/4] PR comments --- .../store/elasticsearch/visibility_store.go | 14 +++++--------- .../elasticsearch/visibility_store_read_test.go | 5 ++--- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/common/persistence/visibility/store/elasticsearch/visibility_store.go b/common/persistence/visibility/store/elasticsearch/visibility_store.go index f5ae1349dc0..fd2e630f1b5 100644 --- a/common/persistence/visibility/store/elasticsearch/visibility_store.go +++ b/common/persistence/visibility/store/elasticsearch/visibility_store.go @@ -745,25 +745,21 @@ func (s *visibilityStore) convertQuery( } func (s *visibilityStore) getScanFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) { - // if sortByDoc is set (which means ScanWorkflowExecutions is invoked), custom - // ORDER BY is not supported. - if len(fieldSorts) > 0 { - return nil, serviceerror.NewInvalidArgument("ORDER BY clause is not supported") - } if len(fieldSorts) == 0 { return docSorter, nil } - return s.getFieldSorter(fieldSorts) + // custom order is not supported by Scan API + return nil, serviceerror.NewInvalidArgument("ORDER BY clause is not supported") } func (s *visibilityStore) getListFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) { if len(fieldSorts) == 0 { return defaultSorter, nil } - return s.getFieldSorter(fieldSorts) + return s.getFieldSorter(fieldSorts), nil } -func (s *visibilityStore) getFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) { +func (s *visibilityStore) getFieldSorter(fieldSorts []*elastic.FieldSort) []elastic.Sorter { s.metricsHandler.Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Record(1) res := make([]elastic.Sorter, len(fieldSorts)+1) for i, fs := range fieldSorts { @@ -772,7 +768,7 @@ func (s *visibilityStore) getFieldSorter(fieldSorts []*elastic.FieldSort) ([]ela // RunID is explicit tiebreaker. res[len(res)-1] = elastic.NewFieldSort(searchattribute.RunID).Desc() - return res, nil + return res } func (s *visibilityStore) getListWorkflowExecutionsResponse( diff --git a/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go b/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go index d777dfa3eed..b4d408b7311 100644 --- a/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go +++ b/common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go @@ -425,7 +425,7 @@ func (s *ESVisibilitySuite) TestGetListFieldSorter() { // test passing non-empty fieldSorts testFieldSorts := [2]*elastic.FieldSort{elastic.NewFieldSort("_test"), elastic.NewFieldSort("_second_tes")} s.mockMetricsHandler.EXPECT().Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Return(metrics.NoopCounterMetricFunc) - sorter, err = s.visibilityStore.getFieldSorter(testFieldSorts[:]) + sorter, err = s.visibilityStore.getListFieldSorter(testFieldSorts[:]) expectedSorter := make([]elastic.Sorter, len(testFieldSorts)+1) expectedSorter[0] = testFieldSorts[0] expectedSorter[1] = testFieldSorts[1] @@ -453,12 +453,11 @@ func (s *ESVisibilitySuite) TestGetFieldSorter() { // test passing non-empty fieldSorts and not sortByDoc testFieldSorts := [2]*elastic.FieldSort{elastic.NewFieldSort("_test"), elastic.NewFieldSort("_second_tes")} s.mockMetricsHandler.EXPECT().Counter(metrics.ElasticsearchCustomOrderByClauseCount.GetMetricName()).Return(metrics.NoopCounterMetricFunc) - sorter, err := s.visibilityStore.getFieldSorter(testFieldSorts[:]) + sorter := s.visibilityStore.getFieldSorter(testFieldSorts[:]) expectedSorter := make([]elastic.Sorter, len(testFieldSorts)+1) expectedSorter[0] = testFieldSorts[0] expectedSorter[1] = testFieldSorts[1] expectedSorter[2] = elastic.NewFieldSort(searchattribute.RunID).Desc() - s.NoError(err) s.Equal(expectedSorter, sorter) } From 6e957d8e34eba7349fd648efc9f1dfe68d57d0f9 Mon Sep 17 00:00:00 2001 From: Saman Barghi Date: Fri, 31 Mar 2023 17:13:24 -0700 Subject: [PATCH 4/4] Fix return order --- .../visibility/store/elasticsearch/visibility_store.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/common/persistence/visibility/store/elasticsearch/visibility_store.go b/common/persistence/visibility/store/elasticsearch/visibility_store.go index fd2e630f1b5..858c565ce5a 100644 --- a/common/persistence/visibility/store/elasticsearch/visibility_store.go +++ b/common/persistence/visibility/store/elasticsearch/visibility_store.go @@ -745,11 +745,12 @@ func (s *visibilityStore) convertQuery( } func (s *visibilityStore) getScanFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) { - if len(fieldSorts) == 0 { - return docSorter, nil - } // custom order is not supported by Scan API - return nil, serviceerror.NewInvalidArgument("ORDER BY clause is not supported") + if len(fieldSorts) > 0 { + return nil, serviceerror.NewInvalidArgument("ORDER BY clause is not supported") + } + + return docSorter, nil } func (s *visibilityStore) getListFieldSorter(fieldSorts []*elastic.FieldSort) ([]elastic.Sorter, error) {