Skip to content

whisper-fetch: fix timestr when using --drop #342

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

Merged
merged 1 commit into from
Jun 27, 2025

Conversation

tmzullinger
Copy link
Contributor

Filtering the values list when --drop is set happens too early for the non-json output. When looping over the values we expect to be able to set the time string based on the number of times through the loop, adding the step value on each pass. With empty values stripped, the time gets further off for each value that is dropped.

Move the drop filtering function to the appropriate places for both json and plain test output.

Testing on databases with more than a year of data and many empty values, the time to execute is no different, so we should not be regressing in any significant way. Though even if we were, it is better to be accurate than fast if one or the other must be picked.

Fixes #250, fixes #305.

@tmzullinger
Copy link
Contributor Author

tmzullinger commented Feb 19, 2025

This more or less duplicates PR #306. I did the work before I looked through the issues and PR's.

I don't much care which one is merged, though I think this is a slightly smaller and more concise change (but am obviously biased).

Apart from the whitespace changes to adjust the indent in the non-json loop, the diffstat is 3 insertions(+), 1 deletion(-).

Filtering the values list when --drop is set happens too early for the
non-json output.  When looping over the values we expect to be able to
set the time string based on the number of times through the loop,
adding the step value on each pass.  With empty values stripped, the
time gets further off for each value that is dropped.

Move the drop filtering function to the appropriate places for both json
and plain test output.

Testing on databases with more than a year of data and many empty
values, the time to execute is no different, so we should not be
regressing in any significant way.  Though even if we were, it is better
to be accurate than fast if one or the other must be picked.

Fixes graphite-project#250, fixes graphite-project#305.
@tmzullinger tmzullinger force-pushed the fetch-fix-drop-times branch from 9538637 to f215ce3 Compare March 15, 2025 15:03
Copy link

stale bot commented Jun 27, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Jun 27, 2025
@tmzullinger
Copy link
Contributor Author

This is still relevant. If the change in #306 is used then this can be closed. But one or the other would be nice to have merged.

@deniszh deniszh merged commit 94c3a61 into graphite-project:master Jun 27, 2025
1 of 2 checks passed
@deniszh
Copy link
Member

deniszh commented Jun 27, 2025

Sure, missed that smh. Merged.

@tmzullinger
Copy link
Contributor Author

Thanks!

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.

[BUG] whisper-fetch.py --drop incorrect timestamps drop nulls and drop zeros returns incorrect timestamp
2 participants