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

fix: Store validateArgs wrongfully overwriting start, end unix time #24741

Closed
wants to merge 3 commits into from

Conversation

paulheg
Copy link

@paulheg paulheg commented Mar 7, 2024

When querying data before 1970-01-01 (UNIX time 0) validateArgs would set start to -in64 max and end to int64 max.

Closes #24669

Changes made:
Removed two faulty if statements.

  • I've read the contributing section of the project README.
  • Signed CLA (if not already signed).

When querying data before 1970-01-01 (UNIX time 0) validateArgs would set start to -in64 max and end to int64 max.
@paulheg paulheg changed the title fix: Store validateArgs wronfully overwriting start, end unix time fix: Store validateArgs wrongfully overwriting start, end unix time Mar 7, 2024
@philjb
Copy link
Contributor

philjb commented Mar 7, 2024

@gwossum - if you have a moment, could you look at this potential bug fix from @paulheg? See the bug report for details. The PR does seem to have targeted the impact code where it is replacing any timestamp less than or equal to 0 aka 1970-01-01 with the smallest signed int (plus 2).

I think the code was intending to do this (similar issue for end)

	if start < models.MinNanoTime {
		start = models.MinNanoTime
	}

as math.MinInt64 and math.MinInt64 + 1 are apparently used as sentinels. @paulheg - I think you need to adjust your fix. Check out the comment on models.MinNanoTime.

This bug seems to be encountered only for queries looking before 1970 so it is rather unlikely to be encountered in the wild, except for @paulheg here!

…ls.MinNanoTime or higher than models.MaxNanoTime
@paulheg
Copy link
Author

paulheg commented Mar 7, 2024

@philjb with the comment of models.MinNanoTime it makes a lot more sense what was going on there.
I updated the pr.

@paulheg
Copy link
Author

paulheg commented Mar 7, 2024

I just added some tests to check this behaviour.

@paulheg
Copy link
Author

paulheg commented Jul 12, 2024

Is there anything blocking us from merging this?
The builds are only failing because of timeouts downloading the dependencies.

@davidby-influx davidby-influx self-requested a review July 12, 2024 18:30
@davidby-influx davidby-influx added area/flux Issues related to the Flux query engine team/edge labels Jul 12, 2024
@davidby-influx davidby-influx self-assigned this Jul 12, 2024
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@davidby-influx
Copy link
Contributor

Cherry-picked to #25146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flux Issues related to the Flux query engine team/edge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants