Skip to content

Commit 1c966b7

Browse files
talorictaloric
committed
fix: handle extra_filters with correct seq
1 parent 601bd20 commit 1c966b7

File tree

2 files changed

+127
-76
lines changed

2 files changed

+127
-76
lines changed

‎server/querier/app/prometheus/service/converters.go‎

Lines changed: 85 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,9 @@ func (p *prometheusReader) parseExtraFilters(filter *extraFilters, db string) (s
387387
if filter.label == "" || filter.operator == "" {
388388
return "", ""
389389
}
390+
if filter.operator == "exist" || filter.operator == "not exist" {
391+
return filter.label, fmt.Sprintf("%s(%s)", filter.operator, filter.label)
392+
}
390393
return filter.label, fmt.Sprintf("%s %s %s", filter.label, filter.operator, escapeSingleQuote(filter.value))
391394
}
392395

@@ -425,26 +428,34 @@ func (p *prometheusReader) parseMatchers(matcher *prompb.LabelMatcher, prefixTyp
425428
}
426429

427430
// parse extra-filters to filters in `where` clause
428-
func (p *prometheusReader) parseExtraFiltersToWhereClause(extraLabelFilters [][]*extraFilters, prefixType prefix, db string, handleTags func(string, bool)) string {
429-
outerFilters := make([]string, 0, len(extraLabelFilters))
430-
for i := 0; i < len(extraLabelFilters); i++ {
431-
innerFilters := make([]string, 0, len(extraLabelFilters[i]))
432-
for j := 0; j < len(extraLabelFilters[i]); j++ {
433-
matcher := extraLabelFilters[i][j]
434-
tagName, newFilter := p.parseExtraFilters(matcher, db)
435-
if newFilter == "" {
436-
continue
437-
}
438-
innerFilters = append(innerFilters, newFilter)
439-
handleTags(tagName, matcher.isTag)
431+
func (p *prometheusReader) parseExtraFiltersToWhereClause(node *filterNode, prefixType prefix, db string, handleTags func(string, bool)) string {
432+
if node == nil {
433+
return ""
434+
}
435+
if node.filter != nil {
436+
// leaf node
437+
tagName, sql := p.parseExtraFilters(node.filter, db)
438+
if sql == "" {
439+
return ""
440440
}
441-
// inside matchers use 'AND' for connected
442-
if len(innerFilters) > 0 {
443-
outerFilters = append(outerFilters, fmt.Sprintf("(%s)", strings.Join(innerFilters, " AND ")))
441+
handleTags(tagName, node.filter.isTag)
442+
return sql
443+
}
444+
// internal AND/OR node: recurse into children
445+
parts := make([]string, 0, len(node.children))
446+
for _, child := range node.children {
447+
s := p.parseExtraFiltersToWhereClause(child, prefixType, db, handleTags)
448+
if s != "" {
449+
parts = append(parts, s)
444450
}
445451
}
446-
// outside matchers use 'OR' for connected
447-
return fmt.Sprintf("(%s)", strings.Join(outerFilters, " OR "))
452+
if len(parts) == 0 {
453+
return ""
454+
}
455+
if len(parts) == 1 {
456+
return parts[0]
457+
}
458+
return fmt.Sprintf("(%s)", strings.Join(parts, " "+node.op+" "))
448459
}
449460

450461
// return: prefixType, metricName, db, table, dataPrecision, metricAlias
@@ -1570,60 +1581,62 @@ type extraFilters struct {
15701581
isTag bool
15711582
}
15721583

1573-
func parseExtraFiltersToMatchers(filters string) ([][]*extraFilters, error) {
1584+
// filterNode is a boolean expression tree of extra filters.
1585+
// Leaf node: filter != nil. Internal node: op is "AND"/"OR", children non-empty.
1586+
type filterNode struct {
1587+
filter *extraFilters
1588+
op string
1589+
children []*filterNode
1590+
}
1591+
1592+
func parseExtraFiltersToMatchers(filters string) (*filterNode, error) {
15741593
fakeSQL := fmt.Sprintf("select 1 from t where %s", filters)
15751594
stmt, err := sqlparser.Parse(fakeSQL)
15761595
if err != nil {
15771596
return nil, err
15781597
}
15791598
selectStmt := stmt.(*sqlparser.Select)
1580-
labelMatchers := make([][]*extraFilters, 0)
1581-
_, err = iterateExprs(selectStmt.Where.Expr, &labelMatchers)
1582-
if err != nil {
1583-
return nil, err
1584-
}
1585-
return labelMatchers, nil
1599+
return iterateExprs(selectStmt.Where.Expr)
15861600
}
15871601

1588-
func iterateExprs(node sqlparser.Expr, labelMatchers *[][]*extraFilters) (sqlparser.Expr, error) {
1602+
// iterateExprs builds a filterNode tree from a SQL expression.
1603+
// AND/OR structure is preserved as-is; no DNF flattening needed.
1604+
func iterateExprs(node sqlparser.Expr) (*filterNode, error) {
15891605
switch node := node.(type) {
15901606
case *sqlparser.AndExpr:
1591-
left, err := iterateExprs(node.Left, labelMatchers)
1607+
left, err := iterateExprs(node.Left)
15921608
if err != nil {
1593-
return left, err
1609+
return nil, err
15941610
}
1595-
right, err := iterateExprs(node.Right, labelMatchers)
1611+
right, err := iterateExprs(node.Right)
15961612
if err != nil {
1597-
return right, err
1613+
return nil, err
15981614
}
15991615
if left == nil {
16001616
return right, nil
1601-
} else if right == nil {
1617+
}
1618+
if right == nil {
16021619
return left, nil
16031620
}
1604-
return node, nil
1621+
return &filterNode{op: "AND", children: []*filterNode{left, right}}, nil
16051622
case *sqlparser.OrExpr:
1606-
left, err := iterateExprs(node.Left, labelMatchers)
1623+
left, err := iterateExprs(node.Left)
16071624
if err != nil {
1608-
return left, err
1625+
return nil, err
16091626
}
1610-
right, err := iterateExprs(node.Right, labelMatchers)
1627+
right, err := iterateExprs(node.Right)
16111628
if err != nil {
1612-
return right, err
1629+
return nil, err
16131630
}
16141631
if left == nil {
16151632
return right, nil
1616-
} else if right == nil {
1633+
}
1634+
if right == nil {
16171635
return left, nil
16181636
}
1619-
return node, nil
1637+
return &filterNode{op: "OR", children: []*filterNode{left, right}}, nil
16201638
case *sqlparser.ParenExpr:
1621-
(*labelMatchers) = append((*labelMatchers), []*extraFilters{})
1622-
expr, err := iterateExprs(node.Expr, labelMatchers)
1623-
if err != nil {
1624-
return expr, err
1625-
}
1626-
return expr, nil
1639+
return iterateExprs(node.Expr)
16271640
case *sqlparser.ComparisonExpr:
16281641
var comparExpr sqlparser.Expr
16291642
if parenExpr, ok := node.Left.(*sqlparser.ParenExpr); ok {
@@ -1645,22 +1658,41 @@ func iterateExprs(node sqlparser.Expr, labelMatchers *[][]*extraFilters) (sqlpar
16451658
op = node.Operator
16461659
istag = true
16471660
}
1648-
lastIndex := len(*labelMatchers) - 1
1649-
if lastIndex < 0 {
1650-
(*labelMatchers) = append((*labelMatchers), []*extraFilters{})
1651-
lastIndex = len(*labelMatchers) - 1
1652-
}
1653-
1654-
(*labelMatchers)[lastIndex] = append((*labelMatchers)[lastIndex], &extraFilters{
1661+
return &filterNode{filter: &extraFilters{
16551662
operator: op,
16561663
// some tag will escape by sqlparser
16571664
// https://github.com/xwb1989/sqlparser/blob/master/token.go#L85
16581665
label: removeEscapeQuote(colName, "`"),
16591666
value: removeEscapeQuote(colValue, "'"),
16601667
isTag: istag,
1661-
})
1662-
return node, nil
1668+
}}, nil
1669+
case *sqlparser.NotExpr:
1670+
if funcExpr, ok := node.Expr.(*sqlparser.FuncExpr); ok &&
1671+
strings.ToLower(funcExpr.Name.String()) == "exist" &&
1672+
len(funcExpr.Exprs) == 1 {
1673+
if aliasedExpr, ok := funcExpr.Exprs[0].(*sqlparser.AliasedExpr); ok {
1674+
tagName := removeEscapeQuote(sqlparser.String(aliasedExpr.Expr), "`")
1675+
return &filterNode{filter: &extraFilters{
1676+
operator: "not exist",
1677+
label: tagName,
1678+
isTag: true,
1679+
}}, nil
1680+
}
1681+
}
1682+
return nil, nil
1683+
case *sqlparser.FuncExpr:
1684+
if strings.ToLower(node.Name.String()) == "exist" && len(node.Exprs) == 1 {
1685+
if aliasedExpr, ok := node.Exprs[0].(*sqlparser.AliasedExpr); ok {
1686+
tagName := removeEscapeQuote(sqlparser.String(aliasedExpr.Expr), "`")
1687+
return &filterNode{filter: &extraFilters{
1688+
operator: "exist",
1689+
label: tagName,
1690+
isTag: true,
1691+
}}, nil
1692+
}
1693+
}
1694+
return nil, nil
16631695
default:
1664-
return node, nil
1696+
return nil, nil
16651697
}
16661698
}

‎server/querier/app/prometheus/service/converters_test.go‎

Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -918,33 +918,52 @@ func TestParseQueryRequestToSQL(t *testing.T) {
918918
}
919919

920920
func TestParseExFilters(t *testing.T) {
921+
p := &prometheusReader{}
922+
noopTags := func(string, bool) {}
923+
921924
var testCases = []struct {
922-
input string
923-
output [][]*prompb.LabelMatcher
924-
}{{
925-
input: "(pod=1 and pod_ns=2) or (pod=1 and pod_ns=3) or (pod=5 and pod_ns=3)",
926-
output: [][]*prompb.LabelMatcher{
927-
{{Name: "pod", Type: prompb.LabelMatcher_EQ, Value: "1"}, {Name: "pod_ns", Type: prompb.LabelMatcher_EQ, Value: "2"}},
928-
{{Name: "pod", Type: prompb.LabelMatcher_EQ, Value: "1"}, {Name: "pod_ns", Type: prompb.LabelMatcher_EQ, Value: "3"}},
929-
{{Name: "pod", Type: prompb.LabelMatcher_EQ, Value: "5"}, {Name: "pod_ns", Type: prompb.LabelMatcher_EQ, Value: "3"}},
930-
},
931-
}}
925+
input string
926+
wantSQL string
927+
}{
928+
{
929+
// AND/OR tree preserved: A AND B AND (C OR D) stays as-is, no DNF expansion
930+
input: "((pod_cluster_id in (2) AND vpc_id IN (1) AND (chost_id IN (1,2,3,4,5,6) OR exist(chost_id))))",
931+
wantSQL: "((pod_cluster_id in (2) AND vpc_id in (1)) AND (chost_id in (1, 2, 3, 4, 5, 6) OR exist(chost_id)))",
932+
},
933+
{
934+
input: "((pod_cluster_id in (2) AND vpc_id IN (1) AND (chost_id IN (1,2,3,4,5,6) OR not exist(chost_id))))",
935+
wantSQL: "((pod_cluster_id in (2) AND vpc_id in (1)) AND (chost_id in (1, 2, 3, 4, 5, 6) OR not exist(chost_id)))",
936+
},
937+
{
938+
// explicit parentheses with OR
939+
input: "(pod_id=1 and pod_ns_id=2) or (pod_id=1 and pod_ns_id=3) or (pod_id=5 and pod_ns_id=3)",
940+
wantSQL: "(((pod_id = 1 AND pod_ns_id = 2) OR (pod_id = 1 AND pod_ns_id = 3)) OR (pod_id = 5 AND pod_ns_id = 3))",
941+
},
942+
{
943+
// AND binds tighter than OR without parentheses
944+
input: "pod_id=1 and pod_ns_id=2 or pod_id=5 and pod_ns_id=3",
945+
wantSQL: "((pod_id = 1 AND pod_ns_id = 2) OR (pod_id = 5 AND pod_ns_id = 3))",
946+
},
947+
{
948+
// simple OR chain
949+
input: "pod_id=1 or pod_id=2 or pod_id=3",
950+
wantSQL: "((pod_id = 1 OR pod_id = 2) OR pod_id = 3)",
951+
},
952+
{
953+
// left side with parens, right side plain
954+
input: "(pod_id=1 and pod_ns_id=2) or pod_id=5",
955+
wantSQL: "((pod_id = 1 AND pod_ns_id = 2) OR pod_id = 5)",
956+
},
957+
}
932958
t.Run("ParseExFilters", func(t *testing.T) {
933-
for i := 0; i < len(testCases); i++ {
934-
output, err := parseExtraFiltersToMatchers(testCases[i].input)
959+
for i, tc := range testCases {
960+
tree, err := parseExtraFiltersToMatchers(tc.input)
935961
if err != nil {
936-
t.Errorf("Expected output: %v, got: %v", testCases[i].output, err)
937-
} else {
938-
outerIndex := len(testCases[i].output)
939-
for j := 0; j < outerIndex; j++ {
940-
innerIndex := len(testCases[i].output[j])
941-
for k := 0; k < innerIndex; k++ {
942-
assert.Equal(t, testCases[i].output[j][k].Name, output[j][k].Name)
943-
assert.Equal(t, testCases[i].output[j][k].Value, output[j][k].Value)
944-
assert.Equal(t, testCases[i].output[j][k].Type, output[j][k].Type)
945-
}
946-
}
962+
t.Errorf("case %d: unexpected error: %v", i, err)
963+
continue
947964
}
965+
got := p.parseExtraFiltersToWhereClause(tree, prefixNone, "", noopTags)
966+
assert.Equal(t, tc.wantSQL, got, "case %d", i)
948967
}
949968
})
950969
}

0 commit comments

Comments
 (0)