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

chore(deps): Update arrow and datafusion to 49.0.0 #24605

Merged
merged 8 commits into from Feb 1, 2024

Conversation

mgattozzi
Copy link
Contributor

@mgattozzi mgattozzi commented Jan 25, 2024

This commit copies in our dependency code from influxdb_iox in order for us to be able to upgrade from a forked version of 46.0.0 to 49.0.0 of both arrow and datafusion. Most of the important changes were around how we consumed the crates in influxdb3(_server/_write). Those diffs are particularly worth looking at as the rest was a straight copy and we don't touch those crates in our development currently for influxdb3 edge.

@pauldix In order to make sure credit is given where it's do I'm going through git logs to make sure everyone who committed in the iox_repo gets credit here with a coauthored by trailer for the commit as this was mostly not my code beyond the influxdb3 portions. I just wanted to open up the PR so you could look at it sooner rather than later.

Here's the command I used to grab all of the authors in the IOx repo for all of the folders changed in influxdb since the last common commit between these two repos till now when I'm copying in all of the code.

git log 427daa8..7565dc0 --pretty=format:"%an <%ae>" -- $(ls ~/influxdata/influxdb/) | sort | uniq

I had to hand remove a few Author doubles with different emails used and opted for the InfluxData email if there was one. With this we can give proper credit for most of these changes sans the influxdb3 crate ones which I did.

Co-authored-by: Adam Curtis adam@influxdata.com
Co-authored-by: Andrew Lamb alamb@influxdata.com
Co-authored-by: Carol (Nichols || Goulding) carol.nichols@gmail.com
Co-authored-by: Chunchun Ye 14298407+appletreeisyellow@users.noreply.github.com
Co-authored-by: Curtis Lee Fulton curtis@merlin.video
Co-authored-by: Dom Dwyer dom@itsallbroken.com
Co-authored-by: Fraser Savage fsavage@influxdata.com
Co-authored-by: Geoffrey Wossum gwossum@influxdata.com
Co-authored-by: Jack 56563911+jdockerty@users.noreply.github.com
Co-authored-by: Jeffrey Smith II jeffreyssmith2nd@gmail.com
Co-authored-by: Joe-Blount jblount@influxdata.com
Co-authored-by: Marco Neumann marco@crepererum.net
Co-authored-by: Marko Mikulicic mkm@influxdata.com
Co-authored-by: Martin Hilton mhilton@influxdata.com
Co-authored-by: Nga Tran nga-tran@live.com
Co-authored-by: opeyemi ofolorunsho@influxdata.com
Co-authored-by: Paul Dix paul@pauldix.net
Co-authored-by: Raphael Taylor-Davies 1781103+tustvold@users.noreply.github.com
Co-authored-by: wiedld dlw405@gmail.com
Co-authored-by: yeame solomonope@users.noreply.github.com

This commit copies in our dependency code from influxdb_iox in order for
us to be able to upgrade from a forked version of 46.0.0 to 49.0.0 of
both arrow and datafusion. Most of the important changes were around how
we consumed the crates in influxdb3(_server/_write). Those diffs are
particularly worth looking at as the rest was a straight copy and we
don't touch those crates in our development currently for influxdb3
edge.
@mgattozzi
Copy link
Contributor Author

@pauldix I removed the protobuf lint as this requires us to be in the IOx repo to work. I commented it out with a todo if we wanted to come back to it, but since we won't make changes to these crates or protobuf files here, I think we can safely comment it out for now. This lint still exists in the IOx repo and so I think that's okay, but I wanted to get your input on it.

Copy link
Member

@pauldix pauldix left a comment

Choose a reason for hiding this comment

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

Just one question, otherwise everything else looks good.

@@ -141,7 +141,7 @@ pub struct TableDefinition {
pub name: String,
#[serde(skip_serializing, skip_deserializing)]
pub schema: Option<Schema>,
columns: BTreeMap<String, ColumnType>,
columns: BTreeMap<String, i16>,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? Is ColumnType no longer public?

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to merge in for now so I can build on this, but let me know the motivation on this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ColumnType does not implement Serialize anymore and can only be constructed via try_from(i16) and it can be turned into an i16. So we can serialize an i16 but not a ColumnType and thus this change.

@pauldix pauldix self-requested a review February 1, 2024 00:17
@pauldix pauldix merged commit ff567cd into main Feb 1, 2024
11 checks passed
@mgattozzi
Copy link
Contributor Author

The Co-authored-by trailers weren't added in the merge. We can add them to the commit and do a force push, but I don't have those kinds of perms.

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

2 participants