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(stdlib): human time parse duration error for decimals #1223

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sainad2222
Copy link

@sainad2222 sainad2222 commented Jan 21, 2025

Credits to @titaneric for original implementation

Summary

New humantime parse duration function doesn't support decimals. So falling back to old method(regex) when ht_parse_duration fails

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

cargo test parse_duration
./scripts/checks.sh

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on
    our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Our CONTRIBUTING.md is a good starting place.
  • If this PR introduces changes to LICENSE-3rdparty.csv, please
    run dd-rust-license-tool write and commit the changes. More details here.
  • For new VRL functions, please also create a sibling PR in Vector to document the new function.

References

@sainad2222 sainad2222 marked this pull request as draft January 21, 2025 16:21
@sainad2222 sainad2222 force-pushed the fix_parse_duration_22255 branch from 334f9b6 to 03f803c Compare January 21, 2025 16:26
@sainad2222 sainad2222 marked this pull request as ready for review January 21, 2025 16:30
@pront pront self-assigned this Jan 21, 2025
@titaneric
Copy link
Contributor

Thank you for trying for fixing this! Is it possible to support multi-units duration in the implementation as well?
In fact, I have the previous implementation here.

@pront
Copy link
Member

pront commented Jan 22, 2025

Thank you for trying for fixing this! Is it possible to support multi-units duration in the implementation as well? In fact, I have the previous implementation here.

Good idea, it could have been in a separate function but I am fine to include in this one as long as we include some more tests 👍

@sainad2222
Copy link
Author

Test cases for multi units duration are already present ig. Any sample test case? Just wanted to understand what kind of new test cases needs to be added just so that we are on the same page

@titaneric
Copy link
Contributor

Perhaps testcase for multi-unit and fractional time duration?

@sainad2222
Copy link
Author

Ahh got it

@sainad2222
Copy link
Author

Added. Thanks @titaneric for the logic

@jszwedko
Copy link
Member

@titaneric if we are adding support to the existing implementation to handle multiple units, is there any reason to keep the implementation using humantime around? Does it offer any other advantages that would motivate supporting both implementations?

@titaneric
Copy link
Contributor

I am not sure, to be honest.
I think if we need to handle fractional and multi-units duration parsing, we need to handle by ourselves.

I think we could remove the humantime crate this time.

Comment on lines -226 to +277
want: Err("unknown unit format: 'w'"),
want: Ok(0.000_001_653_439_153_439_153_5),
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if we want to support weeks. Either we remove support for both w_ns and s_w or keep it as it is. Let me your thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

I think week is a common unit, and I think we could support it

@jszwedko
Copy link
Member

I think if we need to handle fractional and multi-units duration parsing, we need to handle by ourselves.

I agree, it seems like we need to handle the parsing ourselves.

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.

Vector 0.44.0 no longer support decimal in parse_duration
4 participants