-
Notifications
You must be signed in to change notification settings - Fork 175
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
[FEAT] Ellipsize glob scan paths #2809
[FEAT] Ellipsize glob scan paths #2809
Conversation
CodSpeed Performance ReportMerging #2809 will degrade performances by 56.78%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2809 +/- ##
=======================================
Coverage ? 63.29%
=======================================
Files ? 1007
Lines ? 114181
Branches ? 0
=======================================
Hits ? 72276
Misses ? 41905
Partials ? 0
|
e477f18
to
30b3d22
Compare
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.
Welcome and thanks for the contribution! Just a minor fix and we should be good to go!
src/daft-scan/src/glob.rs
Outdated
@@ -272,9 +272,28 @@ impl ScanOperator for GlobScanOperator { | |||
} | |||
|
|||
fn multiline_display(&self) -> Vec<String> { | |||
let condensed_glob_paths = if self.glob_paths.len() <= 6 { |
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.
This should be 7
, otherwise we take the same amount of lines and omit 1 file.
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.
done
src/daft-scan/src/lib.rs
Outdated
|
||
for _ in 0..num_sources { | ||
sources.push(format!("../../tests/assets/parquet-data/mvp.parquet")); | ||
// sources.push(format!("File {}", i + 1)); |
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.
commented out line.
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.
removed the commented line.
On a side note, I am not sure of the failing benchmarking tests. The numbers also seem highly varying, though there was no change in functionality.
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.
@anmolsingh20 the benchmarks are a known issue right now, you can just ignore them for the time being.
209c79c
to
deb577f
Compare
Resolves #2709