[GitHub] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
I'm closing this PR in favor of another which uses a technique to merge 
attribute copies when possible.


---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
Agreed. I've been working on adding @navsan 's idea of merging contiguous 
attributes. I have another [branch with those 
changes](https://github.com/cramja/incubator-quickstep/tree/splitrow_partial_ins).
 It's almost ready, I'm in the process of benchmarking with TPCH to see if it 
can replace PackedRow.

Granted this PR speeds up Splitrow, and has tests so can be merged.


---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[pateljm]

Github user pateljm commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
@cramja This is a nice feature. Thanks @navsan for the detailed comments. 

Both, can we close this PR out over the next few days. Gotta keep moving 
™ :) 


---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
@navsan I made the following changes:
* Make per-column insert info
* Moved some variables outside the loop
* sped up the way that the block estimates the number of tuples it can 
insert

After these changes, the experiment (mentioned in the header to this PR) 
speeds up to `7267.457 ms` with the same setup.


---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[navsan]

Github user navsan commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
Sounds good! Thanks.

On Tue, Sep 20, 2016, 17:22 Marc S  wrote:

> @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 vectors
>
> 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.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> 
,
> or mute the thread
> 

> .
>



---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[cramja]

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 
vectors

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

[GitHub] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[navsan]

Github user navsan commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
Finally, 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. 


---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[navsan]

Github user navsan commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
Also, always explain in the PR what tests/experiments you ran. 

- Did you confirm correctness of the code? How? Just the current test 
suite? TPCH queries?
- How did you determine the 2x performance improvement? What were the test 
parameters? What machine did you run this on?


---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[navsan]

Github user navsan commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
Please address the comments: mostly to do with performance improvements. 
Note that the objective here is not just to make SplitRowStore faster than it 
is today, but to get it to the point where we can drop PackedRowStore. We're 
not close to that right now, because we're doing far too much work in the inner 
loop. 

Also, 
- Consider getting rid of occupancy_bitmap_
- Consider moving the null_bitmap into the header as a single bitmap for 
the entire tuple storage subblock. 
- Consider getting rid of TypedValue 


---
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] incubator-quickstep issue #100: Refactor bulk insert for SplitRowStore

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/100
  
@navsan please take a look


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