Github user cramja commented on the issue:

    https://github.com/apache/incubator-quickstep/pull/100
  
    @navsan love the comments, very thorough.
    
    I updated the header of this PR to include benchmarks/testing info and will 
keep this in mind in the future.
    
    **Changes queued for this PR**
    * getting rid of TypedValue
    * moving some of the computations out of the inner loop and keeping a 
leaner set of running state variables
    * moving to a per-column info struct rather than a number of bitvectors and 
vector<int>s
    
    I'm going to address these in an addendum commit. I think your suggestions 
are great.
    
    **Different PR**
    * getting rid of occupancy_bitmap_
    * moving the null_bitmap into the header as a single bitmap for the entire 
tuple storage subblock.
    
    I think these are valid issues (comments above for specifics) but that they 
can be the subject of another commit/PR in the pursuit of more concise PRs.
    
    **Another PR**
    > Can you coalesce writes for contiguous columns in input/output? This 
helps speed up the common case of materialized join results where (almost) 
everything gets copied out anyway.
    
    > Can you also add support for partialBulkInsert functions? See the commits 
in my branch for reference. It'll help us improve the join result 
materialization cost, as well as a few other operations.
    
    Both these points can be addressed but in a separate PR. 


---
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.
---

Reply via email to