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

Update getKustoDateTime with more lenient formatter #246

Merged
merged 4 commits into from
May 24, 2022

Conversation

breriksenms
Copy link
Contributor

Pull Request Description

The getKustoDateTime function uses 2 potential formatters for the LocalDateTime that either include no digits for the fraction of a second or 7 digits for the fraction of a second. We ran into a situation where the value coming back from ADX had a different number of digits and would cause this method to fail. This code updates that function to just use the built-in ISO_LOCAL_DATE_TIME formatter, which allows for 0-9 digits in the fraction of a second, and appends the literal 'Z' to the end of the formatter to account for that value being present in the datetimes that ADX sends back.


Future Release Comment

Updating the getKustoDateTime function to use the built-in ISO_LOCAL_DATE_TIME formatter, which allows for 0-9 digits in the fraction of a second, and append the literal 'Z' to the end of the formatter to account for that value being present in the datetimes that ADX sends back.

Breaking Changes:

  • None

Features:

  • None

Fixes:

  • Use a more lenient LocalDateTime formatter for parsing in getKustoDateTime

@xatier
Copy link

xatier commented Apr 29, 2022

Related to #241

Copy link
Collaborator

@ohadbitt ohadbitt left a comment

Choose a reason for hiding this comment

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

Nice!

@yogilad
Copy link
Contributor

yogilad commented May 17, 2022

We need to add additional tests for fractions and partial values before completing this PR.
If the existing formatters are not required, they need to be deleted.
Also, there are additional formats which are (probably) used for user input which do not support timezone notation

@breriksenms
Copy link
Contributor Author

Do you have examples of the different datetime formats that you would want this method to support?

@AsafMah
Copy link
Contributor

AsafMah commented May 19, 2022

@breriksenms This PR replaces both KUSTO_DATETIME_PATTERN_NO_FRACTIONS and KUSTO_DATETIME_PATTERN
Can you test it still works with both formats at least?

Also KUSTO_DATETIME_FORMATS in CslDateTimeFormat still doesn't seem to support timezones.

@breriksenms
Copy link
Contributor Author

I added new tests for the getKustoDateTime function that covers the different formats it supports. I'm not certain what you want to do with the KUSTO_DATETIME_FORMATS and timezones. I think that's more for sending a datetime out to Kusto, not for receiving it. Kusto only sends back datetimes in UTC timezone. The original getKustoDateTime function only supported UTC and then only in 2 formats. This PR just expands it to support a variable number of digits in the fraction of a second.

@AsafMah
Copy link
Contributor

AsafMah commented May 24, 2022

Ok, looks good!

@AsafMah AsafMah merged commit 2209318 into Azure:master May 24, 2022
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.

5 participants