[jira] [Commented] (DRILL-5846) Improve Parquet Reader Performance for Flat Data types

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/DRILL-5846?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16319050#comment-16319050 ] 

ASF GitHub Bot commented on DRILL-5846:
---------------------------------------

Github user sachouche commented on the issue:

    https://github.com/apache/drill/pull/1060
  
    Before I reply to the provided comments I want first to thank both Parth and Paul for taking time to review this Pull Request.
    @parthchandra Regarding the High Level Comments
    
    - FS Access Manager
    The attached document listed several possible improvements in terms of CPU, Partitioning (changes to the query plan), and IO layer. I didn't want to club all the changes in a single pull request; instead I started with the Pull Request with the highest priority (in terms of performance improvement) which is CPU related enhancements.
    
    - Drill Memory Package
    I understand where you're coming from; I have analyzed this task and came up with a solution which I felt was the cleanest. Let me take you through my thinking process and the alternative solutions that I have considered:
    a) Why did I use this low level APIs?
    The Bulk Parquet reader uses the Drill Buffer APIs to copy data chunks to the CPU cache; this means that memory sanity checks are always performed. During code profiling I have noticed the Hotspot was using too many instructions (byte based assembly was used); I have researched ways to improve this and found some Hadoop and Compression java code which utilized 64bit access instructions when manipulating byte arrays (e.g., [Snappy](https://github.com/airlift/aircompressor/blob/master/src/main/java/io/airlift/compress/snappy/SnappyRawCompressor.java)).  This makes a huge difference
    
    b) Safety
    As indicated previously, native data is always accessed through the Drill Buffer APIs (that is, accessed data is sanitized). During bulk processing, the new utility has additional checks which are triggered when assertions are enabled. This means all test-suite and QA tests (which run with assertions On) will be able to catch any faulty memory access; note also the checks are simpler since they only pertain to byte arrays boundary checks.
    
    c) Abstraction
    I agree that the Drill code base should follow a common code path when accessing native memory (minimize chances of introducing bugs), though I felt the Drill Memory package was the main abstraction since it was centralized and thus easier to test.
    
    d) Alternatives
    I have considered using the Netty io.netty.util.internal.PlatformDependent0 to access the Java intrinsics APIs though this class has a package scope and the wrapper public class io.netty.util.internal.PlatformDependent doesn't expose this low level functionality. Thus the work around would have been to create MemoryUtils class under the io.netty.util.internal package. I didn't think this was a cleaner approach than just using Java's intrinsic APIs directly (note also that Java9 will expose such intrinsics under a public JDK package instead of the sun.misc. Please let me know if you rather prefer the io.netty.util.internal route instead.
    
    - Changes to the Vector Code
    I had few discussions with Paul about the need to reconcile DRILL-5657; the agreement was to proceed with the current implementation and then apply refactoring (future releases) as my changes are backward compatible (new public APIs added). 
    
    - Arrow Integration
    I will create a task to analyze the impact of the Arrow Integration on the Parquet Reader.
    
    - UserBitShared.proto
    This class was updated to deal with the implicit columns optimization. I have added two fields to the NullableVarChar class to capture the logical number of rows and the fact this feature was active. This allowed me to use different Accessor and Mutator implementation. You and I had a discussion about this and the agreement was to:
    a) The new optimization is configurable (current default is off); though we might change the default at some point
    b) Detect C++ based clients and turn of this optimization (that is, the implicit columns enhancements); I didn't implement this task yet as I forgot the name of the property that you have indicated (this should be quite fast to implement though)
    
    - Testing
    Yes, QA helped me to run the test-suite with the two new options enabled. The new changes didn't modify any of the encoding/compression logic but I'll be happy to add any test(s) you deem necessary.. 
    
    - Code Style
    Thank you for the pointers; I'll stick to your feedback in future changes to ease the review process.
    
        



> Improve Parquet Reader Performance for Flat Data types 
> -------------------------------------------------------
>
>                 Key: DRILL-5846
>                 URL: https://issues.apache.org/jira/browse/DRILL-5846
>             Project: Apache Drill
>          Issue Type: Improvement
>          Components: Storage - Parquet
>    Affects Versions: 1.11.0
>            Reporter: salim achouche
>            Assignee: salim achouche
>              Labels: performance
>             Fix For: 1.13.0
>
>
> The Parquet Reader is a key use-case for Drill. This JIRA is an attempt to further improve the Parquet Reader performance as several users reported that Parquet parsing represents the lion share of the overall query execution. It tracks Flat Data types only as Nested DTs might involve functional and processing enhancements (e.g., a nested column can be seen as a Document; user might want to perform operations scoped at the document level that is no need to span all rows). Another JIRA will be created to handle the nested columns use-case.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)