-
Notifications
You must be signed in to change notification settings - Fork 314
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
Conversation
2a68b2f
to
542e581
Compare
Codecov ReportAttention: Patch coverage is
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 |
@MichaelScofield Some conflicts exist in cargo files. |
c544621
to
dbf9831
Compare
@killme2008 resolved |
7b51de1
to
cec6ca1
Compare
301ad01
to
7173bee
Compare
7173bee
to
193d263
Compare
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
There was a problem hiding this 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 🤣
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. |
Co-authored-by: Yingwen <[email protected]>
@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. |
// TODO: found proper place for this | ||
/// ref to `arrow_schema::datatype` for type name | ||
fn typename_to_cdt(name: &str) -> CDT { | ||
match name { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @discord9
There was a problem hiding this comment.
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)
request.projection = match projection { | ||
Some(x) if !x.is_empty() => Some(x.clone()), | ||
_ => None, | ||
}; |
There was a problem hiding this comment.
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.
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: @evenyagsrc/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: @evenyagsrc/file-engine/*
reviewed by: @evenyagsrc/flow/*
, reviewed by: @discord9src/meta-srv/src/service/admin.rs
src/mito2/*
reviewed by: @evenyagsrc/operator/*
src/promql/*
src/query/*
src/script/*
, reviewed by: @discord9src/servers/*
src/session/src/context.rs
src/sql/*
src/table/*
tests-integration/*
tests/cases/*
Checklist