adding more date types #5444
adding more date types #5444
Conversation
supporting more date32 types adding tests
|
I choose to use regex for multi-match, could it cause much efficiency decrease? |
|
Have you run the date parsing benchmarks against this change, I would expect the code in this PR to represent a fairly significant performance regression |
could you please tell me how to trigger it in CI/CD ? |
We don't have automated benchmarks in CI/CD at the moment, but they can be run with In this case something like this should work |
Got it, I will test it now, Thanks a lot. |
Yeah, that regressed a lot, I will find another way to do it |
|
Marking as a draft for now, feel free to mark ready for review when you would like me to take another look |
| return None; | ||
| } | ||
| (digits[5], digits[7] * 10 + digits[8]) | ||
| use regex::Regex; |
There was a problem hiding this comment.
You'll want to precompile / reuse the regex. That might return most of the performance (although manual implementation probably would still be more efficient).
There was a problem hiding this comment.
Yeah I'd expect using a regex, particularly with capture groups, to be slower than something hand-rolled here.
There was a problem hiding this comment.
Thanks, I'll add some hand-rolled next week
|
@BurntSushi was looking at / thinking about datetime stuff in Rust. Tagging him in case there's room for collab / idea sharing. |
There was a problem hiding this comment.
@BurntSushi was looking at / thinking about datetime stuff in Rust. Tagging him in case there's room for collab / idea sharing.
Was there something specific you had in mind?
| (digits[5], digits[7] * 10 + digits[8]) | ||
| use regex::Regex; | ||
| let re = Regex::new( | ||
| r"^(?:(\d{1,2})[/\-](\d{1,2})[/\-](\d{1,4})|(\d{1,4})[/\-](\d{1,2})[/\-](\d{1,2}))$", |
There was a problem hiding this comment.
Are you sure you want \d here? \d is Unicode-aware by default. So in addition to matching 3, for example, it will also match 𝟛.
| return None; | ||
| } | ||
| (digits[5], digits[7] * 10 + digits[8]) | ||
| use regex::Regex; |
There was a problem hiding this comment.
Yeah I'd expect using a regex, particularly with capture groups, to be slower than something hand-rolled here.
@BurntSushi Not particularly 😅 |
I'm working on a new datetime library. I'm still in the "proving it out stage" and I don't know if it will work out. But if it does, it's months away. |
|
I think if you have specific questions, I'd be happy to try and field them, but otherwise I don't really know anything about what y'all are doing. |
supporting more date32 types
adding tests
Which issue does this PR close?
Closes #5442
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?