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.

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

added regexp.MatchString to handle all test cases #14081

Closed
wants to merge 24 commits into from

Conversation

kushalShukla-web
Copy link
Contributor

@kushalShukla-web kushalShukla-web commented May 11, 2024

still need to work on regexp_test.go file . there are doubts that i will clarify with maintainers

@kushalShukla-web
Copy link
Contributor Author

kushalShukla-web commented May 11, 2024

@ArthurSens can you please review the code and give me any insight :) . like here we should use regexp instead of string.EqualFold right ?

@colega
Copy link
Contributor

colega commented May 11, 2024

Please, add the test case mentioned on the issue to prove that you're actually solving the issue.

return ok
}
for k := range m.values {
val, _ := regexp.MatchString(k, s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we forget about performance, I don't think this would work, as k is a literal and regexp.MatchString doesn't do case-insensitive by default.

The strings.EqualFold call would work here, and most likely more performant than a regexp, but what's the point in running a loop of regexp calls here if the entire objective of FastRegexpMatcher is to avoid a single regexp call.

If we can afford that single regexp call, we can just evaluate the original regexp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. As strings.EqualFold is used for two strings are equal or not in case sensitive way . its not used for checking the regular expression . so i dont think we can use strings.EqualFold here as its only checking for two equal string .
  2. also on line 788 we are checking for if !m.caseSensitive { s = strings.ToLower(s) }
    caseSensitive test cases . so if we have some uppercase or upper and lower case test cases then , strings.ToLower(s) will convert them into lowercase right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

its not used for checking the regular expression

equalMultiStringMapMatcher.values does not contain regular expressions, but just the strings.

so if we have some uppercase or upper and lower case test cases then , strings.ToLower(s) will convert them into lowercase right ?

But unicode has more than just uppercase and lowercase. Please, add the test case speicified in the issue and run it with a debugger setting a breakpoint on that condition on line 788 to get a better understanding of the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yeah i corrected my self here , actually i thought it contains the regular expression ,but it did't . so basically what im assume here is that the regular expression is some how stored in a simple string formate right .
  2. Also do i need to add the test case "ſſs" on the given regexes slice ?
  3. Here how did you manage to understand the 1000 lines of code😅 , like apart from this TestFastRegexMatcher_MatchString i really dont understand what other test functions are doing .

@kushalShukla-web
Copy link
Contributor Author

Hey i think im some how wrong , actually i dont understand that how this function TestFastRegexMatcher_MatchString is related to func (m *equalMultiStringMapMatcher) Matches . i think its some how related to NewFastRegexMatcher .

@colega
Copy link
Contributor

colega commented May 12, 2024

Note that NewFastRegexMatcher analyzes the regular expression and tries to convert it into a set of simplier matchers if it's possible. One of those cases is when the regular expression is just an enumeration of strings, like foo|bar, in which case equalMultiStringMapMatcher is instantiated, with a separate flag to indicate whether the regular expression had the case-insensitive ?i flag.

kushalShukla-web and others added 24 commits May 13, 2024 01:58
…eus#14015)

The check fell into "this matcher equals vector selector's name" case when vector selector doesn't have a name and the matcher is an explicit matcher for an empty __name__ label.

To provide some context about why this is important: some downstream projects use the promql.Parse(expr.String()) to clone an expression's AST, and with this bug that matcher disappears in the cloning.

Signed-off-by: Oleg Zaytsev <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
When `zeroOrOneCharacterStringMatcher` wach checking the input string,
it assumed that if there are more than one bytes, then there are more
than one runes, but that's not necessarily true.

Signed-off-by: Oleg Zaytsev <[email protected]>
…tor (prometheus#14021)

The function `rangeEvalTimestampFunctionOverVectorSelector` appeared to be checking histogram size, however the value it used was always 0 due to subtle variable shadowing.
However we don't need to pass sample values to the `timestamp` function, since the latter only cares about timestamps. This also affects peak sample count in statistics, since we are no longer copying histogram samples.

Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
To facilitate generating OTel translation code for other Prometheus
compatible backends, modify the prometheusremotewrite sources slightly
so that the PrometheusConverter.TimeSeries method is in a file called
timeseries.go. The rationale is to allow other backends to define their
own implementation of this method.

Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Fix language in docs and comments

---------

Signed-off-by: Arve Knudsen <[email protected]>
Co-authored-by: Björn Rabenstein <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Add method `PostingsForLabelMatching` to `tsdb.IndexReader`, to obtain postings for labels with a certain name and values accepted by a provided callback, and use it from `tsdb.PostingsForMatchers`.
The intention is to optimize regexp matcher paths, especially not having to load all label values before matching on them.

Plus tests, and refactor some `tsdb/index.Reader` methods.

Benchmarking shows memory reduction up to ~100%, and speedup of up to ~50%.

Signed-off-by: Arve Knudsen <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
To avoid a circular reference between promql and promqltest.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
This saves having a function solely to call kahanSumInc.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
So that promql package does not bring in test-only dependencies.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Lint started complaining after I moved the file.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
So it nearly compiles.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
And export `DefaultMaxSamplesPerQuery` so callers can replicate previous
behaviour.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
So that tests can call it from another package.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
To packages outside of promql.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
So that they can import promqltest which imports promql.

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
* set enablePerStepStats and lookback duration via
  `NewTestEngine` parameters.
* check maxSamples by recreating query engine
* check lookback without modifying internals

Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
…ackage (prometheus#13932)

Signed-off-by: Anthony J Mirabella <[email protected]>
Signed-off-by: Jesus Vazquez <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Signed-off-by: kushagra Shukla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants