You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "somandal (via GitHub)" <gi...@apache.org> on 2023/04/18 17:30:38 UTC
[GitHub] [pinot] somandal opened a new pull request, #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
somandal opened a new pull request, #10635:
URL: https://github.com/apache/pinot/pull/10635
This PR fixes an issue seen with certain types of window function queries using an empty `OVER()` clause. If the window function itself doesn't take any input columns (such as `COUNT(*)` or `ROW_NUMBER()`), the Apache Calcite rule `CoreRules.PROJECT_WINDOW_TRANSPOSE` creates an empty `LogicalProject` below the `LogicalWindow` and removes the `LogicalProject` above the window as it identifies the upper project as a trivial one.
The following types of queries fail because of the above Apache Calcite bug:
- `SELECT COUNT(*) OVER() from tableName`
- `SELECT 42, COUNT(*) OVER() from tableName`
- `SELECT ROW_NUMBER() OVER() from tableName` [the [PR](https://github.com/apache/pinot/pull/10587) to add support for this is still under review]
The fix this PR adds does the following:
- Within the `PinotWindowExchangeNodeInsertRule` we detect the empty `LogicalProject` below `LogicalWindow` for empty OVER() type of queries.
- If found:
- Add a LogicalProject with a literal below the LogicalWindow
- Modify the LogicalWindow's inputs to include the literal
- Add the LogicalExchange (as before)
- Add a LogicalProject above the LogicalWindow to remove the literal
- Else just create the exchange below LogicalWindow like we always did
Discussed the above solution with @ankitsultana and @walterddr
cc @siddharthteotia @vvivekiyer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174694875
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
+ "\n LogicalWindow(window#0=[window(aggs [COUNT()])])",
+ "\n LogicalExchange(distribution=[hash])",
+ "\n LogicalProject(winLiteral=[0])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "single empty OVER() literal, count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT 42, COUNT(*) OVER() FROM a",
Review Comment:
This works in PostgreSQL (shown for ROW_NUMBER but also works for COUNT(*)):
```
dvdrental=# select amount, ROW_NUMBER() OVER() from payment;
amount | row_number
--------+------------
7.99 | 1
1.99 | 2
7.99 | 3
2.99 | 4
7.99 | 5
5.99 | 6
5.99 | 7
5.99 | 8
2.99 | 9
4.99 | 10
6.99 | 11
0.99 | 12
0.99 | 13
6.99 | 14
8.99 | 15
0.99 | 16
3.99 | 17
4.99 | 18
0.99 | 19
0.99 | 20
4.99 | 21
```
It should also work today for the support added so far in Pinot because Calcite will include the `columnName` column in the project list correctly. Example plans:
```
Query: SELECT a.col1, COUNT(*) OVER() FROM a
Explain plan:
Execution Plan
LogicalWindow(window#0=[window(aggs [COUNT()])])
LogicalExchange(distribution=[hash])
LogicalProject(col1=[$0])
LogicalTableScan(table=[[a]])
Query: SELECT COUNT(a.col1) OVER() FROM a
Explain plan:
Execution Plan
LogicalProject($0=[$1])
LogicalWindow(window#0=[window(aggs [COUNT($0)])])
LogicalExchange(distribution=[hash])
LogicalProject(col1=[$0])
LogicalTableScan(table=[[a]])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175492784
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
Review Comment:
I meant approach (a) in my comment. But probably fine for now. This isn't incorrect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia merged pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia merged PR #10635:
URL: https://github.com/apache/pinot/pull/10635
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1170766601
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,55 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
Review Comment:
got it, added a TODO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #10635:
URL: https://github.com/apache/pinot/pull/10635#issuecomment-1520435852
@somandal - can you rebase please ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175270368
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
+ "\n LogicalWindow(window#0=[window(aggs [COUNT()])])",
+ "\n LogicalExchange(distribution=[hash])",
+ "\n LogicalProject(winLiteral=[0])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "single empty OVER() literal, count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT 42, COUNT(*) OVER() FROM a",
Review Comment:
For example, if we do this query in ROW_NUMBER()
SELECT colName, ROW_NUMBER() OVER() FROM FOO
It is probably easy to reason about the output since we get column values with corresponding number assigned.
Now in COUNT(*), it will be like column values and the overall count of the rows besides each column value which probably does not look like a meaningful result to reason about.
SELECT colName, COUNT(*) OVER() FROM FOO
But that is just an opinion. Syntactically may be it is fine!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174691213
##########
pinot-query-runtime/src/test/resources/queries/WindowFunctions.json:
##########
@@ -51,6 +51,50 @@
[768]
]
},
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT COUNT(*) OVER() FROM {tbl}",
+ "outputs": [
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16]
+ ]
+ },
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT 42, COUNT(*) OVER() FROM {tbl}",
Review Comment:
Same question here
##########
pinot-query-runtime/src/test/resources/queries/WindowFunctions.json:
##########
@@ -51,6 +51,50 @@
[768]
]
},
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT COUNT(*) OVER() FROM {tbl}",
+ "outputs": [
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16]
+ ]
+ },
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT 42, COUNT(*) OVER() FROM {tbl}",
Review Comment:
Same question here. MySQL throws error for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175293292
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
Review Comment:
This LogicalProject looks weird. I mean the `w0$o0=[$1]` part doesn't look familiar to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175388766
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
Review Comment:
Calcite creates such names for each of the window function columns generated as part of the query to ensure column uniqueness (from what I could understand). Do you think we need to rename these?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #10635:
URL: https://github.com/apache/pinot/pull/10635#issuecomment-1520491742
> @somandal - can you rebase please ?
@siddharthteotia I've rebased, can you take a look when you get a chance? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175266113
##########
pinot-query-runtime/src/test/resources/queries/WindowFunctions.json:
##########
@@ -51,6 +51,50 @@
[768]
]
},
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT COUNT(*) OVER() FROM {tbl}",
+ "outputs": [
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16]
+ ]
+ },
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT 42, COUNT(*) OVER() FROM {tbl}",
Review Comment:
Yep that's right. Let's get that document ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175526221
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
Review Comment:
Sounds good. We discussed exploring the option to allow empty DataSchema and empty rows: Object[] = { {}, {}, {} ... } and that might be the cleaner longer term solution for this since this way no actual data records will be sent on the wire.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175537604
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
+ Project projectBelowWindow = LogicalProject.create(project.getInput(), project.getHints(),
+ expsForProjectBelowWindow, expsFieldNamesBelowWindow);
+
+ // Fix up the inputs to the Window to include the literal column and add an exchange
+ final RelDataTypeFactory.Builder outputBuilder = cluster.getTypeFactory().builder();
+ outputBuilder.addAll(projectBelowWindow.getRowType().getFieldList());
+ outputBuilder.addAll(window.getRowType().getFieldList());
+
+ // This scenario is only possible for empty OVER(), add a LogicalExchange with empty hash distribution list
Review Comment:
done, please take a look. This function has some java docs that explain in detail where this modification is necessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1170745139
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,55 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
Review Comment:
@ankitsultana can you elaborate a bit on what kind of queries you expect will need column-less tuples?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1170550097
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +196,58 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = new ArrayList<>();
+ final RelDataTypeFactory.Builder builder = cluster.getTypeFactory().builder();
Review Comment:
good point, fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175537206
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
Review Comment:
fixed to generate better names ($0, $1, etc)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174691817
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
+ "\n LogicalWindow(window#0=[window(aggs [COUNT()])])",
+ "\n LogicalExchange(distribution=[hash])",
+ "\n LogicalProject(winLiteral=[0])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "single empty OVER() literal, count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT 42, COUNT(*) OVER() FROM a",
Review Comment:
Is the problem being solved in this PR not applicable to following kind of query ?
```
SELECT columnName, count(*) OVER()
FROM FOO
```
This shouldn't even be a valid query imo (MySQL it doesn't work). Is that true for Postgres too ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174690790
##########
pinot-query-runtime/src/test/resources/queries/WindowFunctions.json:
##########
@@ -51,6 +51,50 @@
[768]
]
},
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT COUNT(*) OVER() FROM {tbl}",
Review Comment:
I am guessing this is in alignment with Postgres supports because MySQL does not support this kind of query -- `count(*) with empty over()` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1170710127
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,55 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
Review Comment:
These "constants" are used for custom frames (e.g. `ROWS BETWEEN 5 PRECEDING AND CURRENT ROW`, the 5 will be added to the constants). I haven't seen it contain anything for the non-custom frame window function support we've added so far. We'll need to deal with these constants when we start work on sliding window support (custom frames)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174697449
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
+ Project projectBelowWindow = LogicalProject.create(project.getInput(), project.getHints(),
+ expsForProjectBelowWindow, expsFieldNamesBelowWindow);
+
+ // Fix up the inputs to the Window to include the literal column and add an exchange
+ final RelDataTypeFactory.Builder outputBuilder = cluster.getTypeFactory().builder();
+ outputBuilder.addAll(projectBelowWindow.getRowType().getFieldList());
+ outputBuilder.addAll(window.getRowType().getFieldList());
+
+ // This scenario is only possible for empty OVER(), add a LogicalExchange with empty hash distribution list
Review Comment:
> This scenario is only possible for empty OVER()
You may want to clarify this a bit -- scenario happens for empty OVER() for window functions that don't take arguments ?
Related question - What happens if the scenario is empty OVER() with window functions that don't take arguments along with one or more columns in the SELECT list ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175265477
##########
pinot-query-runtime/src/test/resources/queries/WindowFunctions.json:
##########
@@ -51,6 +51,50 @@
[768]
]
},
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT COUNT(*) OVER() FROM {tbl}",
Review Comment:
Got it. Thanks for clarifying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175457046
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
Review Comment:
@siddharthteotia what do you mean by random `Identifier` here exactly? We need to essentially project one value per row in the input. We discussed two ways to do this:
a) Project one of the columns
b) Project a literal (which will essentially project a random value for each row)
We don't care about the actual values of the rows and even something like a list of empty objects would do (but we decided to explore this later). Projecting a column gets into the business of deciding which one, and some types of columns (say string) may result in more data transfer. Let me know if you'd like more clarification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1170422336
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +196,58 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = new ArrayList<>();
+ final RelDataTypeFactory.Builder builder = cluster.getTypeFactory().builder();
Review Comment:
Creating the RelDataType can be avoided and this call can be simplified by using LogicalProject.create(...). Is there a reason for using "new LogicalProject" and building it ourselves here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174691817
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
+ "\n LogicalWindow(window#0=[window(aggs [COUNT()])])",
+ "\n LogicalExchange(distribution=[hash])",
+ "\n LogicalProject(winLiteral=[0])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "single empty OVER() literal, count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT 42, COUNT(*) OVER() FROM a",
Review Comment:
Will this problem not happen for queries
```
SELECT columnName, count(*) OVER()
FROM FOO
```
This shouldn't even be a valid query imo (MySQL it doesn't work). Is that true for Postgres too ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174694860
##########
pinot-query-runtime/src/test/resources/queries/WindowFunctions.json:
##########
@@ -51,6 +51,50 @@
[768]
]
},
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT COUNT(*) OVER() FROM {tbl}",
Review Comment:
yes this query works in PostgreSQL:
```
dvdrental=# select ROW_NUMBER() OVER() from payment;
row_number
------------
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
dvdrental=# select COUNT(*) OVER() from payment;
count
-------
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
14596
```
##########
pinot-query-runtime/src/test/resources/queries/WindowFunctions.json:
##########
@@ -51,6 +51,50 @@
[768]
]
},
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT COUNT(*) OVER() FROM {tbl}",
+ "outputs": [
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16],
+ [16]
+ ]
+ },
+ {
+ "description": "Single empty OVER() count(*) without select col",
+ "sql": "SELECT 42, COUNT(*) OVER() FROM {tbl}",
Review Comment:
Yes this works in PostgreSQL:
```
dvdrental=# select 42, COUNT(*) OVER() from payment;
?column? | count
----------+-------
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
42 | 14596
```
There are many types of custom window frames that works in PostgreSQL but isn't allowed by Apache Calcite. I'll work on a document to list out the ones I know about. I assume that if we want to move towards PostgreSQL compliance, we want to allow these kinds of queries, right?
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
+ "\n LogicalWindow(window#0=[window(aggs [COUNT()])])",
+ "\n LogicalExchange(distribution=[hash])",
+ "\n LogicalProject(winLiteral=[0])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "single empty OVER() literal, count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT 42, COUNT(*) OVER() FROM a",
Review Comment:
This works in PostgreSQL:
```
dvdrental=# select amount, ROW_NUMBER() OVER() from payment;
amount | row_number
--------+------------
7.99 | 1
1.99 | 2
7.99 | 3
2.99 | 4
7.99 | 5
5.99 | 6
5.99 | 7
5.99 | 8
2.99 | 9
4.99 | 10
6.99 | 11
0.99 | 12
0.99 | 13
6.99 | 14
8.99 | 15
0.99 | 16
3.99 | 17
4.99 | 18
0.99 | 19
0.99 | 20
4.99 | 21
```
It should also work today for the support added so far in Pinot because Calcite will include the `columnName` column in the project list correctly. Example plans:
```
Query: SELECT a.col1, COUNT(*) OVER() FROM a
Explain plan:
Execution Plan
LogicalWindow(window#0=[window(aggs [COUNT()])])
LogicalExchange(distribution=[hash])
LogicalProject(col1=[$0])
LogicalTableScan(table=[[a]])
Query: SELECT COUNT(a.col1) OVER() FROM a
Explain plan:
Execution Plan
LogicalProject($0=[$1])
LogicalWindow(window#0=[window(aggs [COUNT($0)])])
LogicalExchange(distribution=[hash])
LogicalProject(col1=[$0])
LogicalTableScan(table=[[a]])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175492784
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
Review Comment:
I meant approach (a) in my comment (Identifier meaning a column). But probably fine for now. This isn't incorrect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175282591
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
+ Project projectBelowWindow = LogicalProject.create(project.getInput(), project.getHints(),
+ expsForProjectBelowWindow, expsFieldNamesBelowWindow);
+
+ // Fix up the inputs to the Window to include the literal column and add an exchange
+ final RelDataTypeFactory.Builder outputBuilder = cluster.getTypeFactory().builder();
+ outputBuilder.addAll(projectBelowWindow.getRowType().getFieldList());
+ outputBuilder.addAll(window.getRowType().getFieldList());
+
+ // This scenario is only possible for empty OVER(), add a LogicalExchange with empty hash distribution list
Review Comment:
The related question part you already answered in a different thread
https://github.com/apache/pinot/pull/10635/files#r1174691817
Might just want to clarify the comment a bit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1175306947
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,59 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ *
+ * TODO: Explore an option to handle empty LogicalProject by actually projecting empty rows for each entry. This way
+ * there will no longer be a need to add a literal to the empty LogicalProject, but just traverse the number of
+ * rows
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
+ final List<String> expsFieldNamesBelowWindow = Collections.singletonList("winLiteral");
Review Comment:
Probably a dumb question - why did we settle on the approach of using hardcoded `Literal` (which I guess takes the value 0 ?) to fix this ?
Can we not pick a random `Identifier` since the `LogicalProject` above the `LogicalWindow` is anyway going to remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1174695446
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -14,6 +14,33 @@
"\n"
]
},
+ {
+ "description": "single empty OVER() count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT COUNT(*) OVER() FROM a",
+ "output": [
+ "Execution Plan",
+ "\nLogicalProject(w0$o0=[$1])",
+ "\n LogicalWindow(window#0=[window(aggs [COUNT()])])",
+ "\n LogicalExchange(distribution=[hash])",
+ "\n LogicalProject(winLiteral=[0])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "single empty OVER() literal, count(*) only",
+ "sql": "EXPLAIN PLAN FOR SELECT 42, COUNT(*) OVER() FROM a",
Review Comment:
can you elaborate on why that shouldn't be a valid query?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1170659192
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,55 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
+ * Only empty OVER() type queries can result in a situation where the LogicalProject below the LogicalWindow is an
+ * empty LogicalProject (i.e. no columns are projected). This can only occur for window functions that don't take
+ * an input column. Some example queries where this can occur are:
+ *
+ * SELECT COUNT(*) OVER() from tableName
+ * SELECT 42, COUNT(*) OVER() from tableName
+ * SELECT ROW_NUMBER() OVER() from tableName
+ *
+ * This function modifies the empty LogicalProject below the LogicalWindow to add a literal and adds a LogicalProject
+ * above LogicalWindow to remove the additional literal column from being projected any further. This also handles
+ * the addition of the LogicalExchange under the LogicalWindow.
+ */
+ private RelNode handleEmptyProjectBelowWindow(Window window, Project project) {
+ RelOptCluster cluster = window.getCluster();
+ RexBuilder rexBuilder = cluster.getRexBuilder();
+
+ // Construct the project that goes below the window (which projects a literal)
+ final List<RexNode> expsForProjectBelowWindow = Collections.singletonList(
+ rexBuilder.makeLiteral(0, cluster.getTypeFactory().createSqlType(SqlTypeName.INTEGER)));
Review Comment:
Windows also have a "constant" concept which seem to be "additional inputs". Do you know where they are supposed to be used?
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,55 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
Review Comment:
Can you leave a TODO for support for column-less tuples?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10635:
URL: https://github.com/apache/pinot/pull/10635#issuecomment-1513614841
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10635?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10635](https://codecov.io/gh/apache/pinot/pull/10635?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a7a7f6a) into [master](https://codecov.io/gh/apache/pinot/commit/44fe6940e19cbf29d2f5f606124c2f6b77e85360?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (44fe694) will **increase** coverage by `4.29%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #10635 +/- ##
============================================
+ Coverage 27.79% 32.08% +4.29%
- Complexity 58 453 +395
============================================
Files 2090 2106 +16
Lines 112648 113080 +432
Branches 16988 17043 +55
============================================
+ Hits 31307 36283 +4976
+ Misses 78232 73509 -4723
- Partials 3109 3288 +179
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.46% <ø> (-0.13%)` | :arrow_down: |
| integration2 | `?` | |
| unittests2 | `13.88% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 500 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10635/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10635: [multistage] Modify empty LogicalProject for window functions to have a literal
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10635:
URL: https://github.com/apache/pinot/pull/10635#discussion_r1170759238
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -171,4 +195,55 @@ private boolean isPartitionByOnlyQuery(Window.Group windowGroup) {
}
return isPartitionByOnly;
}
+
+ /**
Review Comment:
Oh I was alluding to our discussion on Slack. Later on we might wanna support column less records in the intermediate stages in which case we may not need to customize the window/project transpose rule and we may be able to support empty project operators.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org