[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user pateljm commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 LGTM. Merging. Let's handle any remaining issues re. reordering parameters in a separate PR. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 > @hbdeshmukh Have you try to use std::vector instead of std::vector? Using ``const QueryHandle*`` doesn't work because we extract the DAG from it. On the DAG

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user pateljm commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 @zuyu Thanks for a very thorough pass. Thanks @hbdeshmukh for your patience and working with these suggestions in advance! --- If your project is set up for it, you can reply to this

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 @hbdeshmukh I did one pass, and I would highlight two things: * Please remove some duplicated doxygen in operator header files, as a part of incorrect auto-rebasing. * Emphasis the

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 @hbdeshmukh Have you try to use `std::vector` instead of `std::vector`? I believe we don't need to change anything in `QueryHandle` after query optimization. Thanks!

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 Hi @zuyu Reset the changes in ExecutionGenerator cpp and InsertDestination as per your suggestion. Please take another look. --- If your project is set up for it, you can

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 @hbdeshmukh I see the changes in `ExecutionGenerator` and `InsertDestination`, but to me, it is a overkill. We could do it much simpler using the only `query_id` in `QueryContext`

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 I am sorry, I take the part about InsertDestination back. I am indeed setting the query ID passed to the InsertDestination in ``ExecutionGenerator`` and not in ``QueryContext``.

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 FYI, In `optimizer/ExecutionGenerator`, we set `query_id` in `QueryContext` proto, not `QueryContext` itself. So, I think in this PR, we don't need to add `query_id` in

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user hbdeshmukh commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 We pass query ID in ``QueryContext`` so that it serves as a single point of entrance for propagating query ID to any other data structures or classes specific to that query. e.g. In

[GitHub] incubator-quickstep issue #14: QUICKSTEP-8: Long running Foreman thread

Github user zuyu commented on the issue: https://github.com/apache/incubator-quickstep/pull/14 @hbdeshmukh We have to rebase again, as we had "git merge" again. --- 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