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.

Sign up for GitHub

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

Enhance 'sort' field parser to handle various sort formats #139

Merged
merged 2 commits into from
May 17, 2024
Merged

Enhance 'sort' field parser to handle various sort formats #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a6063f
Enhance 'sort' field parser to handle shorthand maps
pivovarit May 17, 2024
43fbd7e
Separate test cases
pivovarit May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 62 additions & 34 deletions quesma/queryparser/query_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,7 @@ func (cw *ClickhouseQueryTranslator) ParseQuery(queryAsJson string) (SimpleQuery
}

if sortPart, ok := queryAsMap["sort"]; ok {
if sortAsArray, ok := sortPart.([]any); ok {
parsedQuery.SortFields = cw.parseSortFields(sortAsArray)
} else {
logger.WarnWithCtx(cw.Ctx).Msgf("unknown sort format, sort value: %v type: %T", sortPart, sortPart)
}
parsedQuery.SortFields = cw.parseSortFields(sortPart)
}

const defaultSize = 0
Expand Down Expand Up @@ -177,11 +173,7 @@ func (cw *ClickhouseQueryTranslator) ParseQueryAsyncSearch(queryAsJson string) (
}

if sort, ok := queryAsMap["sort"]; ok {
if sortAsArray, ok := sort.([]any); ok {
parsedQuery.SortFields = cw.parseSortFields(sortAsArray)
} else {
logger.WarnWithCtx(cw.Ctx).Msgf("unknown sort format, sort value: %v type: %T", sort, sort)
}
parsedQuery.SortFields = cw.parseSortFields(sort)
}
queryInfo := cw.tryProcessSearchMetadata(queryAsMap)

Expand Down Expand Up @@ -1146,38 +1138,74 @@ func (cw *ClickhouseQueryTranslator) extractInterval(queryMap QueryMap) string {

// parseSortFields parses sort fields from the query
// We're skipping ELK internal fields, like "_doc", "_id", etc. (we only accept field starting with "_" if it exists in our table)
func (cw *ClickhouseQueryTranslator) parseSortFields(sortMaps []any) []string {
sortFields := make([]string, 0)
for _, sortMapAsAny := range sortMaps {
sortMap, ok := sortMapAsAny.(QueryMap)
if !ok {
logger.WarnWithCtx(cw.Ctx).Msgf("parseSortFields: unexpected type of value: %T, value: %v", sortMapAsAny, sortMapAsAny)
continue
}

// sortMap has only 1 key, so we can just iterate over it
for k, v := range sortMap {
if strings.HasPrefix(k, "_") && cw.Table.GetFieldInfo(cw.Ctx, k) == clickhouse.NotExists {
// we're skipping ELK internal fields, like "_doc", "_id", etc.
func (cw *ClickhouseQueryTranslator) parseSortFields(sortMaps any) []string {
switch sortMaps := sortMaps.(type) {
case []any:
sortFields := make([]string, 0)
for _, sortMapAsAny := range sortMaps {
sortMap, ok := sortMapAsAny.(QueryMap)
if !ok {
logger.WarnWithCtx(cw.Ctx).Msgf("parseSortFields: unexpected type of value: %T, value: %v", sortMapAsAny, sortMapAsAny)
continue
}
fieldName := cw.Table.ResolveField(cw.Ctx, k)
if vAsMap, ok := v.(QueryMap); ok {
if order, ok := vAsMap["order"]; ok {
if orderAsString, ok := order.(string); ok {
sortFields = append(sortFields, strconv.Quote(fieldName)+" "+orderAsString)

// sortMap has only 1 key, so we can just iterate over it
for k, v := range sortMap {
if strings.HasPrefix(k, "_") && cw.Table.GetFieldInfo(cw.Ctx, k) == clickhouse.NotExists {
// we're skipping ELK internal fields, like "_doc", "_id", etc.
continue
}
fieldName := cw.Table.ResolveField(cw.Ctx, k)
switch v := v.(type) {
case QueryMap:
if order, ok := v["order"]; ok {
if orderAsString, ok := order.(string); ok {
sortFields = append(sortFields, strconv.Quote(fieldName)+" "+orderAsString)
} else {
logger.WarnWithCtx(cw.Ctx).Msgf("unexpected order type: %T, value: %v. Skipping", order, order)
}
} else {
logger.WarnWithCtx(cw.Ctx).Msgf("unexpected order type: %T, value: %v. Skipping", order, order)
sortFields = append(sortFields, strconv.Quote(fieldName))
}
} else {
sortFields = append(sortFields, strconv.Quote(fieldName))
case string:
sortFields = append(sortFields, strconv.Quote(fieldName)+" "+v)
default:
logger.WarnWithCtx(cw.Ctx).Msgf("unexpected 'sort' value's type: %T (key, value): (%s, %v). Skipping", v, k, v)
}
} else {
logger.WarnWithCtx(cw.Ctx).Msgf("unexpected value's type: %T (key, value): (%s, %v). Skipping", v, k, v)
}
}
return sortFields
case map[string]interface{}:
sortFields := make([]string, 0)

for fieldName, fieldValue := range sortMaps {
if strings.HasPrefix(fieldName, "_") && cw.Table.GetFieldInfo(cw.Ctx, fieldName) == clickhouse.NotExists {
// TODO Elastic internal fields will need to be supported in the future
continue
}
if fieldValue, ok := fieldValue.(string); ok {
sortFields = append(sortFields, fmt.Sprintf("%s %s", strconv.Quote(fieldName), fieldValue))
}
}

return sortFields

case map[string]string:
sortFields := make([]string, 0)

for fieldName, fieldValue := range sortMaps {
if strings.HasPrefix(fieldName, "_") && cw.Table.GetFieldInfo(cw.Ctx, fieldName) == clickhouse.NotExists {
// TODO Elastic internal fields will need to be supported in the future
continue
}
sortFields = append(sortFields, fmt.Sprintf("%s %s", strconv.Quote(fieldName), fieldValue))
}

return sortFields
Comment on lines +1177 to +1204
Copy link
Member

Choose a reason for hiding this comment

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

in previous implementation, we do ResolveField. Don't we want to do it in these 2 cases as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is handled at the beginning of each search processing, so we never reach here with nonexistent columns - I will unify that

default:
logger.ErrorWithCtx(cw.Ctx).Msgf("unexpected type of sortMaps: %T, value: %v", sortMaps, sortMaps)
return []string{}
}
return sortFields
}

func (cw *ClickhouseQueryTranslator) parseSize(queryMap QueryMap) (size int, ok bool) {
Expand Down
40 changes: 34 additions & 6 deletions quesma/queryparser/query_parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,22 +453,48 @@ func TestQueryParseDateMathExpression(t *testing.T) {

func Test_parseSortFields(t *testing.T) {
tests := []struct {
sortMap []any
name string
sortMap any
sortFields []string
}{
{
[]any{
name: "compound",
sortMap: []any{
QueryMap{"@timestamp": QueryMap{"format": "strict_date_optional_time", "order": "desc", "unmapped_type": "boolean"}},
QueryMap{"service.name": QueryMap{"order": "asc", "unmapped_type": "boolean"}},
QueryMap{"no_order_field": QueryMap{"unmapped_type": "boolean"}},
QueryMap{"_table_field_with_underscore": QueryMap{"order": "asc", "unmapped_type": "boolean"}}, // this should be accepted, as it exists in the table
QueryMap{"_doc": QueryMap{"order": "desc", "unmapped_type": "boolean"}}, // this should be discarded, as it doesn't exist in the table
},
[]string{`"@timestamp" desc`, `"service.name" asc`, `"no_order_field"`, `"_table_field_with_underscore" asc`},
sortFields: []string{`"@timestamp" desc`, `"service.name" asc`, `"no_order_field"`, `"_table_field_with_underscore" asc`},
},
{
[]any{},
[]string{},
name: "empty",
sortMap: []any{},
sortFields: []string{},
},
{
name: "map[string]string",
sortMap: map[string]string{
"timestamp": "desc",
"_doc": "desc",
},
sortFields: []string{`"timestamp" desc`},
},
{
name: "map[string]interface{}",
sortMap: map[string]interface{}{
"timestamp": "desc",
"_doc": "desc",
},
sortFields: []string{`"timestamp" desc`},
}, {
name: "[]map[string]string",
sortMap: []any{
QueryMap{"@timestamp": "asc"},
QueryMap{"_doc": "asc"},
},
sortFields: []string{`"@timestamp" asc`},
},
}
table, _ := clickhouse.NewTable(`CREATE TABLE `+tableName+`
Expand All @@ -479,6 +505,8 @@ func Test_parseSortFields(t *testing.T) {
lm := clickhouse.NewLogManager(concurrent.NewMapWith(tableName, table), config.QuesmaConfiguration{})
cw := ClickhouseQueryTranslator{ClickhouseLM: lm, Table: table, Ctx: context.Background()}
for _, tt := range tests {
assert.Equal(t, tt.sortFields, cw.parseSortFields(tt.sortMap))
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.sortFields, cw.parseSortFields(tt.sortMap))
})
}
}
Loading