Skip to content

Conversation

@nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Nov 15, 2020

This takes us from 2020-04-22 to 2020-11-14.
There have been a lot of clippy lints introduced since April, and an incompleteness feature warning about specialization (which causes CI to fail).

I have allowed almost all clippy lints in datafusion, as we have >150 warnings there.
I have made changes to fix arrow and parquet lints where I perceive them not to affect open PRs.

@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 15, 2020

FYI @vertexclique

@github-actions
Copy link

@jorgecarleitao
Copy link
Member

Segfault on the coverage, outch :/

@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 15, 2020

Segfault on the coverage, outch :/

I want to look at whether we can use the new llvm-based coverage instead of tarpaulin

The failure was from a newer version of `cargo-tarpaulin`
@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 15, 2020

Segfault on the coverage, outch :/

Found and fixed the problem. We were using an older cached version of cargo-tarpaulin, but the latest version has a bug; so I've forced us into using an older version, and left a reference to the GH issue.

We should be good to go @jorgecarleitao

@codecov-io
Copy link

Codecov Report

Merging #8666 (c094456) into master (3e72c70) will decrease coverage by 0.29%.
The diff coverage is 89.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8666      +/-   ##
==========================================
- Coverage   84.84%   84.55%   -0.30%     
==========================================
  Files         165      177      +12     
  Lines       43322    43567     +245     
==========================================
+ Hits        36757    36838      +81     
- Misses       6565     6729     +164     
Impacted Files Coverage Δ
rust/arrow/src/array/array.rs 91.48% <ø> (-0.23%) ⬇️
rust/arrow/src/compute/kernels/comparison.rs 95.89% <ø> (+0.92%) ⬆️
rust/arrow/src/error.rs 5.26% <0.00%> (-0.30%) ⬇️
rust/datafusion/src/logical_plan/expr.rs 78.94% <ø> (ø)
rust/datafusion/src/logical_plan/extension.rs 0.00% <ø> (ø)
rust/datafusion/src/logical_plan/plan.rs 83.27% <ø> (ø)
rust/datafusion/src/optimizer/filter_push_down.rs 95.95% <ø> (-2.28%) ⬇️
...ust/datafusion/src/physical_plan/hash_aggregate.rs 89.40% <ø> (+3.64%) ⬆️
rust/datafusion/src/physical_plan/merge.rs 64.51% <ø> (-2.76%) ⬇️
rust/datafusion/src/test/mod.rs 89.65% <ø> (+3.82%) ⬆️
... and 166 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2f1e01...c094456. Read the comment docs.

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot @nevi-me , cool fixes to the code also.

Copy link
Contributor

@vertexclique vertexclique left a comment

Choose a reason for hiding this comment

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

Thanks for this Neville!

@jorgecarleitao
Copy link
Member

Do we wait for some of the PRs to be flushed, or do we merge this?

@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 15, 2020

I don't see this being a big risk to open PRs (as some already have merge conflicts). We run 36 CI jobs for this, vs the normal 8; so maybe there's an efficiency argument in merging this sooner, instead of having to rebase later (which I don't mind doing).

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

🎉 -- thank you @nevi-me

FYI I filed https://issues.apache.org/jira/browse/ARROW-10597 to track enablign clippy lints in datafusion

@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 15, 2020

@alamb thanks. A quick solution for many of the lints, is to run clippy-fix with -Z unstable-options, so that one doesn't manually fix them.

@alamb
Copy link
Contributor

alamb commented Nov 16, 2020

@nevi-me check it out: #8687 -- which I think enables clippy lint in all parts of the project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants