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

Add Scan method for ResultSet to scan the rows into the given slice (Part 1) #298

Merged
merged 4 commits into from Jan 11, 2024

Conversation

haoxins
Copy link
Collaborator

@haoxins haoxins commented Dec 19, 2023

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

#296

Description:

This PR includes changes in #297,
I add the first draft implementation for the API proposed in #296.

Some tasks need to be done, such as

  • Support all the fundamental types;
  • Support nest structure type;
  • Add more tests.

But I raised this PR as a draft in case there are some potential concerns from your guys.
So feel free to leave some comments here.

Such as can we drop support for Go versions that < 1.18?

cc @wey-gu @veezhang

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

@haoxins haoxins marked this pull request as ready for review December 27, 2023 05:50
@haoxins haoxins changed the title Add Scan method for ResultSet to scan the rows into the given slice Add Scan method for ResultSet to scan the rows into the given slice (Part 1) Dec 27, 2023
@haoxins
Copy link
Collaborator Author

haoxins commented Dec 27, 2023

@wey-gu @veezhang Any thoughts?

@Nicole00
Copy link
Contributor

Nicole00 commented Jan 5, 2024

some ci failures need to be deal due to the go version.

@haoxins
Copy link
Collaborator Author

haoxins commented Jan 5, 2024

some ci failures need to be deal due to the go version.

@Nicole00 may I just removed the go versions <1.18?

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

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

Comparison is base (6eb07e9) 63.41% compared to head (fd13b92) 63.37%.

Files Patch % Lines
result_set.go 57.40% 18 Missing and 5 partials ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
- Coverage   63.41%   63.37%   -0.05%     
==========================================
  Files           8        9       +1     
  Lines        2392     2452      +60     
==========================================
+ Hits         1517     1554      +37     
- Misses        750      768      +18     
- Partials      125      130       +5     

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

@Nicole00
Copy link
Contributor

Nicole00 commented Jan 8, 2024

some ci failures need to be deal due to the go version.

@Nicole00 may I just removed the go versions <1.18?

For now we can't remove the go version yet. Because it will confluence the users' usage when upgrade the nebula-go.

@haoxins
Copy link
Collaborator Author

haoxins commented Jan 8, 2024

some ci failures need to be deal due to the go version.

@Nicole00 may I just removed the go versions <1.18?

For now we can't remove the go version yet. Because it will confluence the users' usage when upgrade the nebula-go.

ok, I will remove the usage of lo which requires Go 1.18+

@haoxins
Copy link
Collaborator Author

haoxins commented Jan 8, 2024

some ci failures need to be deal due to the go version.

@Nicole00 may I just removed the go versions <1.18?

For now we can't remove the go version yet. Because it will confluence the users' usage when upgrade the nebula-go.

@Nicole00 I have added the Go 1.16+ back

@haoxins
Copy link
Collaborator Author

haoxins commented Jan 10, 2024

@Nicole00 any other comments?

@Aiee Aiee merged commit 3fe0bb1 into vesoft-inc:master Jan 11, 2024
20 checks passed
@haoxins haoxins deleted the scan-rows branch January 11, 2024 12:47
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.

None yet

4 participants