You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by shixuan-fan <gi...@git.apache.org> on 2016/06/23 21:27:55 UTC

[GitHub] incubator-quickstep pull request #39: QUICKSTEP-20: Resolver support for Win...

GitHub user shixuan-fan opened a pull request:

    https://github.com/apache/incubator-quickstep/pull/39

    QUICKSTEP-20: Resolver support for Window Aggregate Function

    Now the resolver could form a logical plan for window aggregate function based on the parsed result.
    
    Suppose we have the following query:
    SELECT attr0, avg(attr1) OVER
    (PARTITION BY attr2
    ORDER BY attr3
    ROWS BETWEEN 3 PRECEDING AND 3 FOLLOWING)
    FROM table1;
    
    The logical plan will look like this:
    input_table -> sort on (attr2, attr3) -> window aggregate -> project on (attr0, avg(attr1)).
    
    Two new classes are introduced: quickstep::optimizer::expressions::WindowAggregateFunction and quickstep::optimizer::logical::WindowAggregate:
      -WindowAggregateFunction will keep the function information and window information. 
      -WindowAggregate will store the input logical node and an alias wrapper of WindowAggregateFunction.
    
    In current version, we only support one window aggregate function. When the PhysicalPlanGenerator sees a logical node with type kWindowAggregate, it will throw an error saying that "window aggregate function is not supported".

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/apache/incubator-quickstep SQL-window-aggregation

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-quickstep/pull/39.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 #39
    
----
commit ae66fbe467cfad0667efe87412f472e673c7cdc7
Author: shixuan <sh...@wisc.edu>
Date:   2016-06-21T20:08:52Z

    Window Aggregation Function in optimizer::expression added
    The resolver could understand optional window clause.
    The resolver could understand window aggregation functions.
    Only one window aggregation function is allowed

commit a540e82ecf64a0c1dffaa6a338b6a69966724bb3
Author: Shixuan Fan <sh...@apache.org>
Date:   2016-06-21T21:12:19Z

    add has_window_aggregate_per_expression and update it while resolving SELECT list

commit 9434e6c730023d66728e59482259ee8da7df514c
Author: shixuan-fan <sh...@apache.org>
Date:   2016-06-22T14:49:24Z

    Pass the reference of window_aggregation_expressions into ExpressionResolutionInfo to keep it updated as resolving

commit 614d17deed54b26c250301fbb305cd2811c87909
Author: shixuan-fan <sh...@apache.org>
Date:   2016-06-22T20:19:28Z

    Added window aggregation class in logical

commit 9ac2fb0d7544475a3c76e81e28ba59db4378aef1
Author: shixuan-fan <sh...@apache.org>
Date:   2016-06-23T14:40:44Z

    added window name checking: duplicate definition and undefined

commit 160761c82407d6e6da2283fabdffae68607a7c08
Author: shixuan-fan <sh...@apache.org>
Date:   2016-06-23T15:59:42Z

    Formed logical plan.
    A window aggregate function could form a logical plan now.
    It will be intercepted by the stratagy in physical plan generator.

commit 5aa705835ced856dba35cbfd09d6fd95c1f99be2
Author: shixuan-fan <sh...@apache.org>
Date:   2016-06-23T20:09:01Z

    Added unittest for resolver and logical generator

commit 0210099e0655813311a973581f70cb69886320d9
Author: shixuan-fan <sh...@apache.org>
Date:   2016-06-23T21:03:02Z

    fixed cpplint issue

----


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68463327
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.cpp ---
    @@ -0,0 +1,193 @@
    +/**
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/expressions/WindowAggregateFunction.hpp"
    +
    +#include <string>
    +#include <utility>
    +#include <vector>
    +
    +#include "expressions/aggregation/AggregateFunction.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "types/Type.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace expressions {
    +
    +bool WindowAggregateFunction::isNullable() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return return_type->isNullable();
    +}
    +
    +const Type& WindowAggregateFunction::getValueType() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return *return_type;
    +}
    +
    +WindowAggregateFunctionPtr WindowAggregateFunction::Create(
    +    const ::quickstep::AggregateFunction &window_aggregate,
    +    const std::vector<ScalarPtr> &arguments,
    +    const WindowInfo window_info,
    +    const std::string window_name,
    --- End diff --
    
    Both `window_info` and `window_name` would have an unnecessary copy on the stack.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68329581
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -209,6 +227,26 @@ struct Resolver::QueryAggregationInfo {
       std::vector<E::AliasPtr> aggregate_expressions;
     };
     
    +struct Resolver::WindowPlan {
    +  explicit WindowPlan(E::WindowInfo window_info_in,
    +                      L::LogicalPtr logical_plan_in)
    --- End diff --
    
    Ditto for using `&` for a `shared_ptr`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68464557
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -1684,13 +1857,45 @@ L::LogicalPtr Resolver::resolveJoinedTableReference(
       THROW_SQL_ERROR_AT(&joined_table_reference) << "Full outer join is not supported yet";
     }
     
    +L::LogicalPtr Resolver::resolveSortInWindow(
    +    const L::LogicalPtr &logical_plan,
    +    const E::WindowInfo &window_info) {
    +  // Sort the table by (p_key, o_key)
    +  std::vector<E::AttributeReferencePtr> sort_attributes(window_info.partition_by_attributes);
    +  sort_attributes.insert(sort_attributes.end(),
    +                         window_info.order_by_attributes.begin(),
    +                         window_info.order_by_attributes.end());
    +
    +  std::vector<bool> sort_directions(
    +      window_info.partition_by_attributes.size(), true);
    +  sort_directions.insert(sort_directions.end(),
    +                         window_info.order_by_directions.begin(),
    +                         window_info.order_by_directions.end());
    +
    +  std::vector<bool> sort_nulls_first(
    +      window_info.partition_by_attributes.size(), false);
    +  sort_nulls_first.insert(sort_nulls_first.end(),
    +                          window_info.nulls_first.begin(),
    +                          window_info.nulls_first.end());
    +
    +  L::LogicalPtr sorted_logical_plan =
    +      L::Sort::Create(logical_plan,
    +                      sort_attributes,
    +                      sort_directions,
    +                      sort_nulls_first,
    --- End diff --
    
    That's totally doable. Let's fix that in another small separate PR to make it more readable.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68331984
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -1684,13 +1857,45 @@ L::LogicalPtr Resolver::resolveJoinedTableReference(
       THROW_SQL_ERROR_AT(&joined_table_reference) << "Full outer join is not supported yet";
     }
     
    +L::LogicalPtr Resolver::resolveSortInWindow(
    +    const L::LogicalPtr &logical_plan,
    +    const E::WindowInfo &window_info) {
    +  // Sort the table by (p_key, o_key)
    +  std::vector<E::AttributeReferencePtr> sort_attributes(window_info.partition_by_attributes);
    +  sort_attributes.insert(sort_attributes.end(),
    +                         window_info.order_by_attributes.begin(),
    +                         window_info.order_by_attributes.end());
    +
    +  std::vector<bool> sort_directions(
    +      window_info.partition_by_attributes.size(), true);
    +  sort_directions.insert(sort_directions.end(),
    +                         window_info.order_by_directions.begin(),
    +                         window_info.order_by_directions.end());
    +
    +  std::vector<bool> sort_nulls_first(
    +      window_info.partition_by_attributes.size(), false);
    +  sort_nulls_first.insert(sort_nulls_first.end(),
    +                          window_info.nulls_first.begin(),
    +                          window_info.nulls_first.end());
    +
    +  L::LogicalPtr sorted_logical_plan =
    +      L::Sort::Create(logical_plan,
    +                      sort_attributes,
    +                      sort_directions,
    +                      sort_nulls_first,
    --- End diff --
    
    We may want to use the move semantics for above three `vector`s.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68804311
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -973,8 +1011,36 @@ L::LogicalPtr Resolver::resolveSelect(
             logical_plan, resolvePredicate(parse_predicate, &expr_resolution_info));
       }
     
    +  // Resolve WINDOW clause.
    +  std::unordered_map<std::string, WindowPlan> sorted_window_map;
    +  if (select_query.window_list() != nullptr) {
    +    if (select_query.window_list()->size() > 1) {
    +      THROW_SQL_ERROR_AT(&(*select_query.window_list()->begin()))
    +        << "Currently we don't support multiple window aggregation functions";
    +    }
    +
    +    // Sort the table according to the window.
    +    for (const ParseWindow &window : *select_query.window_list()) {
    +      // Check for duplicate definition.
    +      // Currently this is useless since we only support one window.
    +      if (sorted_window_map.find(window.name()->value()) != sorted_window_map.end()) {
    +        THROW_SQL_ERROR_AT(window.name())
    +            << "Duplicate definition of window " << window.name()->value();
    +      }
    +
    +      E::WindowInfo resolved_window = resolveWindow(window, *name_resolver);
    +      L::LogicalPtr sorted_logical_plan = resolveSortInWindow(logical_plan,
    +                                                              resolved_window);
    +
    +      WindowPlan window_plan(resolved_window, sorted_logical_plan);
    --- End diff --
    
    `cpplint` sometimes treat the rvalue as a logical `and` expression, so we need to workaround by adding `// NOLINT(whitespace/operators)`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68464072
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.cpp ---
    @@ -0,0 +1,193 @@
    +/**
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/expressions/WindowAggregateFunction.hpp"
    +
    +#include <string>
    +#include <utility>
    +#include <vector>
    +
    +#include "expressions/aggregation/AggregateFunction.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "types/Type.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace expressions {
    +
    +bool WindowAggregateFunction::isNullable() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return return_type->isNullable();
    +}
    +
    +const Type& WindowAggregateFunction::getValueType() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return *return_type;
    +}
    +
    +WindowAggregateFunctionPtr WindowAggregateFunction::Create(
    +    const ::quickstep::AggregateFunction &window_aggregate,
    +    const std::vector<ScalarPtr> &arguments,
    +    const WindowInfo window_info,
    +    const std::string window_name,
    --- End diff --
    
    Sure. I've fixed that in my last commit :)


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68328569
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -973,8 +1011,36 @@ L::LogicalPtr Resolver::resolveSelect(
             logical_plan, resolvePredicate(parse_predicate, &expr_resolution_info));
       }
     
    +  // Resolve WINDOW clause.
    +  std::unordered_map<std::string, WindowPlan> sorted_window_map;
    +  if (select_query.window_list() != nullptr) {
    +    if (select_query.window_list()->size() > 1) {
    +      THROW_SQL_ERROR_AT(&(*select_query.window_list()->begin()))
    +        << "Currently we don't support multiple window aggregation functions";
    +    }
    +
    +    // Sort the table according to the window.
    +    for (const ParseWindow &window : *select_query.window_list()) {
    +      // Check for duplicate definition.
    +      // Currently this is useless since we only support one window.
    +      if (sorted_window_map.find(window.name()->value()) != sorted_window_map.end()) {
    +        THROW_SQL_ERROR_AT(window.name())
    +            << "Duplicate definition of window " << window.name()->value();
    +      }
    +
    +      E::WindowInfo resolved_window = resolveWindow(window, *name_resolver);
    +      L::LogicalPtr sorted_logical_plan = resolveSortInWindow(logical_plan,
    +                                                              resolved_window);
    +
    +      WindowPlan window_plan(resolved_window, sorted_logical_plan);
    --- End diff --
    
    We should use the `move` semantics for `resolved_window` at least.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-quickstep/pull/39


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68325351
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.cpp ---
    @@ -0,0 +1,193 @@
    +/**
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/expressions/WindowAggregateFunction.hpp"
    +
    +#include <string>
    +#include <utility>
    +#include <vector>
    +
    +#include "expressions/aggregation/AggregateFunction.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "types/Type.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace expressions {
    +
    +bool WindowAggregateFunction::isNullable() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return return_type->isNullable();
    +}
    +
    +const Type& WindowAggregateFunction::getValueType() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return *return_type;
    +}
    +
    +WindowAggregateFunctionPtr WindowAggregateFunction::Create(
    +    const ::quickstep::AggregateFunction &window_aggregate,
    +    const std::vector<ScalarPtr> &arguments,
    +    const WindowInfo window_info,
    +    const std::string window_name,
    --- End diff --
    
    For above two arguments, do we intend to create a deep copy of both objects?


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68329454
  
    --- Diff: query_optimizer/logical/WindowAggregate.hpp ---
    @@ -0,0 +1,123 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/Alias.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/NamedExpression.hpp"
    +#include "query_optimizer/logical/Logical.hpp"
    +#include "query_optimizer/logical/LogicalType.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace logical {
    +
    +/** \addtogroup OptimizerLogical
    + *  @{
    + */
    +
    +class WindowAggregate;
    +typedef std::shared_ptr<const WindowAggregate> WindowAggregatePtr;
    +
    +/**
    + * @brief Window aggregate operator that computes window aggregate expressions.
    + */
    +class WindowAggregate : public Logical {
    + public:
    +  LogicalType getLogicalType() const override {
    +    return LogicalType::kWindowAggregate;
    +  }
    +
    +  std::string getName() const override { return "WindowAggregate"; }
    +
    +  /**
    +   * @return The input logical node.
    +   */
    +  const LogicalPtr& input() const { return input_; }
    +
    +  /**
    +   * @return PARTITION BY expressions.
    +   */
    +  const expressions::AliasPtr window_aggregate_expression() const {
    +    return window_aggregate_expression_;
    +  }
    +
    +  LogicalPtr copyWithNewChildren(
    +      const std::vector<LogicalPtr> &new_children) const override;
    +
    +  LogicalPtr copyWithNewInputExpressions(
    +      const std::vector<expressions::ExpressionPtr> &input_expressions) const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getOutputAttributes() const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @brief Creates an Aggregate logical node.
    +   *
    +   * @param input The input node.
    +   * @param window_aggregate_expression The window aggregate expression.
    +   * @return An immutable WindowAggregate node.
    +   */
    +  static WindowAggregatePtr Create(
    +      LogicalPtr input,
    +      const expressions::AliasPtr window_aggregate_expression) {
    +    return WindowAggregatePtr(new WindowAggregate(input, window_aggregate_expression));
    +  }
    +
    + protected:
    +  void getFieldStringItems(
    +      std::vector<std::string> *inline_field_names,
    +      std::vector<std::string> *inline_field_values,
    +      std::vector<std::string> *non_container_child_field_names,
    +      std::vector<OptimizerTreeBaseNodePtr> *non_container_child_fields,
    +      std::vector<std::string> *container_child_field_names,
    +      std::vector<std::vector<OptimizerTreeBaseNodePtr>> *container_child_fields) const override;
    +
    + private:
    +  WindowAggregate(LogicalPtr input,
    +                  const expressions::AliasPtr window_aggregate_expression)
    --- End diff --
    
    Ditto for using `&` for `share_ptr`s.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by hakanmemisoglu <gi...@git.apache.org>.
Github user hakanmemisoglu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68422498
  
    --- Diff: query_optimizer/logical/WindowAggregate.hpp ---
    @@ -0,0 +1,123 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/Alias.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/NamedExpression.hpp"
    +#include "query_optimizer/logical/Logical.hpp"
    +#include "query_optimizer/logical/LogicalType.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace logical {
    +
    +/** \addtogroup OptimizerLogical
    + *  @{
    + */
    +
    +class WindowAggregate;
    +typedef std::shared_ptr<const WindowAggregate> WindowAggregatePtr;
    +
    +/**
    + * @brief Window aggregate operator that computes window aggregate expressions.
    + */
    +class WindowAggregate : public Logical {
    + public:
    +  LogicalType getLogicalType() const override {
    +    return LogicalType::kWindowAggregate;
    +  }
    +
    +  std::string getName() const override { return "WindowAggregate"; }
    +
    +  /**
    +   * @return The input logical node.
    +   */
    +  const LogicalPtr& input() const { return input_; }
    +
    +  /**
    +   * @return PARTITION BY expressions.
    +   */
    +  const expressions::AliasPtr window_aggregate_expression() const {
    +    return window_aggregate_expression_;
    +  }
    +
    +  LogicalPtr copyWithNewChildren(
    +      const std::vector<LogicalPtr> &new_children) const override;
    +
    +  LogicalPtr copyWithNewInputExpressions(
    +      const std::vector<expressions::ExpressionPtr> &input_expressions) const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getOutputAttributes() const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @brief Creates an Aggregate logical node.
    +   *
    +   * @param input The input node.
    +   * @param window_aggregate_expression The window aggregate expression.
    +   * @return An immutable WindowAggregate node.
    +   */
    +  static WindowAggregatePtr Create(
    +      LogicalPtr input,
    +      const expressions::AliasPtr window_aggregate_expression) {
    --- End diff --
    
    I think you should read this article.
    https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68782819
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -973,8 +1011,36 @@ L::LogicalPtr Resolver::resolveSelect(
             logical_plan, resolvePredicate(parse_predicate, &expr_resolution_info));
       }
     
    +  // Resolve WINDOW clause.
    +  std::unordered_map<std::string, WindowPlan> sorted_window_map;
    +  if (select_query.window_list() != nullptr) {
    +    if (select_query.window_list()->size() > 1) {
    +      THROW_SQL_ERROR_AT(&(*select_query.window_list()->begin()))
    +        << "Currently we don't support multiple window aggregation functions";
    +    }
    +
    +    // Sort the table according to the window.
    +    for (const ParseWindow &window : *select_query.window_list()) {
    +      // Check for duplicate definition.
    +      // Currently this is useless since we only support one window.
    +      if (sorted_window_map.find(window.name()->value()) != sorted_window_map.end()) {
    +        THROW_SQL_ERROR_AT(window.name())
    +            << "Duplicate definition of window " << window.name()->value();
    +      }
    +
    +      E::WindowInfo resolved_window = resolveWindow(window, *name_resolver);
    +      L::LogicalPtr sorted_logical_plan = resolveSortInWindow(logical_plan,
    +                                                              resolved_window);
    +
    +      WindowPlan window_plan(resolved_window, sorted_logical_plan);
    --- End diff --
    
    @zuyu Cpplint keeps reporting the following error when I changed the argument to `E::WindowInfo &&window_info_in`:
    
    > ./query_optimizer/resolver/Resolver.cpp:231:  Missing spaces around &&  [whitespace/operators] [3]
    
    
    Any idea on this?


---
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 #39: QUICKSTEP-20: Resolver support for Window Agg...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:

    https://github.com/apache/incubator-quickstep/pull/39
  
    LGTM! Will have another PR regarding the move semantics.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68328028
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -209,6 +227,26 @@ struct Resolver::QueryAggregationInfo {
       std::vector<E::AliasPtr> aggregate_expressions;
     };
     
    +struct Resolver::WindowPlan {
    +  explicit WindowPlan(E::WindowInfo window_info_in,
    +                      L::LogicalPtr logical_plan_in)
    +      : window_info(window_info_in),
    +        logical_plan(logical_plan_in) {}
    +
    +  const E::WindowInfo window_info;
    +  const L::LogicalPtr logical_plan;
    +};
    +
    +struct Resolver::WindowAggregationInfo {
    +  explicit WindowAggregationInfo(const std::unordered_map<std::string, WindowPlan> window_map_in)
    --- End diff --
    
    Please add a reference for `window_map_in`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68416644
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.cpp ---
    @@ -0,0 +1,193 @@
    +/**
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/expressions/WindowAggregateFunction.hpp"
    +
    +#include <string>
    +#include <utility>
    +#include <vector>
    +
    +#include "expressions/aggregation/AggregateFunction.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "types/Type.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace expressions {
    +
    +bool WindowAggregateFunction::isNullable() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return return_type->isNullable();
    +}
    +
    +const Type& WindowAggregateFunction::getValueType() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return *return_type;
    +}
    +
    +WindowAggregateFunctionPtr WindowAggregateFunction::Create(
    +    const ::quickstep::AggregateFunction &window_aggregate,
    +    const std::vector<ScalarPtr> &arguments,
    +    const WindowInfo window_info,
    +    const std::string window_name,
    --- End diff --
    
    Do you mean (window_aggregate and arguments) or (window_info and window_name)? The "above two" is a little bit vague...


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68454552
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -1684,13 +1857,45 @@ L::LogicalPtr Resolver::resolveJoinedTableReference(
       THROW_SQL_ERROR_AT(&joined_table_reference) << "Full outer join is not supported yet";
     }
     
    +L::LogicalPtr Resolver::resolveSortInWindow(
    +    const L::LogicalPtr &logical_plan,
    +    const E::WindowInfo &window_info) {
    +  // Sort the table by (p_key, o_key)
    +  std::vector<E::AttributeReferencePtr> sort_attributes(window_info.partition_by_attributes);
    +  sort_attributes.insert(sort_attributes.end(),
    +                         window_info.order_by_attributes.begin(),
    +                         window_info.order_by_attributes.end());
    +
    +  std::vector<bool> sort_directions(
    +      window_info.partition_by_attributes.size(), true);
    +  sort_directions.insert(sort_directions.end(),
    +                         window_info.order_by_directions.begin(),
    +                         window_info.order_by_directions.end());
    +
    +  std::vector<bool> sort_nulls_first(
    +      window_info.partition_by_attributes.size(), false);
    +  sort_nulls_first.insert(sort_nulls_first.end(),
    +                          window_info.nulls_first.begin(),
    +                          window_info.nulls_first.end());
    +
    +  L::LogicalPtr sorted_logical_plan =
    +      L::Sort::Create(logical_plan,
    +                      sort_attributes,
    +                      sort_directions,
    +                      sort_nulls_first,
    --- End diff --
    
    In `quickstep::optimizer::logical::Sort::Create`, the argument is passed by const lvalue reference. So I just followed the code written before in this file.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68405727
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.cpp ---
    @@ -0,0 +1,193 @@
    +/**
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/expressions/WindowAggregateFunction.hpp"
    +
    +#include <string>
    +#include <utility>
    +#include <vector>
    +
    +#include "expressions/aggregation/AggregateFunction.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "types/Type.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace expressions {
    +
    +bool WindowAggregateFunction::isNullable() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return return_type->isNullable();
    +}
    +
    +const Type& WindowAggregateFunction::getValueType() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return *return_type;
    +}
    +
    +WindowAggregateFunctionPtr WindowAggregateFunction::Create(
    +    const ::quickstep::AggregateFunction &window_aggregate,
    +    const std::vector<ScalarPtr> &arguments,
    +    const WindowInfo window_info,
    +    const std::string window_name,
    --- End diff --
    
    Yeah you are right, maybe a reference would be better :)


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68804650
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -973,8 +1011,36 @@ L::LogicalPtr Resolver::resolveSelect(
             logical_plan, resolvePredicate(parse_predicate, &expr_resolution_info));
       }
     
    +  // Resolve WINDOW clause.
    +  std::unordered_map<std::string, WindowPlan> sorted_window_map;
    +  if (select_query.window_list() != nullptr) {
    +    if (select_query.window_list()->size() > 1) {
    +      THROW_SQL_ERROR_AT(&(*select_query.window_list()->begin()))
    +        << "Currently we don't support multiple window aggregation functions";
    +    }
    +
    +    // Sort the table according to the window.
    +    for (const ParseWindow &window : *select_query.window_list()) {
    +      // Check for duplicate definition.
    +      // Currently this is useless since we only support one window.
    +      if (sorted_window_map.find(window.name()->value()) != sorted_window_map.end()) {
    +        THROW_SQL_ERROR_AT(window.name())
    +            << "Duplicate definition of window " << window.name()->value();
    +      }
    +
    +      E::WindowInfo resolved_window = resolveWindow(window, *name_resolver);
    +      L::LogicalPtr sorted_logical_plan = resolveSortInWindow(logical_plan,
    +                                                              resolved_window);
    +
    +      WindowPlan window_plan(resolved_window, sorted_logical_plan);
    --- End diff --
    
    Thanks! @zuyu 


---
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 #39: QUICKSTEP-20: Resolver support for Window Agg...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:

    https://github.com/apache/incubator-quickstep/pull/39
  
    @shixuan-fan I did one pass. Let me know if you are ready for another pass. 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 pull request #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68326431
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.hpp ---
    @@ -0,0 +1,246 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/ExpressionType.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +
    +class AggregateFunction;
    +class Type;
    +
    +namespace optimizer {
    +namespace expressions {
    +
    +/** \addtogroup OptimizerExpressions
    + *  @{
    + */
    +
    +struct WindowFrameInfo {
    +  /**
    +   * @brief Cosntructor.
    +   *
    +   * @param is_row_in True if this window frame is defined by ROWS, false if
    +   *                  defined by RANGE.
    +   * @param num_preceding_in The number of preceding tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   * @param num_following_in The number of following tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   **/
    +  explicit WindowFrameInfo(const bool is_row_in,
    +                           const int num_preceding_in,
    +                           const int num_following_in)
    +      : is_row(is_row_in),
    +        num_preceding(num_preceding_in),
    +        num_following(num_following_in) {}
    +
    +  const bool is_row;
    +  const int num_preceding;
    +  const int num_following;
    +};
    +
    +struct WindowInfo {
    +  /**
    +   * @brief Constructor.
    +   *
    +   * @param partition_by_attributes_in The partition keys for the window.
    +   * @param order_by_attributes_in The order keys for the window.
    +   * @param order_by_directions_in The order direction for order key.
    +   * @param nulls_first_in The nulls' position for order key.
    +   * @param frame_info_in The window frame information for the window. Null
    +   *        means there is no explicit window frame definition.
    +   **/
    +  explicit WindowInfo(const std::vector<AttributeReferencePtr> &partition_by_attributes_in,
    +                      const std::vector<AttributeReferencePtr> &order_by_attributes_in,
    +                      const std::vector<bool> &order_by_directions_in,
    +                      const std::vector<bool> &nulls_first_in,
    +                      const WindowFrameInfo *frame_info_in)
    +      : partition_by_attributes(partition_by_attributes_in),
    +        order_by_attributes(order_by_attributes_in),
    +        order_by_directions(order_by_directions_in),
    +        nulls_first(nulls_first_in),
    +        frame_info(frame_info_in) {}
    +
    +  const std::vector<AttributeReferencePtr> partition_by_attributes;
    +  const std::vector<AttributeReferencePtr> order_by_attributes;
    +  const std::vector<bool> order_by_directions;
    +  const std::vector<bool> nulls_first;
    +  const WindowFrameInfo *frame_info;
    +};
    +
    +class WindowAggregateFunction;
    +typedef std::shared_ptr<const WindowAggregateFunction> WindowAggregateFunctionPtr;
    +
    +/**
    + * @brief Represents a window aggregate function and its arguments in the
    + *        optimizer. This class wraps some of the functionality from
    + *        quickstep::AggregateFunction and represents a particular instance
    + *        of an aggregate during query optimization.
    + **/
    +class WindowAggregateFunction : public Expression {
    + public:
    +  /**
    +   * @brief Destructor.
    +   */
    +  ~WindowAggregateFunction() override {}
    +
    +  ExpressionType getExpressionType() const override {
    +    return ExpressionType::kWindowAggregateFunction;
    +  }
    +
    +  std::string getName() const override {
    +    return "WindowAggregateFunction";
    +  }
    +
    +  const Type& getValueType() const override;
    +
    +  bool isConstant() const override {
    +    // Window aggregate function is never considered as a constant expression.
    +    return false;
    +  }
    +
    +  ExpressionPtr copyWithNewChildren(
    +      const std::vector<ExpressionPtr> &new_children) const override;
    +
    +  std::vector<AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @return Whether the type of the return value is nullable.
    +   **/
    +  bool isNullable() const;
    +
    +  /**
    +   * @return The WindowAggregateFunction singleton (from the expression system)
    +   *         for this node.
    +   **/
    +  inline const ::quickstep::AggregateFunction& window_aggregate() const {
    +    return window_aggregate_;
    +  }
    +
    +  /**
    +   * @return The list of scalar arguments to this aggregate.
    +   **/
    +  inline const std::vector<ScalarPtr>& arguments() const {
    +    return arguments_;
    +  }
    +
    +  /**
    +   * @return The window info of this window aggregate function.
    +   **/
    +  inline const WindowInfo window_info() const {
    +    return window_info_;
    +  }
    +
    +  /**
    +   * @return The name of the window.
    +   **/
    +  inline const std::string window_name() const {
    +    return window_name_;
    +  }
    +
    +  /**
    +   * @return Whether this is a DISTINCT aggregation.
    +   **/
    +  inline bool is_distinct() const {
    +    return is_distinct_;
    +  }
    +
    +  /**
    +   * @brief Create a new WindowAggregateFunction by directly defined window.
    +   *
    +   * @warning It is an error to call this with arguments that the given
    +   *          aggregate can not apply to.
    +   *
    +   * @param aggregate The underlying WindowAggregateFunction from the expression
    +   *        system.
    +   * @param arguments A list of arguments to the window aggregate function.
    +   * @param window_info The window info of the window aggregate function.
    +   * @param is_distinct Whether this is a DISTINCT aggregation.
    +   * @return A new AggregateFunctionPtr.
    +   **/
    +  static WindowAggregateFunctionPtr Create(const ::quickstep::AggregateFunction &window_aggregate,
    +                                           const std::vector<ScalarPtr> &arguments,
    +                                           const WindowInfo window_info,
    +                                           const std::string window_name,
    --- End diff --
    
    Ditto for the deep copies for two above objects.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68332407
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -2362,17 +2568,19 @@ E::ScalarPtr Resolver::resolveSimpleCaseExpression(
     
     // TODO(chasseur): For now this only handles resolving aggregate functions. In
     // the future it should be extended to resolve scalar functions as well.
    +// TODO(Shixuan): This will handle resolving window aggregation function as well,
    +// which could be extended to general scalar functions.
     E::ScalarPtr Resolver::resolveFunctionCall(
         const ParseFunctionCall &parse_function_call,
         ExpressionResolutionInfo *expression_resolution_info) {
    -  std::string function_name = ToLower(parse_function_call.name()->value());
    -
    -  // TODO(Shixuan): Add support for window aggregation function.
    +  // Check if it is a window function.
       if (parse_function_call.isWindow()) {
    -    THROW_SQL_ERROR_AT(&parse_function_call)
    -        << "Window Aggregation Function is not supported currently";
    +    return resolveWindowAggregateFunction(parse_function_call,
    +                                          expression_resolution_info);
       }
     
    +  std::string function_name = ToLower(parse_function_call.name()->value());
    --- End diff --
    
    I know we moved this line, but we actually could mark `const` for `function_name`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68330834
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -992,6 +1059,29 @@ L::LogicalPtr Resolver::resolveSelect(
       SelectListInfo select_list_info(select_list_expressions,
                                       has_aggregate_per_expression);
     
    +  // Create window aggregate node if needed
    +  for (E::AliasPtr alias : window_aggregation_info.window_aggregate_expressions) {
    +    E::WindowAggregateFunctionPtr window_aggregate_function;
    +    if (!E::SomeWindowAggregateFunction::MatchesWithConditionalCast(alias->expression(),
    +                                                                    &window_aggregate_function)) {
    +      THROW_SQL_ERROR()
    +          << "Unexpected expression in window aggregation function";
    +    }
    +    L::LogicalPtr sorted_logical_plan;
    +
    +    // Get the sorted logical plan
    +    const std::string window_name = window_aggregate_function->window_name();
    +    if (window_name != "") {
    --- End diff --
    
    Suggest to use `!window_name.empty()`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68411758
  
    --- Diff: query_optimizer/logical/WindowAggregate.hpp ---
    @@ -0,0 +1,123 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/Alias.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/NamedExpression.hpp"
    +#include "query_optimizer/logical/Logical.hpp"
    +#include "query_optimizer/logical/LogicalType.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace logical {
    +
    +/** \addtogroup OptimizerLogical
    + *  @{
    + */
    +
    +class WindowAggregate;
    +typedef std::shared_ptr<const WindowAggregate> WindowAggregatePtr;
    +
    +/**
    + * @brief Window aggregate operator that computes window aggregate expressions.
    + */
    +class WindowAggregate : public Logical {
    + public:
    +  LogicalType getLogicalType() const override {
    +    return LogicalType::kWindowAggregate;
    +  }
    +
    +  std::string getName() const override { return "WindowAggregate"; }
    +
    +  /**
    +   * @return The input logical node.
    +   */
    +  const LogicalPtr& input() const { return input_; }
    +
    +  /**
    +   * @return PARTITION BY expressions.
    +   */
    +  const expressions::AliasPtr window_aggregate_expression() const {
    +    return window_aggregate_expression_;
    +  }
    +
    +  LogicalPtr copyWithNewChildren(
    +      const std::vector<LogicalPtr> &new_children) const override;
    +
    +  LogicalPtr copyWithNewInputExpressions(
    +      const std::vector<expressions::ExpressionPtr> &input_expressions) const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getOutputAttributes() const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @brief Creates an Aggregate logical node.
    +   *
    +   * @param input The input node.
    +   * @param window_aggregate_expression The window aggregate expression.
    +   * @return An immutable WindowAggregate node.
    +   */
    +  static WindowAggregatePtr Create(
    +      LogicalPtr input,
    +      const expressions::AliasPtr window_aggregate_expression) {
    --- End diff --
    
    Could you please briefly explain this?


---
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 #39: QUICKSTEP-20: Resolver support for Window Agg...

Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on the issue:

    https://github.com/apache/incubator-quickstep/pull/39
  
    @zuyu Thanks for all your efforts on this! I've fixed them and your second pass will be really appreciated! No hurry. Take your time and have a great weekend :)


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68326516
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.hpp ---
    @@ -0,0 +1,246 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/ExpressionType.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +
    +class AggregateFunction;
    +class Type;
    +
    +namespace optimizer {
    +namespace expressions {
    +
    +/** \addtogroup OptimizerExpressions
    + *  @{
    + */
    +
    +struct WindowFrameInfo {
    +  /**
    +   * @brief Cosntructor.
    +   *
    +   * @param is_row_in True if this window frame is defined by ROWS, false if
    +   *                  defined by RANGE.
    +   * @param num_preceding_in The number of preceding tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   * @param num_following_in The number of following tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   **/
    +  explicit WindowFrameInfo(const bool is_row_in,
    +                           const int num_preceding_in,
    +                           const int num_following_in)
    +      : is_row(is_row_in),
    +        num_preceding(num_preceding_in),
    +        num_following(num_following_in) {}
    +
    +  const bool is_row;
    +  const int num_preceding;
    +  const int num_following;
    +};
    +
    +struct WindowInfo {
    +  /**
    +   * @brief Constructor.
    +   *
    +   * @param partition_by_attributes_in The partition keys for the window.
    +   * @param order_by_attributes_in The order keys for the window.
    +   * @param order_by_directions_in The order direction for order key.
    +   * @param nulls_first_in The nulls' position for order key.
    +   * @param frame_info_in The window frame information for the window. Null
    +   *        means there is no explicit window frame definition.
    +   **/
    +  explicit WindowInfo(const std::vector<AttributeReferencePtr> &partition_by_attributes_in,
    +                      const std::vector<AttributeReferencePtr> &order_by_attributes_in,
    +                      const std::vector<bool> &order_by_directions_in,
    +                      const std::vector<bool> &nulls_first_in,
    +                      const WindowFrameInfo *frame_info_in)
    +      : partition_by_attributes(partition_by_attributes_in),
    +        order_by_attributes(order_by_attributes_in),
    +        order_by_directions(order_by_directions_in),
    +        nulls_first(nulls_first_in),
    +        frame_info(frame_info_in) {}
    +
    +  const std::vector<AttributeReferencePtr> partition_by_attributes;
    +  const std::vector<AttributeReferencePtr> order_by_attributes;
    +  const std::vector<bool> order_by_directions;
    +  const std::vector<bool> nulls_first;
    +  const WindowFrameInfo *frame_info;
    +};
    +
    +class WindowAggregateFunction;
    +typedef std::shared_ptr<const WindowAggregateFunction> WindowAggregateFunctionPtr;
    +
    +/**
    + * @brief Represents a window aggregate function and its arguments in the
    + *        optimizer. This class wraps some of the functionality from
    + *        quickstep::AggregateFunction and represents a particular instance
    + *        of an aggregate during query optimization.
    + **/
    +class WindowAggregateFunction : public Expression {
    + public:
    +  /**
    +   * @brief Destructor.
    +   */
    +  ~WindowAggregateFunction() override {}
    +
    +  ExpressionType getExpressionType() const override {
    +    return ExpressionType::kWindowAggregateFunction;
    +  }
    +
    +  std::string getName() const override {
    +    return "WindowAggregateFunction";
    +  }
    +
    +  const Type& getValueType() const override;
    +
    +  bool isConstant() const override {
    +    // Window aggregate function is never considered as a constant expression.
    +    return false;
    +  }
    +
    +  ExpressionPtr copyWithNewChildren(
    +      const std::vector<ExpressionPtr> &new_children) const override;
    +
    +  std::vector<AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @return Whether the type of the return value is nullable.
    +   **/
    +  bool isNullable() const;
    +
    +  /**
    +   * @return The WindowAggregateFunction singleton (from the expression system)
    +   *         for this node.
    +   **/
    +  inline const ::quickstep::AggregateFunction& window_aggregate() const {
    +    return window_aggregate_;
    +  }
    +
    +  /**
    +   * @return The list of scalar arguments to this aggregate.
    +   **/
    +  inline const std::vector<ScalarPtr>& arguments() const {
    +    return arguments_;
    +  }
    +
    +  /**
    +   * @return The window info of this window aggregate function.
    +   **/
    +  inline const WindowInfo window_info() const {
    +    return window_info_;
    +  }
    +
    +  /**
    +   * @return The name of the window.
    +   **/
    +  inline const std::string window_name() const {
    +    return window_name_;
    +  }
    +
    +  /**
    +   * @return Whether this is a DISTINCT aggregation.
    +   **/
    +  inline bool is_distinct() const {
    +    return is_distinct_;
    +  }
    +
    +  /**
    +   * @brief Create a new WindowAggregateFunction by directly defined window.
    +   *
    +   * @warning It is an error to call this with arguments that the given
    +   *          aggregate can not apply to.
    +   *
    +   * @param aggregate The underlying WindowAggregateFunction from the expression
    +   *        system.
    +   * @param arguments A list of arguments to the window aggregate function.
    +   * @param window_info The window info of the window aggregate function.
    +   * @param is_distinct Whether this is a DISTINCT aggregation.
    +   * @return A new AggregateFunctionPtr.
    +   **/
    +  static WindowAggregateFunctionPtr Create(const ::quickstep::AggregateFunction &window_aggregate,
    +                                           const std::vector<ScalarPtr> &arguments,
    +                                           const WindowInfo window_info,
    +                                           const std::string window_name,
    +                                           const bool is_distinct);
    +
    + protected:
    +  void getFieldStringItems(
    +      std::vector<std::string> *inline_field_names,
    +      std::vector<std::string> *inline_field_values,
    +      std::vector<std::string> *non_container_child_field_names,
    +      std::vector<OptimizerTreeBaseNodePtr> *non_container_child_fields,
    +      std::vector<std::string> *container_child_field_names,
    +      std::vector<std::vector<OptimizerTreeBaseNodePtr>> *container_child_fields) const override;
    +
    + private:
    +  /**
    +   * @brief Constructor.
    +   *
    +   * @param window_aggregate The actual AggregateFunction to use.
    +   * @param arguments A list of arguments to the window aggregate function.
    +   * @param window_info The window info of the window aggregate function.
    +   * @param is_distinct Indicates whether this is a DISTINCT aggregation.
    +   */
    +  WindowAggregateFunction(const ::quickstep::AggregateFunction &window_aggregate,
    +                          const std::vector<ScalarPtr> &arguments,
    +                          const WindowInfo window_info,
    +                          const std::string window_name,
    --- End diff --
    
    Ditto for the deep copy of two above objects.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68332529
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -2471,6 +2679,154 @@ E::ScalarPtr Resolver::resolveFunctionCall(
       return E::ToRef(aggregate_alias);
     }
     
    +E::ScalarPtr Resolver::resolveWindowAggregateFunction(
    +    const ParseFunctionCall &parse_function_call,
    +    ExpressionResolutionInfo *expression_resolution_info) {
    +  if (!expression_resolution_info->window_aggregation_info->window_aggregate_expressions.empty()) {
    +    THROW_SQL_ERROR_AT(&parse_function_call)
    +       << "Currently we only support single window aggregate in the query";
    --- End diff --
    
    Code style: please add one more whitespace indentation.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68326059
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.hpp ---
    @@ -0,0 +1,246 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/ExpressionType.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +
    +class AggregateFunction;
    +class Type;
    +
    +namespace optimizer {
    +namespace expressions {
    +
    +/** \addtogroup OptimizerExpressions
    + *  @{
    + */
    +
    +struct WindowFrameInfo {
    +  /**
    +   * @brief Cosntructor.
    +   *
    +   * @param is_row_in True if this window frame is defined by ROWS, false if
    +   *                  defined by RANGE.
    +   * @param num_preceding_in The number of preceding tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   * @param num_following_in The number of following tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   **/
    +  explicit WindowFrameInfo(const bool is_row_in,
    --- End diff --
    
    Please remove `explicit`, as we don't need it.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68325875
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.cpp ---
    @@ -0,0 +1,193 @@
    +/**
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/expressions/WindowAggregateFunction.hpp"
    +
    +#include <string>
    +#include <utility>
    +#include <vector>
    +
    +#include "expressions/aggregation/AggregateFunction.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "types/Type.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace expressions {
    +
    +bool WindowAggregateFunction::isNullable() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return return_type->isNullable();
    +}
    +
    +const Type& WindowAggregateFunction::getValueType() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return *return_type;
    +}
    +
    +WindowAggregateFunctionPtr WindowAggregateFunction::Create(
    +    const ::quickstep::AggregateFunction &window_aggregate,
    +    const std::vector<ScalarPtr> &arguments,
    +    const WindowInfo window_info,
    +    const std::string window_name,
    +    const bool is_distinct) {
    +#ifdef QUICKSTEP_DEBUG
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +  DCHECK(window_aggregate.canApplyToTypes(argument_types));
    +#endif  // QUICKSTEP_DEBUG
    +
    +  return WindowAggregateFunctionPtr(
    +      new WindowAggregateFunction(window_aggregate, arguments, window_info, window_name, is_distinct));
    +}
    +
    +ExpressionPtr WindowAggregateFunction::copyWithNewChildren(
    +    const std::vector<ExpressionPtr> &new_children) const {
    +  std::vector<ScalarPtr> new_arguments;
    +  for (const ExpressionPtr &expression_ptr : new_children) {
    +    ScalarPtr expr_as_scalar;
    +    CHECK(SomeScalar::MatchesWithConditionalCast(expression_ptr, &expr_as_scalar))
    +        << expression_ptr->toString();
    +    new_arguments.emplace_back(std::move(expr_as_scalar));
    +  }
    +
    +  return WindowAggregateFunctionPtr(
    +      new WindowAggregateFunction(window_aggregate_,
    +                                  new_arguments,
    +                                  window_info_,
    +                                  window_name_,
    +                                  is_distinct_));
    +}
    +
    +std::vector<AttributeReferencePtr> WindowAggregateFunction::getReferencedAttributes() const {
    +  std::vector<AttributeReferencePtr> referenced_attributes;
    +  for (const ScalarPtr &argument : arguments_) {
    +    const std::vector<AttributeReferencePtr> referenced_attributes_in_argument =
    +        argument->getReferencedAttributes();
    +    referenced_attributes.insert(referenced_attributes.end(),
    +                                 referenced_attributes_in_argument.begin(),
    +                                 referenced_attributes_in_argument.end());
    +  }
    +
    +  referenced_attributes.insert(referenced_attributes.end(),
    +                               window_info_.partition_by_attributes.begin(),
    +                               window_info_.partition_by_attributes.end());
    +
    +  referenced_attributes.insert(referenced_attributes.end(),
    +                               window_info_.order_by_attributes.begin(),
    +                               window_info_.order_by_attributes.end());
    +
    +  return referenced_attributes;
    +}
    +
    +void WindowAggregateFunction::getFieldStringItems(
    +    std::vector<std::string> *inline_field_names,
    +    std::vector<std::string> *inline_field_values,
    +    std::vector<std::string> *non_container_child_field_names,
    +    std::vector<OptimizerTreeBaseNodePtr> *non_container_child_fields,
    +    std::vector<std::string> *container_child_field_names,
    +    std::vector<std::vector<OptimizerTreeBaseNodePtr>> *container_child_fields) const {
    +  inline_field_names->push_back("function");
    +  inline_field_values->push_back(window_aggregate_.getName());
    +
    +  container_child_field_names->push_back("arguments");
    +  container_child_fields->emplace_back(CastSharedPtrVector<OptimizerTreeBase>(arguments_));
    +
    +  inline_field_names->push_back("window_name");
    +  inline_field_values->push_back(window_name_);
    +
    +  container_child_field_names->push_back("partition_by");
    +  container_child_fields->emplace_back(
    +      CastSharedPtrVector<OptimizerTreeBase>(window_info_.partition_by_attributes));
    +
    +  container_child_field_names->push_back("order_by");
    +  container_child_fields->emplace_back(
    +      CastSharedPtrVector<OptimizerTreeBase>(window_info_.order_by_attributes));
    +
    +  inline_field_names->push_back("is_ascending");
    +  std::string ascending_list("[");
    +  for (bool is_ascending : window_info_.order_by_directions) {
    +    if (is_ascending) {
    +      ascending_list.append("true,");
    +    } else {
    +      ascending_list.append("false,");
    +    }
    +  }
    +  if (!window_info_.order_by_directions.empty()) {
    +    ascending_list.pop_back();
    +  }
    +  ascending_list.append("]");
    +  inline_field_values->push_back(ascending_list);
    +
    +  inline_field_names->push_back("nulls_first");
    +  std::string nulls_first_flags("[");
    +  for (bool nulls_first_flag : window_info_.nulls_first) {
    --- End diff --
    
    Please add `const` for `nulls_first_flag`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68326145
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.hpp ---
    @@ -0,0 +1,246 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/ExpressionType.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +
    +class AggregateFunction;
    +class Type;
    +
    +namespace optimizer {
    +namespace expressions {
    +
    +/** \addtogroup OptimizerExpressions
    + *  @{
    + */
    +
    +struct WindowFrameInfo {
    +  /**
    +   * @brief Cosntructor.
    +   *
    +   * @param is_row_in True if this window frame is defined by ROWS, false if
    +   *                  defined by RANGE.
    +   * @param num_preceding_in The number of preceding tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   * @param num_following_in The number of following tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   **/
    +  explicit WindowFrameInfo(const bool is_row_in,
    +                           const int num_preceding_in,
    +                           const int num_following_in)
    +      : is_row(is_row_in),
    +        num_preceding(num_preceding_in),
    +        num_following(num_following_in) {}
    +
    +  const bool is_row;
    +  const int num_preceding;
    +  const int num_following;
    +};
    +
    +struct WindowInfo {
    +  /**
    +   * @brief Constructor.
    +   *
    +   * @param partition_by_attributes_in The partition keys for the window.
    +   * @param order_by_attributes_in The order keys for the window.
    +   * @param order_by_directions_in The order direction for order key.
    +   * @param nulls_first_in The nulls' position for order key.
    +   * @param frame_info_in The window frame information for the window. Null
    +   *        means there is no explicit window frame definition.
    +   **/
    +  explicit WindowInfo(const std::vector<AttributeReferencePtr> &partition_by_attributes_in,
    --- End diff --
    
    Ditto for `explicit`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68331825
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -1528,6 +1618,89 @@ L::LogicalPtr Resolver::RenameOutputColumns(
       return L::Project::Create(logical_plan, project_expressions);
     }
     
    +E::WindowInfo Resolver::resolveWindow(const ParseWindow &parse_window,
    +                                      const NameResolver &name_resolver) {
    +  std::vector<E::AttributeReferencePtr> partition_by_attributes;
    +  std::vector<E::AttributeReferencePtr> order_by_attributes;
    +  std::vector<bool> order_by_directions;
    +  std::vector<bool> nulls_first;
    +  E::WindowFrameInfo *frame_info = nullptr;
    +
    +  // Resolve PARTITION BY
    +  if (parse_window.partition_by_expressions() != nullptr) {
    +    for (const ParseExpression &unresolved_partition_by_expression :
    +         *parse_window.partition_by_expressions()) {
    +      ExpressionResolutionInfo expr_resolution_info(
    +          name_resolver,
    +          "PARTITION BY clause" /* clause_name */,
    +          nullptr /* select_list_info */);
    +      E::ScalarPtr partition_by_scalar = resolveExpression(
    +          unresolved_partition_by_expression,
    +          nullptr,  // No Type hint.
    +          &expr_resolution_info);
    +
    +      if (partition_by_scalar->isConstant()) {
    +        THROW_SQL_ERROR_AT(&unresolved_partition_by_expression)
    +            << "Constant expression not allowed in PARTITION BY";
    +      }
    +
    +      E::AttributeReferencePtr partition_by_attribute;
    +      if (!E::SomeAttributeReference::MatchesWithConditionalCast(partition_by_scalar,
    +                                                                 &partition_by_attribute)) {
    +        THROW_SQL_ERROR_AT(&unresolved_partition_by_expression)
    +            << "Only attribute name allowed in PARTITION BY in window definition";
    +      }
    +
    +      partition_by_attributes.push_back(partition_by_attribute);
    +    }
    +  }
    +
    +  // Resolve ORDER BY
    +  if (parse_window.order_by_expressions() != nullptr) {
    +    for (const ParseOrderByItem &order_by_item :
    +         *parse_window.order_by_expressions()) {
    +      ExpressionResolutionInfo expr_resolution_info(
    +          name_resolver,
    +          "ORDER BY clause" /* clause name */,
    +          nullptr /* select_list_info */);
    +      E::ScalarPtr order_by_scalar = resolveExpression(
    +          *order_by_item.ordering_expression(),
    +          nullptr,  // No Type hint.
    +          &expr_resolution_info);
    +
    +      if (order_by_scalar->isConstant()) {
    +        THROW_SQL_ERROR_AT(&order_by_item)
    +            << "Constant expression not allowed in ORDER BY";
    +      }
    +
    +      E::AttributeReferencePtr order_by_attribute;
    +      if (!E::SomeAttributeReference::MatchesWithConditionalCast(order_by_scalar,
    +                                                                 &order_by_attribute)) {
    +        THROW_SQL_ERROR_AT(&order_by_item)
    +            << "Only attribute name allowed in ORDER BY in window definition";
    +      }
    +
    +      order_by_attributes.push_back(order_by_attribute);
    +      order_by_directions.push_back(order_by_item.is_ascending());
    +      nulls_first.push_back(order_by_item.nulls_first());
    +    }
    +  }
    +
    +  // Resolve window frame
    +  if (parse_window.frame_info() != nullptr) {
    +    const quickstep::ParseFrameInfo *parse_frame_info = parse_window.frame_info();
    +    frame_info = new E::WindowFrameInfo(parse_frame_info->is_row,
    +                                     parse_frame_info->num_preceding,
    +                                     parse_frame_info->num_following);
    --- End diff --
    
    Please align with `(` in Line `1692`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68327735
  
    --- Diff: query_optimizer/logical/WindowAggregate.hpp ---
    @@ -0,0 +1,123 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/Alias.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/NamedExpression.hpp"
    +#include "query_optimizer/logical/Logical.hpp"
    +#include "query_optimizer/logical/LogicalType.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace logical {
    +
    +/** \addtogroup OptimizerLogical
    + *  @{
    + */
    +
    +class WindowAggregate;
    +typedef std::shared_ptr<const WindowAggregate> WindowAggregatePtr;
    +
    +/**
    + * @brief Window aggregate operator that computes window aggregate expressions.
    + */
    +class WindowAggregate : public Logical {
    + public:
    +  LogicalType getLogicalType() const override {
    +    return LogicalType::kWindowAggregate;
    +  }
    +
    +  std::string getName() const override { return "WindowAggregate"; }
    +
    +  /**
    +   * @return The input logical node.
    +   */
    +  const LogicalPtr& input() const { return input_; }
    +
    +  /**
    +   * @return PARTITION BY expressions.
    +   */
    +  const expressions::AliasPtr window_aggregate_expression() const {
    +    return window_aggregate_expression_;
    +  }
    +
    +  LogicalPtr copyWithNewChildren(
    +      const std::vector<LogicalPtr> &new_children) const override;
    +
    +  LogicalPtr copyWithNewInputExpressions(
    +      const std::vector<expressions::ExpressionPtr> &input_expressions) const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getOutputAttributes() const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @brief Creates an Aggregate logical node.
    +   *
    +   * @param input The input node.
    +   * @param window_aggregate_expression The window aggregate expression.
    +   * @return An immutable WindowAggregate node.
    +   */
    +  static WindowAggregatePtr Create(
    +      LogicalPtr input,
    +      const expressions::AliasPtr window_aggregate_expression) {
    +    return WindowAggregatePtr(new WindowAggregate(input, window_aggregate_expression));
    +  }
    +
    + protected:
    +  void getFieldStringItems(
    +      std::vector<std::string> *inline_field_names,
    +      std::vector<std::string> *inline_field_values,
    +      std::vector<std::string> *non_container_child_field_names,
    +      std::vector<OptimizerTreeBaseNodePtr> *non_container_child_fields,
    +      std::vector<std::string> *container_child_field_names,
    +      std::vector<std::vector<OptimizerTreeBaseNodePtr>> *container_child_fields) const override;
    +
    + private:
    +  WindowAggregate(LogicalPtr input,
    +                  const expressions::AliasPtr window_aggregate_expression)
    +      : input_(input),
    +        window_aggregate_expression_(window_aggregate_expression) {
    +    addChild(input_);
    +    addInputExpression(window_aggregate_expression_);
    +  }
    +
    +  LogicalPtr input_;
    +  const expressions::AliasPtr window_aggregate_expression_;
    +
    +  DISALLOW_COPY_AND_ASSIGN(WindowAggregate);
    +};
    +
    +/** @} */
    +
    +}  // namespace logical
    +}  // namespace optimizer
    +}  // namespace quickstep
    +
    +#endif /* QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_AGGREGATE_HPP_ */
    --- End diff --
    
    Please use the format `#endif  // QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_AGGREGATE_HPP_`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68329405
  
    --- Diff: query_optimizer/logical/WindowAggregate.hpp ---
    @@ -0,0 +1,123 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_LOGICAL_WINDOW_AGGREGATE_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/Alias.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/NamedExpression.hpp"
    +#include "query_optimizer/logical/Logical.hpp"
    +#include "query_optimizer/logical/LogicalType.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace logical {
    +
    +/** \addtogroup OptimizerLogical
    + *  @{
    + */
    +
    +class WindowAggregate;
    +typedef std::shared_ptr<const WindowAggregate> WindowAggregatePtr;
    +
    +/**
    + * @brief Window aggregate operator that computes window aggregate expressions.
    + */
    +class WindowAggregate : public Logical {
    + public:
    +  LogicalType getLogicalType() const override {
    +    return LogicalType::kWindowAggregate;
    +  }
    +
    +  std::string getName() const override { return "WindowAggregate"; }
    +
    +  /**
    +   * @return The input logical node.
    +   */
    +  const LogicalPtr& input() const { return input_; }
    +
    +  /**
    +   * @return PARTITION BY expressions.
    +   */
    +  const expressions::AliasPtr window_aggregate_expression() const {
    +    return window_aggregate_expression_;
    +  }
    +
    +  LogicalPtr copyWithNewChildren(
    +      const std::vector<LogicalPtr> &new_children) const override;
    +
    +  LogicalPtr copyWithNewInputExpressions(
    +      const std::vector<expressions::ExpressionPtr> &input_expressions) const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getOutputAttributes() const override;
    +
    +  std::vector<expressions::AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @brief Creates an Aggregate logical node.
    +   *
    +   * @param input The input node.
    +   * @param window_aggregate_expression The window aggregate expression.
    +   * @return An immutable WindowAggregate node.
    +   */
    +  static WindowAggregatePtr Create(
    +      LogicalPtr input,
    +      const expressions::AliasPtr window_aggregate_expression) {
    --- End diff --
    
    I think even for `share_ptr`s, we need to use `&`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68326675
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.hpp ---
    @@ -0,0 +1,246 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#ifndef QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +#define QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_
    +
    +#include <memory>
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/ExpressionType.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "utility/Macros.hpp"
    +
    +namespace quickstep {
    +
    +class AggregateFunction;
    +class Type;
    +
    +namespace optimizer {
    +namespace expressions {
    +
    +/** \addtogroup OptimizerExpressions
    + *  @{
    + */
    +
    +struct WindowFrameInfo {
    +  /**
    +   * @brief Cosntructor.
    +   *
    +   * @param is_row_in True if this window frame is defined by ROWS, false if
    +   *                  defined by RANGE.
    +   * @param num_preceding_in The number of preceding tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   * @param num_following_in The number of following tuples the window frame
    +   *                         will cover, -1 means UNBOUNDED.
    +   **/
    +  explicit WindowFrameInfo(const bool is_row_in,
    +                           const int num_preceding_in,
    +                           const int num_following_in)
    +      : is_row(is_row_in),
    +        num_preceding(num_preceding_in),
    +        num_following(num_following_in) {}
    +
    +  const bool is_row;
    +  const int num_preceding;
    +  const int num_following;
    +};
    +
    +struct WindowInfo {
    +  /**
    +   * @brief Constructor.
    +   *
    +   * @param partition_by_attributes_in The partition keys for the window.
    +   * @param order_by_attributes_in The order keys for the window.
    +   * @param order_by_directions_in The order direction for order key.
    +   * @param nulls_first_in The nulls' position for order key.
    +   * @param frame_info_in The window frame information for the window. Null
    +   *        means there is no explicit window frame definition.
    +   **/
    +  explicit WindowInfo(const std::vector<AttributeReferencePtr> &partition_by_attributes_in,
    +                      const std::vector<AttributeReferencePtr> &order_by_attributes_in,
    +                      const std::vector<bool> &order_by_directions_in,
    +                      const std::vector<bool> &nulls_first_in,
    +                      const WindowFrameInfo *frame_info_in)
    +      : partition_by_attributes(partition_by_attributes_in),
    +        order_by_attributes(order_by_attributes_in),
    +        order_by_directions(order_by_directions_in),
    +        nulls_first(nulls_first_in),
    +        frame_info(frame_info_in) {}
    +
    +  const std::vector<AttributeReferencePtr> partition_by_attributes;
    +  const std::vector<AttributeReferencePtr> order_by_attributes;
    +  const std::vector<bool> order_by_directions;
    +  const std::vector<bool> nulls_first;
    +  const WindowFrameInfo *frame_info;
    +};
    +
    +class WindowAggregateFunction;
    +typedef std::shared_ptr<const WindowAggregateFunction> WindowAggregateFunctionPtr;
    +
    +/**
    + * @brief Represents a window aggregate function and its arguments in the
    + *        optimizer. This class wraps some of the functionality from
    + *        quickstep::AggregateFunction and represents a particular instance
    + *        of an aggregate during query optimization.
    + **/
    +class WindowAggregateFunction : public Expression {
    + public:
    +  /**
    +   * @brief Destructor.
    +   */
    +  ~WindowAggregateFunction() override {}
    +
    +  ExpressionType getExpressionType() const override {
    +    return ExpressionType::kWindowAggregateFunction;
    +  }
    +
    +  std::string getName() const override {
    +    return "WindowAggregateFunction";
    +  }
    +
    +  const Type& getValueType() const override;
    +
    +  bool isConstant() const override {
    +    // Window aggregate function is never considered as a constant expression.
    +    return false;
    +  }
    +
    +  ExpressionPtr copyWithNewChildren(
    +      const std::vector<ExpressionPtr> &new_children) const override;
    +
    +  std::vector<AttributeReferencePtr> getReferencedAttributes() const override;
    +
    +  /**
    +   * @return Whether the type of the return value is nullable.
    +   **/
    +  bool isNullable() const;
    +
    +  /**
    +   * @return The WindowAggregateFunction singleton (from the expression system)
    +   *         for this node.
    +   **/
    +  inline const ::quickstep::AggregateFunction& window_aggregate() const {
    +    return window_aggregate_;
    +  }
    +
    +  /**
    +   * @return The list of scalar arguments to this aggregate.
    +   **/
    +  inline const std::vector<ScalarPtr>& arguments() const {
    +    return arguments_;
    +  }
    +
    +  /**
    +   * @return The window info of this window aggregate function.
    +   **/
    +  inline const WindowInfo window_info() const {
    +    return window_info_;
    +  }
    +
    +  /**
    +   * @return The name of the window.
    +   **/
    +  inline const std::string window_name() const {
    +    return window_name_;
    +  }
    +
    +  /**
    +   * @return Whether this is a DISTINCT aggregation.
    +   **/
    +  inline bool is_distinct() const {
    +    return is_distinct_;
    +  }
    +
    +  /**
    +   * @brief Create a new WindowAggregateFunction by directly defined window.
    +   *
    +   * @warning It is an error to call this with arguments that the given
    +   *          aggregate can not apply to.
    +   *
    +   * @param aggregate The underlying WindowAggregateFunction from the expression
    +   *        system.
    +   * @param arguments A list of arguments to the window aggregate function.
    +   * @param window_info The window info of the window aggregate function.
    +   * @param is_distinct Whether this is a DISTINCT aggregation.
    +   * @return A new AggregateFunctionPtr.
    +   **/
    +  static WindowAggregateFunctionPtr Create(const ::quickstep::AggregateFunction &window_aggregate,
    +                                           const std::vector<ScalarPtr> &arguments,
    +                                           const WindowInfo window_info,
    +                                           const std::string window_name,
    +                                           const bool is_distinct);
    +
    + protected:
    +  void getFieldStringItems(
    +      std::vector<std::string> *inline_field_names,
    +      std::vector<std::string> *inline_field_values,
    +      std::vector<std::string> *non_container_child_field_names,
    +      std::vector<OptimizerTreeBaseNodePtr> *non_container_child_fields,
    +      std::vector<std::string> *container_child_field_names,
    +      std::vector<std::vector<OptimizerTreeBaseNodePtr>> *container_child_fields) const override;
    +
    + private:
    +  /**
    +   * @brief Constructor.
    +   *
    +   * @param window_aggregate The actual AggregateFunction to use.
    +   * @param arguments A list of arguments to the window aggregate function.
    +   * @param window_info The window info of the window aggregate function.
    +   * @param is_distinct Indicates whether this is a DISTINCT aggregation.
    +   */
    +  WindowAggregateFunction(const ::quickstep::AggregateFunction &window_aggregate,
    +                          const std::vector<ScalarPtr> &arguments,
    +                          const WindowInfo window_info,
    +                          const std::string window_name,
    +                          const bool is_distinct)
    +      : window_aggregate_(window_aggregate),
    +        arguments_(arguments),
    +        window_info_(window_info),
    +        window_name_(window_name),
    +        is_distinct_(is_distinct) {
    +    for (const ScalarPtr &child : arguments_) {
    +      addChild(child);
    +    }
    +  }
    +
    +  // TODO(Shixuan): Currently this class uses AggregationFunction as
    +  // window_aggregate_. If it really needs to be seperated from the
    +  // AggregationFunction, a new class for WindowAggregationFunction should be
    +  // created as quickstep::WindowAggregateFunction.
    +  const ::quickstep::AggregateFunction &window_aggregate_;
    +  std::vector<ScalarPtr> arguments_;
    +  const WindowInfo window_info_;
    +  const std::string window_name_;
    +  const bool is_distinct_;
    +
    +  DISALLOW_COPY_AND_ASSIGN(WindowAggregateFunction);
    +};
    +
    +/** @} */
    +
    +}  // namespace expressions
    +}  // namespace optimizer
    +}  // namespace quickstep
    +
    +#endif /* QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_ */
    --- End diff --
    
    Use `#endif  // QUICKSTEP_QUERY_OPTIMIZER_EXPRESSIONS_WINDOW_AGGREGATE_FUNCTION_HPP_` instead.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68332715
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -2471,6 +2679,154 @@ E::ScalarPtr Resolver::resolveFunctionCall(
       return E::ToRef(aggregate_alias);
     }
     
    +E::ScalarPtr Resolver::resolveWindowAggregateFunction(
    +    const ParseFunctionCall &parse_function_call,
    +    ExpressionResolutionInfo *expression_resolution_info) {
    +  if (!expression_resolution_info->window_aggregation_info->window_aggregate_expressions.empty()) {
    +    THROW_SQL_ERROR_AT(&parse_function_call)
    +       << "Currently we only support single window aggregate in the query";
    +  }
    +
    +  std::string function_name = ToLower(parse_function_call.name()->value());
    +
    +  // First check for the special case COUNT(*).
    +  bool count_star = false;
    +  if (parse_function_call.star() != nullptr) {
    +    if (function_name != "count") {
    +      THROW_SQL_ERROR_AT(parse_function_call.star())
    +          << "Only COUNT can have star (*) as an argument";
    +    }
    +    count_star = true;
    +  }
    +
    +  std::vector<E::ScalarPtr> resolved_arguments;
    +  const PtrList<ParseExpression> *unresolved_arguments =
    +      parse_function_call.arguments();
    +  // The first aggregate function in the arguments.
    +  const ParseTreeNode *first_aggregate_function = nullptr;
    +  const ParseTreeNode *first_window_aggregate_function = nullptr;
    +  if (unresolved_arguments != nullptr) {
    +    for (const ParseExpression &unresolved_argument : *unresolved_arguments) {
    +      ExpressionResolutionInfo expr_resolution_info(
    +          *expression_resolution_info);
    +      resolved_arguments.push_back(
    +          resolveExpression(unresolved_argument,
    +                            nullptr,  // No Type hint.
    +                            &expr_resolution_info));
    +
    +      // We don't allow aggregate or window aggregate nested in a window
    +      // aggregate function.
    +      if (expr_resolution_info.hasAggregate() &&
    +          first_aggregate_function == nullptr) {
    +        first_aggregate_function =
    +            expr_resolution_info.parse_aggregate_expression;
    +      }
    +
    +      if (expr_resolution_info.hasWindowAggregate() &&
    +          first_window_aggregate_function == nullptr) {
    +        first_window_aggregate_function =
    +            expr_resolution_info.parse_window_aggregate_expression;
    +      }
    +    }
    +  }
    +
    +  if (count_star && !resolved_arguments.empty()) {
    +    THROW_SQL_ERROR_AT(&parse_function_call)
    +        << "COUNT aggregate has both star (*) and non-star arguments.";
    +  }
    +
    +  // Try to look up the AggregateFunction by name using
    +  // AggregateFunctionFactory.
    +  const ::quickstep::AggregateFunction *window_aggregate
    +      = AggregateFunctionFactory::GetByName(function_name);
    +  if (window_aggregate == nullptr) {
    +    THROW_SQL_ERROR_AT(&parse_function_call)
    +        << "Unrecognized function name \""
    +        << parse_function_call.name()->value()
    +        << "\"";
    +  }
    +
    +  // Make sure aggregates are allowed in this context.
    +  if (first_aggregate_function != nullptr) {
    +    THROW_SQL_ERROR_AT(first_aggregate_function)
    +        << "Window aggregation of aggregates is not allowed";
    +  }
    +
    +  // TODO(Shixuan): We currently don't support nested window aggregation since
    +  // TPC-DS doesn't have that. However, it is essentially a nested scalar
    +  // function, which should be supported in the future version of Quickstep.
    +  if (first_window_aggregate_function != nullptr) {
    +    THROW_SQL_ERROR_AT(first_window_aggregate_function)
    +        << "Window aggregation of window aggregates is not allowed";
    +  }
    +
    +  // Make sure a naked COUNT() with no arguments wasn't specified.
    +  if ((window_aggregate->getAggregationID() == AggregationID::kCount)
    +      && (resolved_arguments.empty())
    +      && (!count_star)) {
    +    THROW_SQL_ERROR_AT(&parse_function_call)
    +        << "COUNT aggregate requires an argument (either scalar or star (*))";
    +  }
    +
    +  // Resolve each of the Scalar arguments to the aggregate.
    +  std::vector<const Type*> argument_types;
    +  for (const E::ScalarPtr &argument : resolved_arguments) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  // Make sure that the aggregate can apply to the specified argument(s).
    +  if (!window_aggregate->canApplyToTypes(argument_types)) {
    +    THROW_SQL_ERROR_AT(&parse_function_call)
    +        << "Aggregate function " << window_aggregate->getName()
    +        << " can not apply to the given argument(s).";
    +  }
    +
    +  // A window aggregate function might be defined OVER a window name or a window.
    +  E::WindowAggregateFunctionPtr window_aggregate_function;
    +  if (parse_function_call.window_name() != nullptr) {
    +    std::unordered_map<std::string, WindowPlan> window_map
    +        = expression_resolution_info->window_aggregation_info->window_map;
    +    std::string window_name = parse_function_call.window_name()->value();
    +    std::unordered_map<std::string, WindowPlan>::const_iterator map_it
    +        = window_map.find(window_name);
    +
    +    if (map_it == window_map.end()) {
    +      THROW_SQL_ERROR_AT(parse_function_call.window_name())
    +        << "Undefined window " << window_name;
    --- End diff --
    
    Code style: please add two more whitespace indentations.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68327923
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -209,6 +227,26 @@ struct Resolver::QueryAggregationInfo {
       std::vector<E::AliasPtr> aggregate_expressions;
     };
     
    +struct Resolver::WindowPlan {
    +  explicit WindowPlan(E::WindowInfo window_info_in,
    --- End diff --
    
    No need for `explicit`, and we may want to add a reference to `window_info_in`, or use a rvalue instead.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68330476
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -973,8 +1011,36 @@ L::LogicalPtr Resolver::resolveSelect(
             logical_plan, resolvePredicate(parse_predicate, &expr_resolution_info));
       }
     
    +  // Resolve WINDOW clause.
    +  std::unordered_map<std::string, WindowPlan> sorted_window_map;
    +  if (select_query.window_list() != nullptr) {
    +    if (select_query.window_list()->size() > 1) {
    +      THROW_SQL_ERROR_AT(&(*select_query.window_list()->begin()))
    +        << "Currently we don't support multiple window aggregation functions";
    --- End diff --
    
    Code style: add two more whitespace indentations.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68333358
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -2471,6 +2679,154 @@ E::ScalarPtr Resolver::resolveFunctionCall(
       return E::ToRef(aggregate_alias);
     }
     
    +E::ScalarPtr Resolver::resolveWindowAggregateFunction(
    --- End diff --
    
    I see this new method shares too many common code with `Resolver::resolveFunctionCall`, so I would suggest to combine them.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68327042
  
    --- Diff: query_optimizer/logical/WindowAggregate.cpp ---
    @@ -0,0 +1,85 @@
    +/**
    + *   Copyright 2011-2015 Quickstep Technologies LLC.
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/logical/WindowAggregate.hpp"
    +
    +#include <string>
    +#include <vector>
    +
    +#include "query_optimizer/OptimizerTree.hpp"
    +#include "query_optimizer/expressions/Alias.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/ExpressionUtil.hpp"
    +#include "query_optimizer/expressions/NamedExpression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace logical {
    +
    +namespace E = ::quickstep::optimizer::expressions;
    +
    +LogicalPtr WindowAggregate::copyWithNewChildren(
    +    const std::vector<LogicalPtr> &new_children) const {
    +  DCHECK_EQ(getNumChildren(), new_children.size());
    +  return Create(new_children[0], window_aggregate_expression_);
    +}
    +
    +std::vector<E::AttributeReferencePtr> WindowAggregate::getOutputAttributes() const {
    +  std::vector<E::AttributeReferencePtr> output_attributes(input_->getOutputAttributes());
    +  output_attributes.push_back(E::ToRef(window_aggregate_expression_));
    +  return output_attributes;
    +}
    +
    +std::vector<E::AttributeReferencePtr> WindowAggregate::getReferencedAttributes() const {
    +  return window_aggregate_expression_->getReferencedAttributes();
    +}
    +
    +LogicalPtr WindowAggregate::copyWithNewInputExpressions(
    +    const std::vector<E::ExpressionPtr> &input_expressions) const {
    +  // Only one expression needed
    +  DCHECK_EQ(input_expressions.size(), static_cast<std::uint32_t>(1));
    --- End diff --
    
    Please refactor to `DCHECK_EQ(1u, input_expressions.size());`, where the left-hand side is the expected value, and the right-hand the checked expression.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68463508
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -1684,13 +1857,45 @@ L::LogicalPtr Resolver::resolveJoinedTableReference(
       THROW_SQL_ERROR_AT(&joined_table_reference) << "Full outer join is not supported yet";
     }
     
    +L::LogicalPtr Resolver::resolveSortInWindow(
    +    const L::LogicalPtr &logical_plan,
    +    const E::WindowInfo &window_info) {
    +  // Sort the table by (p_key, o_key)
    +  std::vector<E::AttributeReferencePtr> sort_attributes(window_info.partition_by_attributes);
    +  sort_attributes.insert(sort_attributes.end(),
    +                         window_info.order_by_attributes.begin(),
    +                         window_info.order_by_attributes.end());
    +
    +  std::vector<bool> sort_directions(
    +      window_info.partition_by_attributes.size(), true);
    +  sort_directions.insert(sort_directions.end(),
    +                         window_info.order_by_directions.begin(),
    +                         window_info.order_by_directions.end());
    +
    +  std::vector<bool> sort_nulls_first(
    +      window_info.partition_by_attributes.size(), false);
    +  sort_nulls_first.insert(sort_nulls_first.end(),
    +                          window_info.nulls_first.begin(),
    +                          window_info.nulls_first.end());
    +
    +  L::LogicalPtr sorted_logical_plan =
    +      L::Sort::Create(logical_plan,
    +                      sort_attributes,
    +                      sort_directions,
    +                      sort_nulls_first,
    --- End diff --
    
    We could add a new constructor w/ rvalues for the performance reasons.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68330778
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -992,6 +1059,29 @@ L::LogicalPtr Resolver::resolveSelect(
       SelectListInfo select_list_info(select_list_expressions,
                                       has_aggregate_per_expression);
     
    +  // Create window aggregate node if needed
    +  for (E::AliasPtr alias : window_aggregation_info.window_aggregate_expressions) {
    --- End diff --
    
    Please use `const E::AliasPtr &alias`.


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68333549
  
    --- Diff: query_optimizer/resolver/Resolver.cpp ---
    @@ -2794,6 +3150,10 @@ void Resolver::rewriteIfOrdinalReference(
       }
     }
     
    +std::string Resolver::GenerateWindowAggregateAttributeAlias(int index) {
    +  return std::string("$window_aggregate").append(std::to_string(index));
    --- End diff --
    
    I think this is overkill. How about the following refactor:
    
    `return "$window_aggregate" + std::to_string(index);`


---
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 #39: QUICKSTEP-20: Resolver support for Win...

Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:

    https://github.com/apache/incubator-quickstep/pull/39#discussion_r68325767
  
    --- Diff: query_optimizer/expressions/WindowAggregateFunction.cpp ---
    @@ -0,0 +1,193 @@
    +/**
    + *   Copyright 2015 Pivotal Software, Inc.
    + *   Copyright 2016, Quickstep Research Group, Computer Sciences Department,
    + *     University of Wisconsin\u2014Madison.
    + *
    + *   Licensed under the Apache License, Version 2.0 (the "License");
    + *   you may not use this file except in compliance with the License.
    + *   You may obtain a copy of the License at
    + *
    + *       http://www.apache.org/licenses/LICENSE-2.0
    + *
    + *   Unless required by applicable law or agreed to in writing, software
    + *   distributed under the License is distributed on an "AS IS" BASIS,
    + *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + *   See the License for the specific language governing permissions and
    + *   limitations under the License.
    + **/
    +
    +#include "query_optimizer/expressions/WindowAggregateFunction.hpp"
    +
    +#include <string>
    +#include <utility>
    +#include <vector>
    +
    +#include "expressions/aggregation/AggregateFunction.hpp"
    +#include "query_optimizer/expressions/AttributeReference.hpp"
    +#include "query_optimizer/expressions/Expression.hpp"
    +#include "query_optimizer/expressions/PatternMatcher.hpp"
    +#include "query_optimizer/expressions/Scalar.hpp"
    +#include "types/Type.hpp"
    +#include "utility/Cast.hpp"
    +
    +#include "glog/logging.h"
    +
    +namespace quickstep {
    +namespace optimizer {
    +namespace expressions {
    +
    +bool WindowAggregateFunction::isNullable() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return return_type->isNullable();
    +}
    +
    +const Type& WindowAggregateFunction::getValueType() const {
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments_) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +
    +  const Type *return_type = window_aggregate_.resultTypeForArgumentTypes(argument_types);
    +  DCHECK(return_type != nullptr);
    +  return *return_type;
    +}
    +
    +WindowAggregateFunctionPtr WindowAggregateFunction::Create(
    +    const ::quickstep::AggregateFunction &window_aggregate,
    +    const std::vector<ScalarPtr> &arguments,
    +    const WindowInfo window_info,
    +    const std::string window_name,
    +    const bool is_distinct) {
    +#ifdef QUICKSTEP_DEBUG
    +  std::vector<const Type*> argument_types;
    +  for (const ScalarPtr &argument : arguments) {
    +    argument_types.emplace_back(&argument->getValueType());
    +  }
    +  DCHECK(window_aggregate.canApplyToTypes(argument_types));
    +#endif  // QUICKSTEP_DEBUG
    +
    +  return WindowAggregateFunctionPtr(
    +      new WindowAggregateFunction(window_aggregate, arguments, window_info, window_name, is_distinct));
    +}
    +
    +ExpressionPtr WindowAggregateFunction::copyWithNewChildren(
    +    const std::vector<ExpressionPtr> &new_children) const {
    +  std::vector<ScalarPtr> new_arguments;
    +  for (const ExpressionPtr &expression_ptr : new_children) {
    +    ScalarPtr expr_as_scalar;
    +    CHECK(SomeScalar::MatchesWithConditionalCast(expression_ptr, &expr_as_scalar))
    +        << expression_ptr->toString();
    +    new_arguments.emplace_back(std::move(expr_as_scalar));
    +  }
    +
    +  return WindowAggregateFunctionPtr(
    +      new WindowAggregateFunction(window_aggregate_,
    +                                  new_arguments,
    +                                  window_info_,
    +                                  window_name_,
    +                                  is_distinct_));
    +}
    +
    +std::vector<AttributeReferencePtr> WindowAggregateFunction::getReferencedAttributes() const {
    +  std::vector<AttributeReferencePtr> referenced_attributes;
    +  for (const ScalarPtr &argument : arguments_) {
    +    const std::vector<AttributeReferencePtr> referenced_attributes_in_argument =
    +        argument->getReferencedAttributes();
    +    referenced_attributes.insert(referenced_attributes.end(),
    +                                 referenced_attributes_in_argument.begin(),
    +                                 referenced_attributes_in_argument.end());
    +  }
    +
    +  referenced_attributes.insert(referenced_attributes.end(),
    +                               window_info_.partition_by_attributes.begin(),
    +                               window_info_.partition_by_attributes.end());
    +
    +  referenced_attributes.insert(referenced_attributes.end(),
    +                               window_info_.order_by_attributes.begin(),
    +                               window_info_.order_by_attributes.end());
    +
    +  return referenced_attributes;
    +}
    +
    +void WindowAggregateFunction::getFieldStringItems(
    +    std::vector<std::string> *inline_field_names,
    +    std::vector<std::string> *inline_field_values,
    +    std::vector<std::string> *non_container_child_field_names,
    +    std::vector<OptimizerTreeBaseNodePtr> *non_container_child_fields,
    +    std::vector<std::string> *container_child_field_names,
    +    std::vector<std::vector<OptimizerTreeBaseNodePtr>> *container_child_fields) const {
    +  inline_field_names->push_back("function");
    +  inline_field_values->push_back(window_aggregate_.getName());
    +
    +  container_child_field_names->push_back("arguments");
    +  container_child_fields->emplace_back(CastSharedPtrVector<OptimizerTreeBase>(arguments_));
    +
    +  inline_field_names->push_back("window_name");
    +  inline_field_values->push_back(window_name_);
    +
    +  container_child_field_names->push_back("partition_by");
    +  container_child_fields->emplace_back(
    +      CastSharedPtrVector<OptimizerTreeBase>(window_info_.partition_by_attributes));
    +
    +  container_child_field_names->push_back("order_by");
    +  container_child_fields->emplace_back(
    +      CastSharedPtrVector<OptimizerTreeBase>(window_info_.order_by_attributes));
    +
    +  inline_field_names->push_back("is_ascending");
    +  std::string ascending_list("[");
    +  for (bool is_ascending : window_info_.order_by_directions) {
    --- End diff --
    
    Please add `const` for `is_ascending`.


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