Skip to content

upgrade to v19 - #264

Closed
jychen7 wants to merge 8 commits into
roapi:mainfrom
jychen7:263-upgrade-to-datafusion-19
Closed

upgrade to v19#264
jychen7 wants to merge 8 commits into
roapi:mainfrom
jychen7:263-upgrade-to-datafusion-19

Conversation

@jychen7

@jychen7 jychen7 commented Mar 3, 2023

Copy link
Copy Markdown
Collaborator

close #263

2023-03-02

issues to fix:

  • columnq/src/table/delta.rs
    • the trait From<Infallible> is not implemented for ColumnQError
    • the trait From<&SchemaTypeStruct> is not implemented for datafusion::arrow::datatypes::Schema
    • the trait datafusion::datasource::TableProvider is not implemented for deltalake::DeltaTable
  • df.collect() now returns "error[E0507]: cannot move out of an Arc"
    • move occurs because value has type datafusion::dataframe::DataFrame, which does not implement the Copy trait
  • columnq/src/table/mod.rs
    • the trait Deserialize<'_> is not implemented for datafusion::arrow::datatypes::DataType

2023-03-25

  • roapi/src/context.rs
    • expected struct Arc, found struct DataFrame
    • implementation of graphql_parser::common::Text is not general enough
  • roapi/src/server/postgres.rs
    • expected struct arrow_array::record_batch::RecordBatch, found struct columnq::arrow::record_batch::RecordBatch
    • the trait From<DFSchema> is not implemented for arrow_schema::schema::Schema
    • move occurs because value has type DataFrame, which does not implement the Copy trait

@houqp

houqp commented Mar 20, 2023

Copy link
Copy Markdown
Member

@jychen7 let me know if you need any help getting this over the finish line!

@jychen7

jychen7 commented Mar 20, 2023

Copy link
Copy Markdown
Collaborator Author

@houqp yes, I was stuck at the above three errors and I am sorry that forgot to follow up. Could you help?

Comment thread columnq/src/query/graphql.rs
Comment thread Cargo.toml
Comment thread roapi/src/context.rs

@jychen7 jychen7 Mar 25, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is the last error to fix. I don't quite understand the error and need help on this, @houqp

error: implementation of `graphql_parser::common::Text` is not general enough
   --> roapi/src/context.rs:124:68
    |
124 |       ) -> Result<Vec<arrow::record_batch::RecordBatch>, QueryError> {
    |  ____________________________________________________________________^
125 | |         self.cq.query_graphql(query).await
126 | |     }
    | |_____^ implementation of `graphql_parser::common::Text` is not general enough
    |
    = note: `graphql_parser::common::Text<'1>` would have to be implemented for the type `&'0 str`, for any two lifetimes `'0` and `'1`...
    = note: ...but `graphql_parser::common::Text<'2>` is actually implemented for the type `&'2 str`, for some specific lifetime `'2`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not obvious to me why this is happening yet, will have to take a closer look 🤔

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I also take a look and have no idea...

@chitralverma chitralverma Apr 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the last error to fix. I don't quite understand the error and need help on this, @houqp

error: implementation of `graphql_parser::common::Text` is not general enough
   --> roapi/src/context.rs:124:68
    |
124 |       ) -> Result<Vec<arrow::record_batch::RecordBatch>, QueryError> {
    |  ____________________________________________________________________^
125 | |         self.cq.query_graphql(query).await
126 | |     }
    | |_____^ implementation of `graphql_parser::common::Text` is not general enough
    |
    = note: `graphql_parser::common::Text<'1>` would have to be implemented for the type `&'0 str`, for any two lifetimes `'0` and `'1`...
    = note: ...but `graphql_parser::common::Text<'2>` is actually implemented for the type `&'2 str`, for some specific lifetime `'2`

I solved this by removing explicit lifetimes and changing the &str to String. Not sure if this is the recommended way, the rust experts can guide me here.

@jychen7 @houqp @rtyler I have a local branch where I have successfully upgraded roapi to the latest versions of datafusion, arrow and delta-rs, along with some other minor upgrades were required.

shall I raise a PR for it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry @jychen7 that I dropped the ball on this one ;(

@chitralverma please feel free to send a new MR

@jychen7 jychen7 May 1, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I am sorry that I don't have time to look into this recently, but I could try best to help review

feel free to raise a PR. I will close this one after that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done !
#275

@rtyler

rtyler commented Apr 18, 2023

Copy link
Copy Markdown

Unfortunately Datafusion is already up to v22 🤕

There were a lot of API changes between v19 and v22 that we had to address in delta-rs, kind of a pain 😦

@houqp

houqp commented May 30, 2023

Copy link
Copy Markdown
Member

datafusion upgraded with #279

@houqp houqp closed this May 30, 2023
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.

Upgrade to Datafusion v19

4 participants