Support case insensetive logql keywords #10733
Conversation
shantanualsi
force-pushed
the
case-insensetive-logql
branch
from
089592e
to
f52e2be
Compare
| @@ -68,13 +68,16 @@ func TestLex(t *testing.T) { | |||
| {`count_over_time({foo="bar"}[5m])`, []int{COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, RANGE, CLOSE_PARENTHESIS}}, | |||
| {`count_over_time({foo="bar"} |~ "\\w+" | unwrap foo[5m])`, []int{COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, PIPE_MATCH, STRING, PIPE, UNWRAP, IDENTIFIER, RANGE, CLOSE_PARENTHESIS}}, | |||
| {`sum(count_over_time({foo="bar"}[5m])) by (foo,bar)`, []int{SUM, OPEN_PARENTHESIS, COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, RANGE, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, COMMA, IDENTIFIER, CLOSE_PARENTHESIS}}, | |||
| {`SUM(Count_Over_Time({foo="bar"}[5m])) BY (foo,bar)`, []int{SUM, OPEN_PARENTHESIS, COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, RANGE, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, COMMA, IDENTIFIER, CLOSE_PARENTHESIS}}, | |||
There was a problem hiding this comment.
should we only support either all caps or all nocaps?
There was a problem hiding this comment.
hmm.. not sure about this nuance! need to check with @dannykopping
There was a problem hiding this comment.
I can't think of a reason why we'd want to enforce a single case over the whole function.
Stylistically it's ¯\_(ツ)_/¯ if folks waNt_To_qUeRy_liKE_tHIS() but at least it's not case-sensitive.
| @@ -226,7 +228,7 @@ func (l *lexer) Lex(lval *exprSymType) int { | |||
| } | |||
| } | |||
|
|
|||
| if tok, ok := functionTokens[tokenText]; ok { | |||
| if tok, ok := functionTokens[tokenTextLower]; ok { | |||
There was a problem hiding this comment.
isFunction also needs a check for by/without tokens
There was a problem hiding this comment.
can you explain what you mean here exactly? they should be covered (see test case line 71)
There was a problem hiding this comment.
sorry I wasn't clear. I think isFunction would return false for SUM BY (foo) since we only check against lower case by and without
Line 396 in 4c3e8f5
There was a problem hiding this comment.
good catch! ⚾️
There was a problem hiding this comment.
fixed this and added a test case to cover this. please verify.
| @@ -215,7 +215,9 @@ func (l *lexer) Lex(lval *exprSymType) int { | |||
| } | |||
|
|
|||
| tokenText := l.TokenText() | |||
| tokenNext := tokenText + string(l.Peek()) | |||
| tokenTextLower := strings.ToLower(l.TokenText()) | |||
| tokenNext := strings.ToLower(tokenText + string(l.Peek())) | |||
There was a problem hiding this comment.
i don't know why we peek here for functions, next token might also need to be covered I think
There was a problem hiding this comment.
not sure either, I was also wondering about it.. I just added the toLower func to the existing implementation
**What this PR does / why we need it**: Adds support for case-insensetive logql queries that fixes grafana#10638. **Which issue(s) this PR fixes**: grafana#10638 **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [x] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e)
What this PR does / why we need it:
Adds support for case-insensetive logql queries that fixes #10638.
Which issue(s) this PR fixes:
#10638
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)CHANGELOG.mdupdatedadd-to-release-noteslabeldocs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PR