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

The Remap parse_timestamp function format argument should be optional #5496

Closed
binarylogic opened this issue Dec 11, 2020 · 4 comments
Closed
Assignees
Labels
domain: parsing Anything related to parsing within Vector domain: vrl Anything related to the Vector Remap Language type: enhancement A value-adding code change that enhances its existing functionality.

Comments

@binarylogic
Copy link
Contributor

binarylogic commented Dec 11, 2020

I think this is already implemented in the to_timestamp function. To make timestamp parsing easier, we should do what we currently do in our legacy types conversion code:

https://github.com/timberio/vector/blob/2ab17c8ac514211d5579d16821b5e47c050c745b/src/types.rs#L205-L228

That is, automatically detect and parse common formats.

@binarylogic binarylogic added type: enhancement A value-adding code change that enhances its existing functionality. domain: parsing Anything related to parsing within Vector domain: vrl Anything related to the Vector Remap Language labels Dec 11, 2020
@JeanMertz
Copy link
Contributor

If we do this, there isn't much that separates to_timestamp from parse_timestamp any more. Should we consolidate the two?

@binarylogic
Copy link
Contributor Author

Yes, I think so, although to_timestamp handles integers.

@JeanMertz
Copy link
Contributor

JeanMertz commented Dec 13, 2020

If we'd go with vectordotdev/vrl#145, I'd personally favour:

  • to_timestamp(.foo, default = "2020/10/21") (together with to_string, etc)
  • timestamp::parse(.foo, format = "%s") (for this one I'd make the format optional, and remove the default parameter)

@binarylogic
Copy link
Contributor Author

Actually, I'm going to close this. I think the current behavior is correct:

  • to_timestamp is automatic, intended for simple conversions.
  • parse_timestamp is specific to a custom format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: parsing Anything related to parsing within Vector domain: vrl Anything related to the Vector Remap Language type: enhancement A value-adding code change that enhances its existing functionality.
Projects
None yet
Development

No branches or pull requests

4 participants