-
Notifications
You must be signed in to change notification settings - Fork 912
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
Remove device_vector
s from parquet
#7853
Conversation
Any benchmark results you can show to verify no regression? |
rerun tests |
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.
Approved, assuming that performance did not regress.
// TODO (dm): Enable dictionary for list after refactor | ||
if (physical_type() != BOOLEAN && physical_type() != UNDEFINED_TYPE && !is_list()) { | ||
alloc_dictionary(_data_count); | ||
// TODO (dm): Enable dictionary for list and struct after refactor |
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.
Maybe file an issue (if you haven't already)?
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.
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.
LGTM! (Needs #7758 in first, of course.)
Codecov Report
@@ Coverage Diff @@
## branch-0.20 #7853 +/- ##
===============================================
- Coverage 82.88% 82.51% -0.38%
===============================================
Files 103 103
Lines 17668 17296 -372
===============================================
- Hits 14645 14272 -373
- Misses 3023 3024 +1
Continue to review full report at Codecov.
|
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.
Looks good overall. Just a question here, mostly a nit.
No noticeable effect on benchmarks. Benchmark results
|
@gpucibot merge |
- Replaced raw pointers in parquet writer with appropriate spans - Used `hostdevice_2dvector` as approprate - Various parameter cleanups Depends on #7758 and #7853 Authors: - Devavret Makkar (https://github.com/devavret) - Mark Harris (https://github.com/harrism) Approvers: - Mike Wilson (https://github.com/hyperbolic2346) - Vukasin Milovanovic (https://github.com/vuule) URL: #7950
Removes
device_vector
in favour of eitherdevice_uvector
ordevice_buffer
as appropriate in parquet reader and writer.Contributes to #7287
Depends on #7758