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 arrow-array support for Field #111

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Add arrow-array support for Field #111

merged 1 commit into from
Jan 18, 2024

Conversation

HoKim98
Copy link
Contributor

@HoKim98 HoKim98 commented Oct 23, 2023

This PR allows the &dyn Array of arrow-array to be converted to Grafana's Field structure as easily as the Array trait of arrow2.

// AS-IS (unchanged)
let array = ::arrow2::array::PrimitiveArray::from_vec(vec![1, 2, 3]);  // Int32Array -> ::arrow2::Array trait
let field = ::grafana_plugin_sdk::data::ArrayIntoField::try_into_field(array, "field")?;

// TO-BE (added)
let array = Arc::new(::arrow_array::array::PrimitiveArray::from_vec(vec![1, 2, 3]));  // Int32Array -> Arc<dyn ::arrow_array::Array>
let field = ::grafana_plugin_sdk::data::ArrayRefIntoField::try_into_field(&array, "field")?;

Thanks to the interoperability of arrow-array and arrow2, the implementation of arrow2 closely matches that of arrow-array and is easily convertible.

However, unnecessary duplication of code occurred, and I think this can be solved by defining a macro. If you think this is necessary, I will pursue it.

@CLAassistant
Copy link

CLAassistant commented Oct 23, 2023

CLA assistant check
All committers have signed the CLA.

@HoKim98
Copy link
Contributor Author

HoKim98 commented Oct 26, 2023

Any progress on this? 😄

@sd2k
Copy link
Collaborator

sd2k commented Jan 4, 2024

Woah, sorry, for some reason I didn't get notified for this! I've turned on notifications now 🙈

This looks good to me, I'll try and get it merged soon.

@sd2k sd2k self-requested a review January 4, 2024 12:10
@sd2k sd2k self-assigned this Jan 4, 2024
@sd2k
Copy link
Collaborator

sd2k commented Jan 4, 2024

I think this will need rebasing after #112, sorry about that.

@HoKim98
Copy link
Contributor Author

HoKim98 commented Jan 18, 2024

We’re up to our ears in work :) Rebased to latest commit!

@sd2k sd2k added the enhancement New feature or request label Jan 18, 2024
@sd2k sd2k merged commit 4456e2a into grafana:main Jan 18, 2024
5 of 6 checks passed
@sd2k
Copy link
Collaborator

sd2k commented Jan 18, 2024

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants