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

fix:class 'ResultSetUtil.java' parse datetime type error #240 #241

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

bobobod
Copy link

@bobobod bobobod commented Oct 7, 2023

No description provided.

@wey-gu
Copy link
Member

wey-gu commented Oct 7, 2023

I had put this fix PR as hacktoberfest, check https://bit.ly/hack-nebula on how to participate :)

@codecov-commenter
Copy link

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (d6b73ea) 0.00% compared to head (d10eaa4) 0.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #241   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          74      74           
  Lines        2532    2533    +1     
  Branches      275     275           
======================================
- Misses       2532    2533    +1     
Files Coverage Δ
...rg/nebula/contrib/ngbatis/utils/ResultSetUtil.java 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -126,27 +127,28 @@ public static <T> T getValue(ValueWrapper valueWrapper, Class<T> resultType) {
}

private static Object transformDateTime(DateTimeWrapper dateTime) {
DateTime localDateTime = dateTime.getLocalDateTime();
Copy link
Contributor

@Nicole00 Nicole00 Oct 11, 2023

Choose a reason for hiding this comment

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

can we give some description about Why convert the datetime to localDateTime?
If i am using other zone but not utc8, is there any problems with the results?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we give some description about Why convert the datetime to localDateTime? If i am using other zone but not utc8, is there any problems with the results?

It seems like this.

#240

Copy link
Author

Choose a reason for hiding this comment

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

You'd better test it locally.when the zone is GMT+8, this method of converting time will ultimately result in 8 hours less than expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My nebula-storaged has been unable to start recently and lacks a cross time zone testing environment, so it has been unable to verify. T^T

Copy link
Author

Choose a reason for hiding this comment

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

image

Copy link
Author

Choose a reason for hiding this comment

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

image
This looks like the data returned after calling the service is correct, but after conversion, an error occurred

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the time zone of the machine where the nebula service is located?

Copy link
Author

Choose a reason for hiding this comment

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

it‘s deployed on a virtual machine, the time zone is correct.

Copy link
Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the point, use LocalDateTime can get the correct time info according to the local zone.

Copy link
Contributor

@Nicole00 Nicole00 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@CorvusYe CorvusYe left a comment

Choose a reason for hiding this comment

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

LGTM

@CorvusYe CorvusYe merged commit 5e75595 into nebula-contrib:master Oct 23, 2023
1 check passed
@CorvusYe CorvusYe mentioned this pull request Nov 14, 2023
@CorvusYe CorvusYe mentioned this pull request Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants