Skip to content

ARROW-5880: [C++][Parquet] Use TypedBufferBuilder instead of ArrayBuilder in writer.cc #4827

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.

Sign up for GitHub

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

Jump to bottom
Closed
emkornfield wants to merge 11 commits into from
Closed

ARROW-5880: [C++][Parquet] Use TypedBufferBuilder instead of ArrayBuilder in writer.cc #4827

emkornfield wants to merge 11 commits into from

Conversation

@emkornfield
Copy link
Contributor

@emkornfield emkornfield commented Jul 9, 2019

  • Other small cleanups/comments based on my grokking of the code.
  • Vectorize inner loop of List based LevelBuilder where possible

bkietz
bkietz requested changes Jul 9, 2019
View reviewed changes
cpp/src/parquet/arrow/writer.cc Outdated
Copy link
Member

@bkietz bkietz Jul 9, 2019

Choose a reason for hiding this comment

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

Suggested change
// arrays otherwise we would preallocate. An upper boun on memory
// arrays otherwise we would preallocate. An upper bound on memory

bkietz
bkietz reviewed Jul 9, 2019
View reviewed changes
@fsaintjacques fsaintjacques changed the title ARROW-5880: Use TypedBufferBuilder instead of ArrayBuilder in writer.cc ARROW-5880: [C++] Use TypedBufferBuilder instead of ArrayBuilder in writer.cc Jul 9, 2019
@emkornfield emkornfield changed the title ARROW-5880: [C++] Use TypedBufferBuilder instead of ArrayBuilder in writer.cc ARROW-5880: [C++][Parquet] Use TypedBufferBuilder instead of ArrayBuilder in writer.cc Jul 10, 2019
@emkornfield
Copy link
Contributor Author

emkornfield commented Jul 10, 2019

@bkietz thanks for the review, all good suggestions.

@wesm
Copy link
Member

wesm commented Jul 10, 2019

Added some missing exports.

Beware as you make more changes on the Parquet codebase that my patch #4841 is slightly disruptive to the src/parquet/arrow directory (but should not be too bad to rebase)

@emkornfield
Copy link
Contributor Author

emkornfield commented Jul 12, 2019

@wesm thanks for the fixes. I will look more closely as I modify code to watch out for potential merge conflicts (I have enough other things on my plate that it might be a few days anyways before I get back to this).

I restarted the failing travis-CI job which looked spurious

@codecov-io
Copy link

codecov-io commented Jul 18, 2019

Codecov Report

Merging #4827 into master will increase coverage by 1.74%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4827      +/-   ##
==========================================
+ Coverage   87.31%   89.05%   +1.74%     
==========================================
  Files         894      718     -176     
  Lines      134066   100316   -33750     
==========================================
- Hits       117055    89334   -27721     
+ Misses      16659    10982    -5677     
+ Partials      352        0     -352
Impacted Files Coverage Δ
cpp/src/arrow/type.h 92.69% <ø> (ø) ⬆️
cpp/src/arrow/buffer-builder.h 100% <100%> (ø) ⬆️
cpp/src/parquet/arrow/writer.cc 96.71% <95%> (-0.04%) ⬇️
cpp/src/plasma/thirdparty/ae/ae.c 70.75% <0%> (-0.95%) ⬇️
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
rust/datafusion/src/bin/main.rs
... and 171 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 1abf18f...2f77313. Read the comment docs.

wesm
wesm approved these changes Jul 18, 2019
View reviewed changes
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1

@wesm wesm closed this in 6109647 Jul 18, 2019
kszucs pushed a commit that referenced this pull request Jul 22, 2019
@emkornfield @wesm @kszucs
ARROW-5880: [C++][Parquet] Use TypedBufferBuilder instead of ArrayBui…
a08e46e 
…lder in writer.cc

- Other small cleanups/comments based on my grokking of the code.
- Vectorize inner loop of List based LevelBuilder where possible

Closes #4827 from emkornfield/parquet_cleanup and squashes the following commits:

2f77313 <Wes McKinney> Export operator<< symbols for DataType, TimeUnit on Windows
e1698e5 <Micah Kornfield> fix compilation issues
152a6ee <Micah Kornfield> change back to signed it seems to be what the API uses
67ccf3e <Micah Kornfield> update references
ef7fade <Micah Kornfield> remove unneeded nesting
6776e61 <emkornfield> Update cpp/src/parquet/arrow/writer.cc
7cbc8e4 <emkornfield> Update cpp/src/parquet/arrow/writer.cc
83ee5ae <emkornfield> Update cpp/src/parquet/arrow/writer.cc
325af52 <Micah Kornfield> undo reserve
4516110 <Micah Kornfield> Small style issues
7795352 <Micah Kornfield> change from array builder to buffer builde

Lead-authored-by: Micah Kornfield <emkornfield@gmail.com>
Co-authored-by: emkornfield <emkornfield@gmail.com>
Co-authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimport asfimport mentioned this pull request Aug 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkietz bkietz bkietz requested changes

@wesm wesm wesm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@emkornfield @wesm @codecov-io @bkietz