[GitHub] [orc] pgaref edited a comment on pull request #635: ORC-742: LazyIO for non-filter columns

[GitBox <gi...@apache.org>]

pgaref edited a comment on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-781509748


   > > Left some initial comments here and there but in general I will have to agree with the existing comments that this is just to hard to review this PR and needs splitting. I would propose the following split:
   > 
   > Thanks for your feedback and suggestions @pgaref
   > 
   > > 
   > 
   > I would like to propose updates to checkstyle based on line and indentation feedback I have received on this PR.
   > 
   > > ```
   > > * Findbugs onliner
   > > ```
   > 
   > Can you please clarify what you mean by this?
   > 
   > > ```
   > > * Changes to MapReduce classes
   > > 
   > > * Introduce OrcFilterContext (also affects row-filter tests etc. but is manageable)
   > > 
   > > * Introduce ReadLevel on Reader and Planner
   > > 
   > > * Introduce [ORC-743](https://issues.apache.org/jira/browse/ORC-743) changes to the planner (partial-reads?)
   > > ```
   > 
   > [ORC-743](https://issues.apache.org/jira/browse/ORC-743) includes SArg to Filter conversion, the partial reads are part of this PR. The configuration for partial reads is absent in both the PRs right now.
   > 
   > I will treat this as introduce partial reads, followed by [ORC-743](https://issues.apache.org/jira/browse/ORC-743) which might have other splits that it needs.
   > 
   > > ```
   > > * Documentation changes after we merge all the expected functionality
   > > 
   > > * Code changes like making a variable final or using the UTF8 Character Set, or new methods (less critical)
   > > ```
   > 
   > Any changes that are not required we can pull it out to an unrelated PR. I would like to include methods that avoid code duplication into the PR still.
   > 
   > > Please let me know what you think and again thanks for all the work!
   > 
   > Hope that makes sense. Please let me know.
   
   > > * Findbugs onliner
   
   What I meant: https://github.com/apache/orc/pull/635/files/4f400a3893aa7d7a32ffad7970041a6e8acf7c28#diff-e1aaa921b730eea0b914cb64c8adfe6e743a0395c114d60f26ee19e036da20e9
   
   >I will treat this as introduce partial reads,
   
   Sounds good to me -- lets just make it configurable
   
   > I would like to include methods that avoid code duplication into the PR still.
   
   Sure, it makes sense
   
   @pavibhai Sounds like a plan -- lets start splitting this and I will be happy to help along the way!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org