You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by cramja <gi...@git.apache.org> on 2016/11/07 22:09:17 UTC
[GitHub] incubator-quickstep pull request #129: Partial inserts
GitHub user cramja opened a pull request:
https://github.com/apache/incubator-quickstep/pull/129
Partial inserts
This change enables use of the PartialBulkInserts feature, an efficient implementation of which was introduced in the SplitRowStore in #100 . Most of these changes were imported from @navsan 's branch which exploited partial inserts but only implemented the storage optimization required to take advantage of them for the PackedRowStore.
*Performance*
We ran SSB100 on the cloudlab machines and found that SplitRow's performance was almost exactly the same as PackedRow. `packed+split opt` includes both packed and splitrow with the copy optimization and uses both splitrow and packed row intermediate blocks. `splitrow opt ` uses only optimized splitrowstore blocks for intermediates.
```
time (s)
12.579 12.175 12.114
no opt splitrow opt packed+split opt
```
*TODO*
In a later PR (after paper submission), we will remove packed rowstore as it represents unnecessary complexity.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cramja/incubator-quickstep partial-inserts
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/129.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #129
----
commit 57144fd898ebec3e28d8b889b89254bafdfdf895
Author: navsan <na...@gmail.com>
Date: 2016-11-07T20:35:01Z
Adds support for PartialBulkInserts in StorageBlocks
- Enables use of PartialBulkInserts in StorageBlocks
- Value accessor changes to allow use of 2 insert destinations
- HashJoin now uses PartialBulkInserts
commit 6c63c03cdc9cac42ff35ca804c1622dace049a81
Author: cramja <ma...@gmail.com>
Date: 2016-11-07T22:00:31Z
Enables PartialInserts for SplitRow + Only use SplitRow Intermediates
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/129#discussion_r88499673
--- Diff: storage/InsertDestination.hpp ---
@@ -517,6 +527,12 @@ class PartitionAwareInsertDestination : public InsertDestination {
ValueAccessor *accessor,
bool always_mark_full = false) override;
+ void bulkInsertTuplesFromValueAccessors(
+ const std::vector<std::pair<ValueAccessor *, std::vector<attribute_id>>> &accessor_attribute_map,
+ bool always_mark_full = false) override {
+ FATAL_ERROR("bulkInsertTuplesFromValueAccessors is not implemented for PartitionAwareInsertDestination");
--- End diff --
Same as above `LOG(FATAL)`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/129#discussion_r88500702
--- Diff: storage/StorageBlock.hpp ---
@@ -307,13 +307,60 @@ class StorageBlock : public StorageBlockBase {
* iteration will be advanced to the first non-inserted tuple or, if
* all accessible tuples were inserted in this block, to the end
* position.
+ * @param max_tuples_to_insert Insert at most these many tuples
* @return The number of tuples inserted from accessor.
**/
tuple_id bulkInsertTuplesWithRemappedAttributes(
const std::vector<attribute_id> &attribute_map,
ValueAccessor *accessor);
/**
+ * @brief Insert up to max_num_tuples_to_insert tuples from a ValueAccessor
+ * as a single batch, using the attribute_map to project and reorder
+ * columns from the input ValueAccessor. Does not update header.
+ *
+ * @note Typical usage is where you want to bulk-insert columns from two
+ * or more value accessors. Instead of writing out the columns into
+ * one or more column vector value accessors, you can simply use this
+ * function with the appropriate attribute_map for each value
+ * accessor (InsertDestination::bulkInsertTuplesFromValueAccessors
+ * handles all the details) to insert tuples without an extra temp copy.
+ *
+ * @warning Must call bulkInsertPartialTuplesFinalize() to update the header,
+ * until which point, the insertion is not visible to others.
+ * @warning The inserted tuples may be placed in an "incorrect" or
--- End diff --
Is this warning still true? I thought it is safe to call any methods after `bulkInsertPartialTuplesFinalize` is called. For instance, in the SplitRowStore, it is safe to scan for tuples as long as `finalize` was called
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
Does it normally prompt for a uname/pw? I entered my apache name:
```
marc@marc-laptop:incubator-quickstep (master)*$ git push apache master
Username for 'https://git-wip-us.apache.org': my-apache-name
Password for 'https://my-apache-name@git-wip-us.apache.org':
fatal: Authentication failed for 'https://git-wip-us.apache.org/repos/asf/incubator-quickstep.git/'
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
@cramja If you have not set up a ssh way, it is required to enter your apache id username and password. But it seems that you have an authentication issues.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by hakanmemisoglu <gi...@git.apache.org>.
Github user hakanmemisoglu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
@cramja
I had the same problem, you are probably trying to push to the read-only Github repo.
[Here is the link for the workflow.](https://cwiki.apache.org/confluence/display/QUICKSTEP/Workflow+For+Committers)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
Looks good to me.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
It's happening in #140 as well
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
@zuyu The first commit was largely @navsan 's work, the second was mine. I think it makes sense to keep separate for provenance.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
Hi @cramja,
Since @hbdeshmukh has signed off for this PR, and the CI has only the system failures, not related to your changes, it should be good to merge.
Before doing so, I have a basic question that why this PR has two commits from two contributors: @cramja and @navsan? Could @cramja please squash them into one? Cheers!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/129#discussion_r88559668
--- Diff: storage/StorageBlockLayout.cpp ---
@@ -154,11 +154,7 @@ StorageBlockLayout* StorageBlockLayout::GenerateDefaultLayout(const CatalogRelat
description->set_num_slots(1);
TupleStorageSubBlockDescription *tuple_store_description = description->mutable_tuple_store_description();
- if (relation_variable_length) {
- tuple_store_description->set_sub_block_type(TupleStorageSubBlockDescription::SPLIT_ROW_STORE);
- } else {
- tuple_store_description->set_sub_block_type(TupleStorageSubBlockDescription::PACKED_ROW_STORE);
- }
+ tuple_store_description->set_sub_block_type(TupleStorageSubBlockDescription::SPLIT_ROW_STORE);
--- End diff --
Just kidding. We need the default block type to be `SPLIT` because, if it's not, then the now-required partial bulk insert method which has not been implemented in the packed row store will cause a crash during query execution..
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
@hbdeshmukh What is the site you use with directions to merge?
I have the following error when I try to push to apache's master:
```
marc@marc-laptop:incubator-quickstep (master)*$ git pull upstream master
From https://github.com/apache/incubator-quickstep
* branch master -> FETCH_HEAD
Updating ea5f28b..178ed4b
Fast-forward
cli/CommandExecutor.cpp | 5 ++--
cli/QuickstepCli.cpp | 17 ++++++------
query_execution/PolicyEnforcerBase.cpp | 6 ++---
query_execution/PolicyEnforcerBase.hpp | 4 +--
query_execution/QueryExecutionMessages.proto | 32 ++++++++--------------
query_execution/Shiftboss.cpp | 57 ++++++++++++++++++++++------------------
query_execution/Shiftboss.hpp | 2 ++
query_execution/Worker.cpp | 28 +++++++++-----------
query_execution/Worker.hpp | 13 +++++----
query_optimizer/CMakeLists.txt | 1 -
query_optimizer/QueryProcessor.hpp | 15 +----------
relational_operators/WorkOrderFactory.cpp | 48 +++++++++++++++++----------------
relational_operators/WorkOrderFactory.hpp | 4 +++
13 files changed, 110 insertions(+), 122 deletions(-)
marc@marc-laptop:incubator-quickstep (master)*$ git checkout partial-inserts
Switched to branch 'partial-inserts'
Your branch is up-to-date with 'origin/partial-inserts'.
marc@marc-laptop:incubator-quickstep (partial-inserts)*$ git rebase master
First, rewinding head to replay your work on top of it...
Applying: Adds support for PartialBulkInserts in StorageBlocks
Applying: Set Default block type to only be the splitrow store.
marc@marc-laptop:incubator-quickstep (partial-inserts)*$ git push upstream master
remote: Permission to apache/incubator-quickstep.git denied to cramja.
fatal: unable to access 'https://github.com/apache/incubator-quickstep.git/': The requested URL returned error: 403
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
Has this error happened to anyone before on Travis @hbdeshmukh @hakanmemisoglu @zuyu ?
```
[ 64%] Building CXX object types/operations/comparisons/CMakeFiles/quickstep_types_operations_comparisons_GreaterComparison.dir/GreaterComparison.cpp.o
clang: error: unable to execute command: Killed
clang: error: clang frontend command failed due to signal (use -v to see invocation)
Ubuntu clang version 3.7.1-svn253571-1~exp1 (branches/release_37) (based on LLVM 3.7.1)
Target: x86_64-pc-linux-gnu
Thread model: posix
clang: note: diagnostic msg: PLEASE submit a bug report to http://bugs.debian.org/ and include the crash backtrace, preprocessed source, and associated run script.
clang: note: diagnostic
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/129
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
@navsan could you verify these are the changes we wanted from your branch? Then I will merge, and at a later date, remove the PackedRowStore.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
That\u2019s usually an out-of-memory error.
> On Nov 18, 2016, at 12:35, Marc S <no...@github.com> wrote:
>
> Has this error happened to anyone before on Travis @hbdeshmukh <https://github.com/hbdeshmukh> @hakanmemisoglu <https://github.com/hakanmemisoglu> @zuyu <https://github.com/zuyu> ?
>
> [ 64%] Building CXX object types/operations/comparisons/CMakeFiles/quickstep_types_operations_comparisons_GreaterComparison.dir/GreaterComparison.cpp.o
> clang: error: unable to execute command: Killed
> clang: error: clang frontend command failed due to signal (use -v to see invocation)
> Ubuntu clang version 3.7.1-svn253571-1~exp1 (branches/release_37) (based on LLVM 3.7.1)
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> clang: note: diagnostic msg: PLEASE submit a bug report to http://bugs.debian.org/ and include the crash backtrace, preprocessed source, and associated run script.
> clang: note: diagnostic
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-quickstep/pull/129#issuecomment-261606587>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACZB683CNsuyYfIADuSEHtE704BQHWgeks5q_e_ogaJpZM4Krx__>.
>
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #129: Partial inserts
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on the issue:
https://github.com/apache/incubator-quickstep/pull/129
@hbdeshmukh
I ran this with SBB 100 on cloudlab. The experiments mentioned in the header did not include the HashJoin modification, these results do.
787.11
830.48
870.63
1039.8
891.61
926.0
2125.64
988.8
1035.28
933.44
1509.11
887.47
1160.36
Total: 13985.73
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---