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

Change timestamp fieldType to be nullable #184

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

nekketsuuu
Copy link
Contributor

@nekketsuuu nekketsuuu commented Jul 30, 2022

#162

Since datumParserTimestamp can return nil value, fieldType also should allow nil.
Without this, nil timestamp will be recognized as a strange timestamp, such as 1754-08-30 UTC (-6795364578871).

Follow-up: 4750ff8

Screenshots

Beforebefore
Afterafter

The second record contains a null value in clock measure, which is displayed as 1754-08-31 in the first screenshot and as an empty cell in the second screenshot.

How to reproduce these screenshots

  1. Insert above data points to a newly created table. I used this Ruby script: https://gist.github.com/nekketsuuu/c84479128ca9487be7c018c741732be1
  2. Create a Table panel on Grafana, with a query select * from $__database.$__table where $__timeFilter.

Tests

Currently I marked this pull request as draft because I couldn't update test files. It's difficult to me to update them for a reason described by #183. I'd appreciate if someone would tell me how to regenerate golden files from scratch.

Solved by 9e24fb7

Since datumParserTimestamp can return nil value, fieldType also should allow nil.
Without this, nil timestamp will be recognized as a strange timestamp, such as 1754-08-30 22:43:41 UTC (-6795364578871).

Follow-up: 4750ff8
@CLAassistant
Copy link

CLAassistant commented Jul 30, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sunker
Copy link
Collaborator

sunker commented Sep 20, 2022

@nekketsuuu I have now re-generated the test files for you. Sorry it's been taking such a long time. Let us know when this PR is ready for review and we'll take a look. Thanks!

@nekketsuuu nekketsuuu marked this pull request as ready for review September 20, 2022 14:50
@nekketsuuu nekketsuuu requested a review from a team as a code owner September 20, 2022 14:50
@nekketsuuu
Copy link
Contributor Author

@sunker Awesome! Thank you for looking my pull request! I checked your changes and marked this pull request is ready for review.

@sunker sunker changed the title fix: Change timestamp fieldType to be nullable Change timestamp fieldType to be nullable Sep 22, 2022
Copy link
Collaborator

@sunker sunker left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for contributing @nekketsuuu!

@sunker sunker merged commit 71cd89f into grafana:main Sep 22, 2022
@nekketsuuu nekketsuuu deleted the nekketsuuu-null-timestamp branch September 22, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants