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.

Sign up for GitHub

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

Jump to bottom

Je/fix timestamp #6862

Merged
merged 3 commits into from
Aug 8, 2023
Merged

Je/fix timestamp #6862

merged 3 commits into from
Aug 8, 2023

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Aug 4, 2023

What, How & Why?

Fixes #6855

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@cla-bot cla-bot bot added the cla: yes label Aug 4, 2023
@jedelbo jedelbo marked this pull request as draft August 6, 2023 11:10
@jedelbo jedelbo force-pushed the je/fix-timestamp branch 2 times, most recently from ffe8e7b to a322b00 Compare August 7, 2023 09:15
@jedelbo jedelbo marked this pull request as ready for review August 7, 2023 09:51
@jedelbo jedelbo requested a review from nicola-cab August 7, 2023 09:51
@jedelbo jedelbo force-pushed the je/fix-timestamp branch 2 times, most recently from 05b1f35 to 96b1fd7 Compare August 7, 2023 13:07
nicola-cab
nicola-cab reviewed Aug 8, 2023
View reviewed changes
Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

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

Very few minor comments. Also, lint build is complaining.

test/test_json.cpp
strftime(buffer2, sizeof(buffer2), "%Y-%m-%d %H:%M:%S", &buf);
CHECK(strcmp(buffer1, buffer2) == 0);
}
/*
Copy link
Member

Choose a reason for hiding this comment

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

Is this forgotten?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. If you want to test the performance of the new implementation against the old implementation this could come in handy.

src/realm/util/serializer.cpp Outdated
int secs = remainingSeconds - minutes * 60;

julian_to_date(julian_days, &year, &month, &day);
// const char* format = (year < 0) ? "%05d-%02d-%02d %02d:%02d:%02d" : "%04d-%02d-%02d %02d:%02d:%02d";
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be deleted

test/test_json.cpp
@@ -24,6 +24,7 @@
#include <string>
#include <fstream>
#include <ostream>
#include <chrono>
Copy link
Member

Choose a reason for hiding this comment

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

Probably this include is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well - if you would like to run the performance testing it is.

test/test_upgrade_database.cpp
@@ -757,7 +757,7 @@ TEST_IF(Upgrade_Database_9_10, REALM_MAX_BPNODE_SIZE == 4 || REALM_MAX_BPNODE_SI
if (REALM_MAX_BPNODE_SIZE == 1000) {
auto sg = DB::create(*hist, temp_copy);
if (generate_json) {
std::ofstream expect(test_util::get_test_path_prefix() + "expect_test_upgrade_database_9_to_10.json");
std::ofstream expect(test_util::get_test_path_prefix() + "/expect_test_upgrade_database_9_to_10.json");
Copy link
Member

Choose a reason for hiding this comment

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

Why this / here? Isn't what we do in bool initialize_test_path(int argc, const char* argv[]) not sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently the prefix did not include the terminating '/'

@jedelbo
Improve convertion of Timestamp to string
cd4a6c8 
Use Fliegel & Van Flandern algorithm
@jedelbo
Copy link
Contributor Author

jedelbo commented Aug 8, 2023

lint is complaining on the formatting of the Json files. I will not fix that. They are formatted like jq likes it.

@jedelbo jedelbo requested a review from nicola-cab August 8, 2023 12:11
@jedelbo jedelbo merged commit 4876aff into master Aug 8, 2023
2 checks passed
@jedelbo jedelbo deleted the je/fix-timestamp branch August 8, 2023 13:35
@jedelbo jedelbo mentioned this pull request Aug 9, 2023
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nicola-cab nicola-cab nicola-cab approved these changes

Assignees

@jedelbo jedelbo

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change json serialization format of realm timestamps to follow ISO 8601
2 participants
@jedelbo @nicola-cab