[GitHub] incubator-quickstep issue #27: QUICKSTEP-18: Allow BasicColumnStoreTupleStor...

Github user pateljm commented on the issue: https://github.com/apache/incubator-quickstep/pull/27 Thanks @craig-chasseur! LGTM. Ready to merge, but need you to squash all the commits into one. Can you rebase as per the instructions at:

[GitHub] incubator-quickstep issue #30: Fix conditional per-target flags for lexer

Github user pateljm commented on the issue: https://github.com/apache/incubator-quickstep/pull/30 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,

[GitHub] incubator-quickstep pull request #30: Fix conditional per-target flags for l...

Github user asfgit closed the pull request at: https://github.com/apache/incubator-quickstep/pull/30 --- 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

[GitHub] incubator-quickstep pull request #27: QUICKSTEP-18: Allow BasicColumnStoreTu...

Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/27#discussion_r67010440 --- Diff: storage/BasicColumnStoreTupleStorageSubBlock.hpp --- @@ -211,11 +213,12 @@ class BasicColumnStoreTupleStorageSubBlock : public

Re: [jira] [Commented] (QUICKSTEP-20) Add parser support for SQL window aggregation function

Great point Julian and something that I have been thinking about too. I’d love to kick off a discussion to see how we can find a way to make this work. I’d love to give a talk to the Calcite team sometime later on this summer (July?) on Quickstep and explore this very synergy. Some of the

Re: [jira] [Commented] (QUICKSTEP-20) Add parser support for SQL window aggregation function

The prerequisite for integrating a SQL parsing and/or planning front-end would be to have a lower-level interface than SQL. Imagine a text representation (in say JSON or XML) of a queries in logical or physical relational algebra. (Logical would have the logical operators, e.g. Join, and

[GitHub] incubator-quickstep pull request #33: BugFix: Update NumQueuedWorkOrders to ...

GitHub user navsan opened a pull request: https://github.com/apache/incubator-quickstep/pull/33 BugFix: Update NumQueuedWorkOrders to fix scheduling Quickstep scheduling is currently broken (since PR #14). The foreman only schedules work for one worker, leaving all other workers

[GitHub] incubator-quickstep pull request #33: BugFix: Update NumQueuedWorkOrders to ...

Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/33#discussion_r67095580 --- Diff: query_execution/PolicyEnforcer.hpp --- @@ -60,12 +61,14 @@ class PolicyEnforcer { const std::size_t num_numa_nodes,

[GitHub] incubator-quickstep issue #33: BugFix: Update NumQueuedWorkOrders to fix sch...

Github user navsan commented on the issue: https://github.com/apache/incubator-quickstep/pull/33 @hbdeshmukh I think it's rather bug-prone to maintain queue state in the Foreman. Why not just look it up in the TMB itself? I noticed that you made the same comment above the

[GitHub] incubator-quickstep issue #33: BugFix: Update NumQueuedWorkOrders to fix sch...

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/33 LGTM, merging. --- 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

[GitHub] incubator-quickstep issue #33: BugFix: Update NumQueuedWorkOrders to fix sch...

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/33 That's true. I don't think it is too bug-prone. We just need to just increment and decrement the count at a couple of places (which I missed, admittedly). What TMB gives us

[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...

Github user hbdeshmukh commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/32#discussion_r67089282 --- Diff: query_execution/QueryExecutionMessages.proto --- @@ -20,11 +20,16 @@ package quickstep.serialization; message EmptyMessage {

[GitHub] incubator-quickstep issue #5: DO NOT MERGE: CI w/ gRPC To Test the Distribut...

Github user navsan commented on the issue: https://github.com/apache/incubator-quickstep/pull/5 I’m not sure why the gflags-shared target already exists. Is it because gRPC has its own gflags? If so, how does the static target work? @Zuyu, I don’t think I’ll be able to

[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/32 Hi @zuyu You are right that at this point we can keep the two proto message classes combined. However in the future, in order to add more information to the normal WorkOrder

[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/32 Even for the info you mentioned, `RebuildWorkOrder` could also have these stored in `proto`. --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] incubator-quickstep issue #33: BugFix: Update NumQueuedWorkOrders to fix sch...

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/33 Thanks @navsan and @rogersjeffreyl for fixing this. This was an oversight on my part. I checked the correctness of queries for PR #14, but I should have also tested the performance,

[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...

Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/32#discussion_r67067611 --- Diff: query_execution/QueryExecutionMessages.proto --- @@ -20,11 +20,16 @@ package quickstep.serialization; message EmptyMessage { }

[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...

Github user zuyu commented on a diff in the pull request: https://github.com/apache/incubator-quickstep/pull/32#discussion_r67067803 --- Diff: query_execution/Worker.cpp --- @@ -47,13 +49,32 @@ void Worker::run() { } ClientIDMap *thread_id_map =

Re: Guideline To Update Copyright Header

Just make sure that you do the right thing with the files copyrighted by somebody other that Pivotal and Quickstep Technologies. For example, I definitely see Google and a few others in the codebase. Thanks, Roman. On Mon, Jun 13, 2016 at 4:41 PM, Jignesh Patel wrote: >> +1

Re: Guideline To Update Copyright Header

Following up on Roman’s excellent comment, I took all the copyright notices under third_party and collected them into a new NOTICE file (see below). Notice that there are comments within [Square Braces]. This will make it easier for us to update the NOTICE file if we chose to upgrade some of

[GitHub] incubator-quickstep issue #5: DO NOT MERGE: CI w/ gRPC To Test the Distribut...

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/5 @navsan We need to fix the `cmake` error regarding `gflags-shared` (#13) when `ENABLE_DISTRIBUTED=ON`: https://travis-ci.org/apache/incubator-quickstep/jobs/137651757#L3727 --- If

[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/32 @hbdeshmukh Since the time field in `WorkOrderCompletion` proto is optional, we still do not need to split into two. --- If your project is set up for it, you can reply to this email and