In short, floating-point multiplication and floor rounding together caused this bug. So I fixed the conversion from numLit.Val float64 to rangeNl time.Duration.

Below is a simple example explaining everything.

package main

import (
	"fmt"
	"math"
	"time"
)

func OutdatedImpl(inSecondDuration float64) time.Duration {
	return time.Duration(inSecondDuration*1000) * time.Millisecond
}

func UpdatedImpl(inSecondDuration float64) time.Duration {
	return time.Duration(math.Round(inSecondDuration * float64(time.Second)))
}

func main() {
	inSecondDurations := []float64{1.000, 1.001, 1.002}
	var duration time.Duration
	for _, inSecondDuration := range inSecondDurations {
		duration = OutdatedImpl(inSecondDuration)
		fmt.Printf("[outdated] duration in s: %v, in ms: %v, in ns: %v\n", inSecondDuration, duration.Milliseconds(), duration.Nanoseconds())
		duration = UpdatedImpl(inSecondDuration)
		fmt.Printf("[updated] duration in s: %v, in ms: %v, in ns: %v\n\n", inSecondDuration, duration.Milliseconds(), duration.Nanoseconds())
	}
}

[outdated] duration in s: 1, in ms: 1000, in ns: 1000000000
[updated] duration in s: 1, in ms: 1000, in ns: 1000000000

[outdated] duration in s: 1.001, in ms: 1000, in ns: 1000000000
[updated] duration in s: 1.001, in ms: 1001, in ns: 1001000000

[outdated] duration in s: 1.002, in ms: 1002, in ns: 1002000000
[updated] duration in s: 1.002, in ms: 1002, in ns: 1002000000

Nice catch.
I don't have the full context, but from skimming the parser, I’m not
sure why these values are initially parsed as seconds

machine424

+1, the loss of precision is introduced by dur.Seconds().

It seems like I should ask the author of this code block about the considerations for converting to seconds instead of milliseconds?

I think the changes were made in #9138
it seems that a regression was introduced there. The tests you added in promql/parser/parse_test.go pass on release-2.53 (I didn't try the other tests)

It is converted to seconds because when using a plain number in Prometheus, it is considered to be seconds.

It is converted to seconds because when using a plain number in Prometheus, it is considered to be seconds.

But 1001ms isn't a plain number, as I mentionedin #16478 (comment), this wasn't the case before #9138

I'm not a big fan of "let's parse all numbers the same way, but let's have some special logic for durations", it even goes against #9138.
I think parsing should be aware of this and take care of it.

#9138 makes them all similar. What would you propose as a fix?

I believe the fix could also be to introduce a .Duration() time.Duration to NumberLiteral and use it when we need it.

I haven't looked closely at the code, so I can't provide a complete/working solution. That said, I do think it's a pity that we lose such info/precision during parsing. I was wondering in #16478 (comment) we could just return durations in ms instead of s in some cases. Or even for all cases given we want ms precision, and then make sure the ms->s is done correctly when needed (I don't see why we'd need that, if we only work with ms)

For now this is a correct fix for the issue. I think we could change to Milliseconds but at the expense of potentially breaking downstream users, do I am happy with this PR as it.

Thanks!

Hi, I was rewording descriptions for the changelog to make them more useful for end-users, so I expanded the title of this PR.

I did not understand what "function selector" was supposed to mean, so dropped the word "function".

Something like foo[1m] is named a "range vector selector" in the docs.

Hi, I was rewording descriptions for the changelog to make them more useful for end-users, so I expanded the title of this PR.

I did not understand what "function selector" was supposed to mean, so dropped the word "function".

Something like foo[1m] is named a "range vector selector" in the docs.

@bboreham Thanks! Your revision is spot on.