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

feat: Change Iterator base on google guideline #131

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

NguyenHoangSon96
Copy link

@NguyenHoangSon96 NguyenHoangSon96 commented Jan 20, 2025

Closes #

Proposed Changes

_ Rewrite QueryIterator base on google guidelines
Link issue: Issue
Guidelines suggested Guidelines

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Jan 20, 2025
@NguyenHoangSon96 NguyenHoangSon96 linked an issue Jan 20, 2025 that may be closed by this pull request
@NguyenHoangSon96
Copy link
Author

@bednar @vlastahajek
Some code blocks reported by codecov has been ignored because I don't think it is necessary to test them, most of them just copied from the old code...
Please tell me if you think otherwise

Should we deprecate the QueryIterator? 🤔

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 53.62319% with 32 lines in your changes missing coverage. Please review.

Project coverage is 86.41%. Comparing base (53e02a2) to head (db475aa).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
influxdb3/point_value_iterator.go 57.69% 16 Missing and 6 partials ⚠️
influxdb3/query.go 41.17% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   88.11%   86.41%   -1.71%     
==========================================
  Files          15       16       +1     
  Lines        1346     1413      +67     
==========================================
+ Hits         1186     1221      +35     
- Misses        134      160      +26     
- Partials       26       32       +6     

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

@bednar bednar requested review from alespour and karel-rehor and removed request for bednar and alespour January 20, 2025 08:14
Copy link
Contributor

@alespour alespour left a comment

Choose a reason for hiding this comment

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

Nice! LGTM

return it.index
}

func asPoints(record arrow.Record, index int) (*PointValues, error) {
Copy link
Contributor

@vlastahajek vlastahajek Jan 23, 2025

Choose a reason for hiding this comment

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

This method is almost the same as in query_iterator.go. Please, refactor it so there is only one such logic.

it.index++

for it.record == nil || it.index >= int(it.record.NumRows()) {
if !it.reader.Next() {
Copy link
Contributor

@vlastahajek vlastahajek Jan 23, 2025

Choose a reason for hiding this comment

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

flight.Reader.Next() can produce an error and return false.
We should check for the error and act accordingly.

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.

QueryIterator should not panic on error
3 participants