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/15 20:05:11 UTC
[GitHub] incubator-quickstep pull request #35: Added Parser support for SQL Window Ag...
GitHub user shixuan-fan opened a pull request:
https://github.com/apache/incubator-quickstep/pull/35
Added Parser support for SQL Window Aggregation Function
Added parser support for SQL Window Aggregation Function.
The parser will now understand the window aggregation function, the grammar will be like
SELECT avg(attr1) OVER
(partition by attr2,
order by attr3 desc nulls last,
rows between 3 preceding and 3 following)
FROM table1
Also, we could give the window a name to make it look much cleaner:
SELECT avg(attr1) OVER w1, sum(attr2) OVER w2 FROM table1
WINDOW w1 AS ( ... window ... definition ... )
WINDOW w2 AS ( ... window ... definition ... )
Currently, when the resolver see a function with a window aggregation function, it will report error saying that it is currently 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/35.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 #35
----
commit 3bcdd91e7edbd01d5a431c2baa3c6c336c83f63b
Author: shixuan <sh...@wisc.edu>
Date: 2016-06-14T23:07:32Z
QUICKSTEP-20: Added parser support for SQL window aggregation function
commit cf0412e8fa911623349d61665499ef2ffc43197d
Author: shixuan <sh...@wisc.edu>
Date: 2016-06-15T18:01:09Z
QUICKSTEP-20: added parser support for window aggregation functions
----
---
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 #35: QUICKSTEP-20: Added Parser support for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/35
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67446034
--- Diff: parser/ParseWindow.hpp ---
@@ -0,0 +1,204 @@
+/**
+ * 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_PARSER_PARSE_WINDOW_HPP_
+#define QUICKSTEP_PARSER_PARSE_WINDOW_HPP_
+
+#include <string>
+#include <vector>
+
+#include "parser/ParseExpression.hpp"
+#include "parser/ParseLiteralValue.hpp"
+#include "parser/ParseOrderBy.hpp"
+#include "parser/ParseTreeNode.hpp"
+#include "utility/PtrList.hpp"
+
+#include "glog/logging.h"
+
+namespace quickstep {
+
+/**
+ * @brief The information of the how the framing in the window is defined
+ **/
+struct ParseFrameInfo : ParseTreeNode {
+ /**
+ * @brief Constructor.
+ * @param row True if the frame mode is ROW, false if it is RANGE.
+ * @param num_pre The number of rows/value of range that is preceding
+ * the current row in the frame.
+ * @param num_follow The number of rows/value of range that is following
+ * the current row in the frame.
+ **/
+ ParseFrameInfo(const int line_number,
+ const int column_number,
+ bool row,
+ long num_pre,
+ long num_follow)
--- End diff --
Question: will the three above arguments change later?
---
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 #35: QUICKSTEP-20: Added Parser support for SQL Wi...
Posted by jianqiao <gi...@git.apache.org>.
Github user jianqiao commented on the issue:
https://github.com/apache/incubator-quickstep/pull/35
LGTM. Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and 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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67460045
--- Diff: query_optimizer/resolver/Resolver.cpp ---
@@ -2366,6 +2367,12 @@ E::ScalarPtr Resolver::resolveFunctionCall(
ExpressionResolutionInfo *expression_resolution_info) {
std::string function_name = ToLower(parse_function_call.name()->value());
+ // TODO(Shixuan): Add support for window aggregation function
--- End diff --
Code style: Please add a period in the end of the comment.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67445231
--- Diff: parser/CMakeLists.txt ---
@@ -114,6 +114,7 @@ add_library(quickstep_parser_ParseSubqueryTableReference ParseSubqueryTableRefer
add_library(quickstep_parser_ParseTableReference ParseTableReference.cpp ParseTableReference.hpp)
add_library(quickstep_parser_ParseTreeNode ../empty_src.cpp ParseTreeNode.hpp)
add_library(quickstep_parser_ParserUtil ParserUtil.cpp ParserUtil.hpp)
+add_library(quickstep_parser_ParseWindow ../empty_src.cpp ParseWindow.hpp)
--- End diff --
Please sort in the alphabet order, i.e., putting between `quickstep_parser_ParseTreeNode` and `quickstep_parser_ParserUtil`.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67445310
--- Diff: parser/CMakeLists.txt ---
@@ -338,6 +347,7 @@ target_link_libraries(quickstep_parser_SqlParser
quickstep_parser_ParseSubqueryTableReference
quickstep_parser_ParseTableReference
quickstep_parser_ParserUtil
+ quickstep_parser_ParseWindow
--- End diff --
Please sort in the alphabet order.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459445
--- Diff: parser/ParseBasicExpressions.hpp ---
@@ -429,6 +430,46 @@ class ParseFunctionCall : public ParseExpression {
return star_.get();
}
+ /**
+ * @return The window name.
+ **/
+ const ParseString* window_name() const {
+ return window_name_.get();
+ }
+
+ /**
+ * @return The window.
+ **/
+ const ParseWindow* window() const {
+ return window_.get();
+ }
+
+ /**
+ * @brief Check if this function is a window aggregation function
+ *
+ * @return True if this function is a window aggregation function; False
+ * otherwise.
+ **/
+ bool isWindow() const {
+ return window_name_ != nullptr || window_ != nullptr;
+ }
+
+ /**
+ * @brief Set the window name.
+ * @param window_name The window name.
+ **/
+ void setWindowName(ParseString *window_name) {
+ window_name_.reset(std::move(window_name));
+ }
+
+ /**
+ * @brief Set the window.
+ * @param window The window.
+ **/
+ void setWindow(ParseWindow *window) {
+ window_.reset(std::move(window));
--- End diff --
Ditto for `move`.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67518021
--- Diff: parser/ParseWindow.hpp ---
@@ -0,0 +1,204 @@
+/**
+ * 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_PARSER_PARSE_WINDOW_HPP_
+#define QUICKSTEP_PARSER_PARSE_WINDOW_HPP_
+
+#include <string>
+#include <vector>
+
+#include "parser/ParseExpression.hpp"
+#include "parser/ParseLiteralValue.hpp"
+#include "parser/ParseOrderBy.hpp"
+#include "parser/ParseTreeNode.hpp"
+#include "utility/PtrList.hpp"
+
+#include "glog/logging.h"
+
+namespace quickstep {
+
+/**
+ * @brief The information of the how the framing in the window is defined
+ **/
+struct ParseFrameInfo : ParseTreeNode {
+ /**
+ * @brief Constructor.
+ * @param row True if the frame mode is ROW, false if it is RANGE.
+ * @param num_pre The number of rows/value of range that is preceding
+ * the current row in the frame.
+ * @param num_follow The number of rows/value of range that is following
+ * the current row in the frame.
+ **/
+ ParseFrameInfo(const int line_number,
+ const int column_number,
+ bool row,
+ long num_pre,
+ long num_follow)
--- End diff --
Will add "const". 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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459704
--- Diff: parser/SqlLexer.lpp ---
@@ -212,6 +215,7 @@ unsigned_numeric_literal {exact_numeric_literal}|{approximate_numeric_literal}
"for" return TOKEN_FOR;
"foreign" return TOKEN_FOREIGN;
"from" return TOKEN_FROM;
+ "following" return TOKEN_FOLLOWING;
--- End diff --
Please move `following` before `for`.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459470
--- Diff: parser/ParseSelect.hpp ---
@@ -195,6 +205,14 @@ class ParseSelect : public ParseTreeNode {
non_container_child_field_names->push_back("limit");
non_container_child_fields->push_back(limit_.get());
}
+
+ if (window_list_ != nullptr) {
+ container_child_field_names->push_back("window_list");
+ container_child_fields->emplace_back();
+ for (const ParseWindow& window : *window_list_) {
--- End diff --
Code style:
`const ParseWindow &window`.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67520588
--- Diff: parser/SqlParser.ypp ---
@@ -1332,6 +1369,83 @@ opt_limit_clause:
}
}
+opt_window_clause:
+ {
+ $$ = nullptr;
+ }
+ | window_declaration_list
+
+window_declaration_list:
+ window_declaration {
+ $$ = new quickstep::PtrList<quickstep::ParseWindow>();
+ $$->push_back($1);
+ }
+ | window_declaration_list window_declaration {
+ $$ = $1;
+ $$->push_back($2);
+ }
+
+window_declaration:
--- End diff --
@zuyu Since window_declaration is a part of window_declaration_list,I put it this way in order to maintain the hierarchy. If alphabet order has higher precedence then I will change them. 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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459515
--- Diff: parser/ParseWindow.hpp ---
@@ -0,0 +1,204 @@
+/**
+ * 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_PARSER_PARSE_WINDOW_HPP_
+#define QUICKSTEP_PARSER_PARSE_WINDOW_HPP_
+
+#include <string>
+#include <vector>
+
+#include "parser/ParseExpression.hpp"
+#include "parser/ParseLiteralValue.hpp"
+#include "parser/ParseOrderBy.hpp"
+#include "parser/ParseTreeNode.hpp"
+#include "utility/PtrList.hpp"
+
+#include "glog/logging.h"
+
+namespace quickstep {
+
+/**
+ * @brief The information of the how the framing in the window is defined
+ **/
+struct ParseFrameInfo : ParseTreeNode {
+ /**
+ * @brief Constructor.
+ * @param row True if the frame mode is ROW, false if it is RANGE.
+ * @param num_pre The number of rows/value of range that is preceding
+ * the current row in the frame.
+ * @param num_follow The number of rows/value of range that is following
+ * the current row in the frame.
+ **/
+ ParseFrameInfo(const int line_number,
+ const int column_number,
+ bool row,
+ long num_pre,
+ long num_follow)
+ : ParseTreeNode(line_number, column_number),
+ is_row(row),
+ num_preceding(num_pre),
+ num_following(num_follow) {
+ }
+
+ std::string getName() const override { return "FrameInfo"; }
+
+ bool is_row;
+ long num_preceding;
+ long num_following;
+
+ 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<const ParseTreeNode *> *non_container_child_fields,
+ std::vector<std::string> *container_child_field_names,
+ std::vector<std::vector<const ParseTreeNode *>> *container_child_fields)
+ const override {
+ inline_field_names->push_back("frame_mode");
+ inline_field_values->push_back(is_row ? "row" : "range");
+
+ inline_field_names->push_back("num_preceding");
+ inline_field_values->push_back(std::to_string(num_preceding));
+
+ inline_field_names->push_back("num_following");
+ inline_field_values->push_back(std::to_string(num_following));
+ }
+};
+
+/**
+ * @brief The parsed representation of a WINDOW definition.
+ **/
+class ParseWindow : public ParseTreeNode {
+ public:
+ /**
+ * @brief Constructor.
+ * @param line_number The line number of the first token of this WINDOW clause
+ * in the SQL statement.
+ * @param column_number The column number of the first token of this WINDOW
+ * clause in the SQL statement.
+ * @param partition_by_expressions Optional grouping expressions that might be
+ * specified in the SQL statement. Similar to
+ * GROUP BY with regular aggregates.
+ * @param order_by_expressions Optional ordering expressions that might be
+ * specified in the SQL statement.
+ * @param frame_info The information about framing.
+ **/
+ ParseWindow(const int line_number,
+ const int column_number,
+ PtrList<ParseExpression> *partition_by_expressions,
+ PtrList<ParseOrderByItem> *order_by_expressions,
+ ParseFrameInfo *frame_info)
+ : ParseTreeNode(line_number, column_number),
+ partition_by_expressions_(partition_by_expressions),
+ order_by_expressions_(order_by_expressions),
+ frame_info_(frame_info) {
+ }
+
+ /**
+ * @brief Destructor.
+ **/
+ ~ParseWindow() override {}
+
+ std::string getName() const override {
+ return "window";
+ }
+
+ /**
+ * @brief Grouping expressions.
+ **/
+ const PtrList<ParseExpression> *partition_by_expressions() const {
+ return partition_by_expressions_.get();
+ }
+
+ /**
+ * @brief Ordering expressions.
+ **/
+ const PtrList<ParseOrderByItem> *order_by_expressions() const {
--- End diff --
Code style:
`const PtrList<,,,>* order_by_expressions() const`.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459941
--- Diff: parser/preprocessed/SqlParser_gen.hpp ---
@@ -1,8 +1,8 @@
-/* A Bison parser, made by GNU Bison 3.0.4. */
+/* A Bison parser, made by GNU Bison 3.0.2. */
--- End diff --
Please use a modern `bison` to re-generate these files.
---
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 #35: QUICKSTEP-20: Added Parser support for SQL Wi...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/35
@shixuan-fan I've done one pass. Please also fix the style issues that caused the build failures.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459437
--- Diff: parser/ParseBasicExpressions.hpp ---
@@ -429,6 +430,46 @@ class ParseFunctionCall : public ParseExpression {
return star_.get();
}
+ /**
+ * @return The window name.
+ **/
+ const ParseString* window_name() const {
+ return window_name_.get();
+ }
+
+ /**
+ * @return The window.
+ **/
+ const ParseWindow* window() const {
+ return window_.get();
+ }
+
+ /**
+ * @brief Check if this function is a window aggregation function
+ *
+ * @return True if this function is a window aggregation function; False
+ * otherwise.
+ **/
+ bool isWindow() const {
+ return window_name_ != nullptr || window_ != nullptr;
+ }
+
+ /**
+ * @brief Set the window name.
+ * @param window_name The window name.
+ **/
+ void setWindowName(ParseString *window_name) {
+ window_name_.reset(std::move(window_name));
--- End diff --
I don't think we need `move` here.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459841
--- Diff: parser/SqlParser.ypp ---
@@ -1332,6 +1369,83 @@ opt_limit_clause:
}
}
+opt_window_clause:
+ {
+ $$ = nullptr;
+ }
+ | window_declaration_list
+
+window_declaration_list:
+ window_declaration {
+ $$ = new quickstep::PtrList<quickstep::ParseWindow>();
+ $$->push_back($1);
+ }
+ | window_declaration_list window_declaration {
+ $$ = $1;
+ $$->push_back($2);
+ }
+
+window_declaration:
--- End diff --
Please move before `window_declaration_list`.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67460001
--- Diff: parser/tests/Select.test ---
@@ -1709,3 +1709,121 @@ SelectStatement
| +-TableReference[table=bar]
+-from_clause=
+-TableReference[table=test]
+==
+
+select avg(attr1) over w from test
+window w as
+(partition by attr2, attr3
+ order by attr4
+ rows between 3 preceding and 3 following)
--- End diff --
Please rewrite the query with capitalized `SQL` keywords.
---
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 #35: QUICKSTEP-20: Added Parser support for SQL Wi...
Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/35
@zuyu Thanks for all your effort in this! Will settle these issues now :)
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67459543
--- Diff: parser/ParseWindow.hpp ---
@@ -0,0 +1,204 @@
+/**
+ * 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_PARSER_PARSE_WINDOW_HPP_
+#define QUICKSTEP_PARSER_PARSE_WINDOW_HPP_
+
+#include <string>
+#include <vector>
+
+#include "parser/ParseExpression.hpp"
+#include "parser/ParseLiteralValue.hpp"
+#include "parser/ParseOrderBy.hpp"
+#include "parser/ParseTreeNode.hpp"
+#include "utility/PtrList.hpp"
+
+#include "glog/logging.h"
+
+namespace quickstep {
+
+/**
+ * @brief The information of the how the framing in the window is defined
+ **/
+struct ParseFrameInfo : ParseTreeNode {
+ /**
+ * @brief Constructor.
+ * @param row True if the frame mode is ROW, false if it is RANGE.
+ * @param num_pre The number of rows/value of range that is preceding
+ * the current row in the frame.
+ * @param num_follow The number of rows/value of range that is following
+ * the current row in the frame.
+ **/
+ ParseFrameInfo(const int line_number,
+ const int column_number,
+ bool row,
+ long num_pre,
+ long num_follow)
+ : ParseTreeNode(line_number, column_number),
+ is_row(row),
+ num_preceding(num_pre),
+ num_following(num_follow) {
+ }
+
+ std::string getName() const override { return "FrameInfo"; }
+
+ bool is_row;
+ long num_preceding;
+ long num_following;
+
+ 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<const ParseTreeNode *> *non_container_child_fields,
+ std::vector<std::string> *container_child_field_names,
+ std::vector<std::vector<const ParseTreeNode *>> *container_child_fields)
+ const override {
+ inline_field_names->push_back("frame_mode");
+ inline_field_values->push_back(is_row ? "row" : "range");
+
+ inline_field_names->push_back("num_preceding");
+ inline_field_values->push_back(std::to_string(num_preceding));
+
+ inline_field_names->push_back("num_following");
+ inline_field_values->push_back(std::to_string(num_following));
+ }
+};
+
+/**
+ * @brief The parsed representation of a WINDOW definition.
+ **/
+class ParseWindow : public ParseTreeNode {
+ public:
+ /**
+ * @brief Constructor.
+ * @param line_number The line number of the first token of this WINDOW clause
+ * in the SQL statement.
+ * @param column_number The column number of the first token of this WINDOW
+ * clause in the SQL statement.
+ * @param partition_by_expressions Optional grouping expressions that might be
+ * specified in the SQL statement. Similar to
+ * GROUP BY with regular aggregates.
+ * @param order_by_expressions Optional ordering expressions that might be
+ * specified in the SQL statement.
+ * @param frame_info The information about framing.
+ **/
+ ParseWindow(const int line_number,
+ const int column_number,
+ PtrList<ParseExpression> *partition_by_expressions,
+ PtrList<ParseOrderByItem> *order_by_expressions,
+ ParseFrameInfo *frame_info)
+ : ParseTreeNode(line_number, column_number),
+ partition_by_expressions_(partition_by_expressions),
+ order_by_expressions_(order_by_expressions),
+ frame_info_(frame_info) {
+ }
+
+ /**
+ * @brief Destructor.
+ **/
+ ~ParseWindow() override {}
+
+ std::string getName() const override {
+ return "window";
+ }
+
+ /**
+ * @brief Grouping expressions.
+ **/
+ const PtrList<ParseExpression> *partition_by_expressions() const {
+ return partition_by_expressions_.get();
+ }
+
+ /**
+ * @brief Ordering expressions.
+ **/
+ const PtrList<ParseOrderByItem> *order_by_expressions() const {
+ return order_by_expressions_.get();
+ }
+
+ /**
+ * @brief Frame information.
+ **/
+ const ParseFrameInfo* frame_info() const {
+ return frame_info_.get();
+ }
+
+ /**
+ * @return The window name.
+ */
+ const ParseString* name() const {
+ return name_.get();
+ }
+
+ /**
+ * @brief Set the alias of the window.
+ * @param name The alias of the window.
+ **/
+ void setName(ParseString *name) {
+ name_.reset(std::move(name));
--- End diff --
Please remove `move`.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67550182
--- Diff: parser/SqlParser.ypp ---
@@ -1332,6 +1369,83 @@ opt_limit_clause:
}
}
+opt_window_clause:
+ {
+ $$ = nullptr;
+ }
+ | window_declaration_list
+
+window_declaration_list:
+ window_declaration {
+ $$ = new quickstep::PtrList<quickstep::ParseWindow>();
+ $$->push_back($1);
+ }
+ | window_declaration_list window_declaration {
+ $$ = $1;
+ $$->push_back($2);
+ }
+
+window_declaration:
--- End diff --
Do I need to reorder all the expressions in alphabetic order? It seems that the file is not sorted that way :(
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67558193
--- Diff: parser/SqlParser.ypp ---
@@ -1332,6 +1369,83 @@ opt_limit_clause:
}
}
+opt_window_clause:
+ {
+ $$ = nullptr;
+ }
+ | window_declaration_list
+
+window_declaration_list:
+ window_declaration {
+ $$ = new quickstep::PtrList<quickstep::ParseWindow>();
+ $$->push_back($1);
+ }
+ | window_declaration_list window_declaration {
+ $$ = $1;
+ $$->push_back($2);
+ }
+
+window_declaration:
--- End diff --
First, we should sort them whenever possible.
In addition, in thiw PR we should commit code sorted in alphabetic order.
Finally, there are some committed but unsorted code, so we should fix in another separate PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #35: QUICKSTEP-20: Added Parser support for...
Posted by jianqiao <gi...@git.apache.org>.
Github user jianqiao commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/35#discussion_r67582107
--- Diff: parser/SqlParser.ypp ---
@@ -1332,6 +1369,83 @@ opt_limit_clause:
}
}
+opt_window_clause:
+ {
+ $$ = nullptr;
+ }
+ | window_declaration_list
+
+window_declaration_list:
+ window_declaration {
+ $$ = new quickstep::PtrList<quickstep::ParseWindow>();
+ $$->push_back($1);
+ }
+ | window_declaration_list window_declaration {
+ $$ = $1;
+ $$->push_back($2);
+ }
+
+window_declaration:
--- End diff --
I think currently `SqlParser.hpp` is mostly organized in this way:
```
A:
TOKEN_X SubA1 SubA2
...
;
SubA1:
TOKEN_Y SubSubA1 SubSubA2
...
;
SubSubA1:
...
;
...
SubA2:
...
;
```
I.e. kind of infix-order traversal of the grammar tree.
Let's just have this PR in this structure. We can have a standalone PR if there is a proposal for better organization of the grammar rules.
---
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 #35: QUICKSTEP-20: Added Parser support for...
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/35#discussion_r67537621
--- Diff: parser/SqlParser.ypp ---
@@ -1332,6 +1369,83 @@ opt_limit_clause:
}
}
+opt_window_clause:
+ {
+ $$ = nullptr;
+ }
+ | window_declaration_list
+
+window_declaration_list:
+ window_declaration {
+ $$ = new quickstep::PtrList<quickstep::ParseWindow>();
+ $$->push_back($1);
+ }
+ | window_declaration_list window_declaration {
+ $$ = $1;
+ $$->push_back($2);
+ }
+
+window_declaration:
--- End diff --
If the alphabet order does not affect the correctness, we should use it. 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.
---