Skip to content

Conversation

@tianchen92
Copy link
Contributor

Related to ARROW-6225.

@tianchen92
Copy link
Contributor Author

cc @emkornfield

@codecov-io
Copy link

codecov-io commented Aug 19, 2019

Codecov Report

Merging #5099 into master will increase coverage by 2.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5099      +/-   ##
==========================================
+ Coverage   87.64%   89.76%   +2.12%     
==========================================
  Files        1014      685     -329     
  Lines      145922   102521   -43401     
  Branches     1437        0    -1437     
==========================================
- Hits       127887    92027   -35860     
+ Misses      17673    10494    -7179     
+ Partials      362        0     -362
Impacted Files Coverage Δ
cpp/src/arrow/testing/gtest_util.h 80.53% <0%> (-16.84%) ⬇️
cpp/src/parquet/arrow/reader.h 65% <0%> (-15%) ⬇️
cpp/src/arrow/testing/util.h 91.3% <0%> (-8.7%) ⬇️
cpp/src/parquet/thrift.h 86.13% <0%> (-7.9%) ⬇️
cpp/src/arrow/util/compression.cc 82.14% <0%> (-4.96%) ⬇️
cpp/src/parquet/arrow/reader.cc 81.43% <0%> (-3.48%) ⬇️
cpp/src/arrow/status.cc 48.88% <0%> (-3.34%) ⬇️
cpp/src/arrow/dataset/file_base.h 87.5% <0%> (-2.98%) ⬇️
python/pyarrow/pandas-shim.pxi 64.35% <0%> (-0.91%) ⬇️
python/pyarrow/tests/test_array.py 95.65% <0%> (-0.7%) ⬇️
... and 721 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 2ba0566...456a532. Read the comment docs.

@emkornfield
Copy link
Contributor

sorry haven't gotten all the way through the review, will try to so later.

@tianchen92
Copy link
Contributor Author

tianchen92 commented Aug 22, 2019

@emkornfield Hi, Micah, I updated this PR according to your suggestion, please see if we could get it merged? I think I could provide a iterator API after then. thanks!

@tianchen92
Copy link
Contributor Author

@emkornfield Micah, when you have time, remember to take a look at this one, thanks :)

@emkornfield
Copy link
Contributor

emkornfield commented Aug 23, 2019 via email

@tianchen92
Copy link
Contributor Author

@emkornfield Hi Micah, I did a refactor, please take another look
i. remove all writers in consumer, and keep a currentIndex just as you said setValueCount is not cheap
ii. make MapConsumer use SturctConsumer as its delegate
iii. remove putChild/setDataVector API usages
iv. resolve other comments.
Thanks!

@tianchen92
Copy link
Contributor Author

@emkornfield Micah, please help take a look when you have a time :)

@emkornfield
Copy link
Contributor

@tianchen92 looking better, but still have some comments.. Thank you for the quick turn arounds. In general, while try to review every night I don't necessarily always have time. Please, only ping me if you haven't heard anything on a review for 5 business days (also once you've updated and something is ready for review, you can use the github request re-review feature).

@tianchen92
Copy link
Contributor Author

@emkornfield Thanks for your careful review, revised according to your comments.

@tianchen92 tianchen92 closed this Aug 28, 2019
@tianchen92 tianchen92 reopened this Aug 28, 2019
@tianchen92
Copy link
Contributor Author

@emkornfield PR updated, please help take a look, thanks!

Copy link
Contributor

@emkornfield emkornfield left a comment

Choose a reason for hiding this comment

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

One small issue, otherwise I think we can merge this and move on. At some point it might be good to refactor some logic from the primitive consumers into a base class.

@tianchen92
Copy link
Contributor Author

One small issue, otherwise I think we can merge this and move on. At some point it might be good to refactor some logic from the primitive consumers into a base class.

Thanks, I fixed now. Also I have a question about Enum type in https://issues.apache.org/jira/browse/ARROW-6356 please take a look~

@tianchen92
Copy link
Contributor Author

tianchen92 commented Aug 29, 2019

@emkornfield build passed, I think we could get it merged and I will submit a follow-up PR today, thanks.

@tianchen92
Copy link
Contributor Author

cc @emkornfield

@emkornfield
Copy link
Contributor

@ursabot build

@emkornfield
Copy link
Contributor

+1, Please lets make sure to get test coverage directly on StructConsumer in a follow-up PR.

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
Related to [ARROW-6225](https://issues.apache.org/jira/browse/ARROW-6265).

Closes apache#5099 from tianchen92/ARROW-6265 and squashes the following commits:

456a532 <tianchen> fix
ac9540d <tianchen> fix test
788fef1 <tianchen> fix
3639520 <tianchen> refactor
a415e93 <tianchen> fix array/map consume logic
54c6662 <tianchen> avoid mem copy
bb5346e <tianchen> fix array and map consumer
f435f66 <tianchen> add comments and fix array/map consumers
790471f <tianchen> fix style
37789be <tianchen> support fixed type
73edd51 <tianchen> initial support array and map

Authored-by: tianchen <[email protected]>
Signed-off-by: Micah Kornfield <[email protected]>
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.

3 participants