Change to comfy-table from prettytable-rs#656
Change to comfy-table from prettytable-rs#656alamb merged 3 commits intoapache:masterfrom PsiACE:comfy-table
Conversation
Signed-off-by: Chojan Shang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #656 +/- ##
=======================================
Coverage 82.43% 82.43%
=======================================
Files 168 168
Lines 47340 47340
=======================================
Hits 39027 39027
Misses 8313 8313 Continue to review full report at Codecov.
|
|
Looks good @PsiACE -- thank you -- I will look carefully tomorrow but this looks great |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
All in all I think this is a good change. Thank you @PsiACE
I would like at least one other approval from a maintainer before merging it. Perhaps @andygrove @paddyhoran or @nevi-me have thoughts
If we do go with this change, I don't think we should backport it to the 5.X line -- but instead release it for the first time in 6.0 - 2 months or so (see notes under datafusion section below)
Maintenance
From crates.io it does appear that https://crates.io/crates/prettytable-rs was last updated almost 3 years ago, though it does seem to be very heavily used (though arrow is the third largest user)
https://crates.io/crates/comfy-table/4.0.1 was last updated a month ago and looks more actively maintained, but is less widely used.
This seems like a good tradeoff to me
Dependency Analysis
Looking at the dependencies
cargo tree -p arrow --features=prettyprintHere are the comfy table dependencies:
├── comfy-table v4.0.1
│ ├── crossterm v0.20.0
│ │ ├── bitflags v1.2.1
│ │ ├── libc v0.2.98
│ │ ├── mio v0.7.13
│ │ │ ├── libc v0.2.98
│ │ │ └── log v0.4.14
│ │ │ └── cfg-if v1.0.0
│ │ ├── parking_lot v0.11.1
│ │ │ ├── instant v0.1.10
│ │ │ │ └── cfg-if v1.0.0
│ │ │ ├── lock_api v0.4.4
│ │ │ │ └── scopeguard v1.1.0
│ │ │ └── parking_lot_core v0.8.3
│ │ │ ├── cfg-if v1.0.0
│ │ │ ├── instant v0.1.10 (*)
│ │ │ ├── libc v0.2.98
│ │ │ └── smallvec v1.6.1
│ │ ├── signal-hook v0.3.9
│ │ │ ├── libc v0.2.98
│ │ │ └── signal-hook-registry v1.4.0
│ │ │ └── libc v0.2.98
│ │ └── signal-hook-mio v0.2.1
│ │ ├── libc v0.2.98
│ │ ├── mio v0.7.13 (*)
│ │ └── signal-hook v0.3.9 (*)
│ ├── strum v0.21.0
│ └── strum_macros v0.21.1 (proc-macro)
│ ├── heck v0.3.3
│ │ └── unicode-segmentation v1.8.0
│ ├── proc-macro2 v1.0.27
│ │ └── unicode-xid v0.2.2
│ ├── quote v1.0.9
│ │ └── proc-macro2 v1.0.27 (*)
│ └── syn v1.0.74
│ ├── proc-macro2 v1.0.27 (*)
│ ├── quote v1.0.9 (*)
│ └── unicode-xid v0.2.2
Here are the dependencies from pretty-table:
├── prettytable-rs v0.8.0
│ ├── atty v0.2.14
│ │ └── libc v0.2.98
│ ├── csv v1.1.6 (*)
│ ├── encode_unicode v0.3.6
│ ├── lazy_static v1.4.0
│ ├── term v0.5.2
│ │ ├── byteorder v1.4.3
│ │ └── dirs v1.0.5
│ │ └── libc v0.2.98
│ └── unicode-width v0.1.8
DataFusion
I also verified that https://github.com/apache/arrow-datafusion would work with this change by patching a local datafusion checkout to use this branch and then running cargo test -p datafusion
There are two differences in the tests reported, both edge cases (empty tables and floating point). I think they are changes for the better but also not strictly speaking backwards compatible. See datafusion-compile-run.txt for the full run
paddyhoran
left a comment
paddyhoran
left a comment
There was a problem hiding this comment.
It's hard to predict how a library will be maintained in the future but I agree with this change. I'm no expert on this aspect but is there anything we need to do due to it being MIT licensed?
arrow/Cargo.toml
Outdated
| flatbuffers = { version = "=2.0.0", optional = true } | ||
| hex = "0.4" | ||
| prettytable-rs = { version = "0.8.0", optional = true } | ||
| comfy-table = { version = "4.0", optional = true } |
There was a problem hiding this comment.
Please add default-features = false. This will allow the prettyprint feature to be enabled in wasm32-wasi target in the next comfy-table release.
Ref: Nukesor/comfy-table#47
FYI @jorgecarleitao
Signed-off-by: Chojan Shang <[email protected]>
|
I think this has been open long enough for comment. Merging in (will not backport, will plan to release in arrow 6.0.0) |
|
Thanks again @PsiACE |
* Change to comfy-table Signed-off-by: Chojan Shang <[email protected]> * Apply review Signed-off-by: Chojan Shang <[email protected]>
5 participants
Signed-off-by: Chojan Shang [email protected]
Which issue does this PR close?
Closes #69 .
Rationale for this change
comfy-table is still actively maintained.
also, solve RUSTSEC-2018-0015: term is looking for a new maintainer
What changes are included in this PR?
Change to comfy-table from prettytable-rs.
Are there any user-facing changes?
No.