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

build(deps): update datafusion to latest and arrow to 51.0 #3661

Merged
merged 42 commits into from
Apr 18, 2024

Conversation

MichaelScofield
Copy link
Collaborator

@MichaelScofield MichaelScofield commented Apr 8, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

pre-work to #3520

What's changed and what's your intention?

Old version of datafusion cannot rewrite TableScan plan with different table source: it uses plan equality to determine whether the rewrite is happened, while the table source field of TableScan plan is not considered in eq.

This PR also updates the "friends" of datafusion, including Arrow (to version 51), Parquet, sqlparser and tonic.

supercede #3554

Per Module Review Checklist

Please tick the checkbox if you are done reviewing a module

  • benchmarks/src/bin/nyc-taxi.rs reviewed by: @evenyag
  • src/catalog/src/table_source.rs
  • src/common/datasource/*
  • src/common/function/src/scalars/*
  • src/common/macro/src/range_fn.rs
  • src/datanode/src/region_server.rs
  • src/datatypes/src/* reviewed by: @evenyag
  • src/file-engine/* reviewed by: @evenyag
  • src/flow/*, reviewed by: @discord9
  • src/meta-srv/src/service/admin.rs
  • src/mito2/* reviewed by: @evenyag
  • src/operator/*
  • src/promql/*
  • src/query/*
  • src/script/*, reviewed by: @discord9
  • src/servers/*
  • src/session/src/context.rs
  • src/sql/*
  • src/table/*
  • tests-integration/*
  • tests/cases/*

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 77.02596% with 292 lines in your changes are missing coverage. Please review.

Project coverage is 85.46%. Comparing base (5107822) to head (94b75f4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3661      +/-   ##
==========================================
- Coverage   85.50%   85.46%   -0.05%     
==========================================
  Files         966      966              
  Lines      162218   162498     +280     
==========================================
+ Hits       138710   138871     +161     
- Misses      23508    23627     +119     

@killme2008
Copy link
Contributor

@MichaelScofield Some conflicts exist in cargo files.

@MichaelScofield
Copy link
Collaborator Author

@killme2008 resolved

@MichaelScofield MichaelScofield marked this pull request as ready for review April 17, 2024 13:07
@tisonkun tisonkun changed the title chore: update datafusion build(deps): update datafusion to latest and arrow to 51.0 Apr 17, 2024
Copy link
Collaborator

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Approve as a whole. Every module expert should give a look IMO 🤣

@tisonkun
Copy link
Collaborator

I suppose we merge this patch ASPA to avoid further conflict, as long as there is no disabled tests and all tricky hacks are marked.

src/mito2/src/memtable/partition_tree/tree.rs Outdated Show resolved Hide resolved
src/common/recordbatch/src/filter.rs Outdated Show resolved Hide resolved
src/mito2/src/memtable/time_series.rs Outdated Show resolved Hide resolved
src/mito2/src/sst/parquet/reader.rs Outdated Show resolved Hide resolved
src/datatypes/src/error.rs Outdated Show resolved Hide resolved
src/flow/clippy.toml Outdated Show resolved Hide resolved
@tisonkun
Copy link
Collaborator

@waynexia May you confirm the PromQL part? And I suppose @MichaelScofield can confirm the rest.

@MichaelScofield
Copy link
Collaborator Author

@waynexia May you confirm the PromQL part? And I suppose @MichaelScofield can confirm the rest.

@waynexia himself made the changes to PromQL part, no need to do further review here. I've confirmed the rest.

@tisonkun
Copy link
Collaborator

Then let's give another approval to move this forward. cc @waynexia @v0y4g3r @zhongzc

@MichaelScofield MichaelScofield added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit 314f270 Apr 18, 2024
20 checks passed
@MichaelScofield MichaelScofield deleted the chore/update-datafusion branch April 18, 2024 12:25
// TODO: found proper place for this
/// ref to `arrow_schema::datatype` for type name
fn typename_to_cdt(name: &str) -> CDT {
match name {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to match name. to_uppercase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @discord9

Will change it in my next PR, there might be a better way which is to reuse datafusion's logic(which is using sqlparser to parse typename)

Comment on lines +835 to +838
request.projection = match projection {
Some(x) if !x.is_empty() => Some(x.clone()),
_ => None,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we use request.projection = projection.cloned(); here? Though Some(vec![]) seems meaningless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants