[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user majetideepak commented on the issue: https://github.com/apache/orc/pull/41 @omalley https://issues.apache.org/jira/browse/ORC-127 https://github.com/apache/orc/pull/77 Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user omalley commented on the issue:
https://github.com/apache/orc/pull/41
Deepak,
We got a notice of a new problem from Coverity. Can you look at it?
** CID 173749: Uninitialized members (UNINIT_CTOR)
/c++/src/Reader.cc: 259 in
orc::RowReaderImpl::RowReaderImpl(std::shared_ptr, const
orc::RowReaderOptions &)()
*** CID 173749: Uninitialized members (UNINIT_CTOR)
/c++/src/Reader.cc: 259 in
orc::RowReaderImpl::RowReaderImpl(std::shared_ptr, const
orc::RowReaderOptions &)()
253 } else {
254 previousRow = firstRowOfStripe[firstStripe]-1;
255 }
256
257 ColumnSelector column_selector(contents.get());
258 column_selector.updateSelected(selectedColumns, options);
>>> CID 173749: Uninitialized members (UNINIT_CTOR)
>>> Non-static class member "rowsInCurrentStripe" is not initialized in
this constructor nor in any functions that it calls.
259 }
260
261 const RowReaderOptions& RowReaderImpl::getRowReaderOptions()
const {
262 return options;
263 }
264
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user majetideepak commented on the issue: https://github.com/apache/orc/pull/41 I updated the patch to use a `shared_ptr` for `FileContents`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user omalley commented on the issue: https://github.com/apache/orc/pull/41 I think we should have FileContents be a shared ptr so that if the user deallocates the Reader, the RowReader won't reach into deallocated memory. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user majetideepak commented on the issue: https://github.com/apache/orc/pull/41 Replaced with REDUNDANT_MOVE. valgrind does not show any memory leaks due to this patch. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user omalley commented on the issue: https://github.com/apache/orc/pull/41 This is looking good. To compile on MacOS, I needed to replace some of the std::move with REDUNDANT_MOVE. See omalley:pr/41 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user majetideepak commented on the issue: https://github.com/apache/orc/pull/41 Included review comments. This patch is huge because I split the monolithic Reader.cc into various files - Reader.hh/Reader.cc: ReaderImpl and RowReaderImpl - Statistics.hh/Statistics.cc: ColumnStatisticsImpl, StatisticsImpl, Type*StatisticsImpl - StripeStream.hh/StripeStream.cc: StripeStreamsImpl, StreamInformationImpl, StripeInformationImpl - Options.hh: ReaderOptions, RowReaderOptions @omalley any feedback ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user omalley commented on the issue: https://github.com/apache/orc/pull/41 The ReaderOptions needs to split in half too: * ReaderOptions * setTailLocation * setErrorStream * setSerializedFooter * RowReaderOptions * include * range * throwOnHive11DecimalOverflow * forcedScaleOnHive11Decimal --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user swalkaus commented on the issue: https://github.com/apache/orc/pull/41 I might rename "Reader" something like "FileTailReader" but, in general, the refactoring makes sense. The schema can be read and columns metadata known before column selection is decided. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] orc issue #41: ORC-58: Move code for reading rows from Reader to RowReader
Github user majetideepak commented on the issue: https://github.com/apache/orc/pull/41 @omalley, please let me know your feedback. Thanks! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
