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: respect iox::column_type::field metadata when mapping query results into values #114

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

bednar
Copy link
Member

@bednar bednar commented Nov 5, 2024

Closes https://github.com/influxdata/EAR/issues/5646

Proposed Changes

1/ This PR changed how we parse response from InfluxDB, now we respect the iox::column_type::field metadata.
2/ Enhance documentation for Query API.

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)

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 24 lines in your changes missing coverage. Please review.

Project coverage is 88.03%. Comparing base (c805bae) to head (bc28c61).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
influxdb3/query_iterator.go 73.33% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
+ Coverage   87.60%   88.03%   +0.42%     
==========================================
  Files          15       15              
  Lines        1299     1337      +38     
==========================================
+ Hits         1138     1177      +39     
  Misses        134      134              
+ Partials       27       26       -1     

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

@bednar bednar marked this pull request as ready for review November 5, 2024 13:23
Copy link
Contributor

@jstirnaman jstirnaman left a comment

Choose a reason for hiding this comment

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

Thanks for alerting me!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
influxdb3/query_iterator.go Outdated Show resolved Hide resolved
influxdb3/query_iterator.go Outdated Show resolved Hide resolved
@bednar
Copy link
Member Author

bednar commented Nov 6, 2024

@jstirnaman thanks for you review! All is done.

Copy link
Contributor

@vlastahajek vlastahajek left a comment

Choose a reason for hiding this comment

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

Generally, it looks good.
However, I would recommend changing interface{} to any

@bednar
Copy link
Member Author

bednar commented Nov 6, 2024

Thx @vlastahajek, the interface{} is changed to any.

Copy link
Contributor

@karel-rehor karel-rehor 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.

@bednar bednar force-pushed the respect-column-type-from-response branch 6 times, most recently from 687fb20 to dd4a81b Compare November 12, 2024 09:17
@bednar bednar force-pushed the respect-column-type-from-response branch from dd4a81b to 795cb87 Compare November 12, 2024 09:18
@bednar
Copy link
Member Author

bednar commented Nov 12, 2024

@karel-rehor, @vlastahajek, could you please review this PR again? I’ve updated the major version of the client and the base "package".

Copy link
Contributor

@karel-rehor karel-rehor left a comment

Choose a reason for hiding this comment

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

Tests pass locally. All examples work for me. Changes make sense. Looks good.

Copy link
Contributor

@vlastahajek vlastahajek left a comment

Choose a reason for hiding this comment

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

What do you think about providing a utility function for backward compatibility?

@bednar
Copy link
Member Author

bednar commented Nov 13, 2024

@vlastahajek, could you clarify what the function for backward compatibility will do? Will it be a helper to translate time.Time to arrow.Timestamp?

// - iox::column_type::field::float: => float64
// - iox::column_type::field::string: => string
// - iox::column_type::field::boolean: => bool
//
// Returns:
// - A map[string]interface{} object representing the current value.
func (i *QueryIterator) Value() map[string]interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change interface{} -> any here and elsewhere?

@vlastahajek
Copy link
Contributor

@vlastahajek, could you clarify what the function for backward compatibility will do? Will it be a helper to translate time.Time to arrow.Timestamp?

If it makes sense at all, either a function(s) that would convert the new output to the previous form for the output methods Value and AdPoints().

@bednar
Copy link
Member Author

bednar commented Nov 13, 2024

@vlastahajek, could you clarify what the function for backward compatibility will do? Will it be a helper to translate time.Time to arrow.Timestamp?

If it makes sense at all, either a function(s) that would convert the new output to the previous form for the output methods Value and AdPoints().

I don’t think that’s necessary. The changes involve the types of return objects. Previously, they were from the arrow package, but now they are GoLang types, which are more convenient for users and easier to use.

@bednar bednar merged commit ed5570e into main Nov 15, 2024
11 of 12 checks passed
@bednar bednar deleted the respect-column-type-from-response branch November 15, 2024 08:26
@bednar bednar added this to the 1.0.0 milestone Nov 15, 2024
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.

4 participants