-
Notifications
You must be signed in to change notification settings - Fork 1.9k
IGNITE-28925 SQL Calcite: Fix unexpected downstream().end() call for execution nodes #13407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
731b84c
58bbdbd
366b426
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,11 @@ public static <Row> CollectNode<Row> createCountCollector(ExecutionContext<Row> | |
|
|
||
| if (waiting == 0) | ||
| source().request(waiting = IN_BUFFER_SIZE); | ||
| else if (waiting < 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It look like delayed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we must keep invariant: "end downstream only if any rows is requested". If we request initially 1 row, push that row on "end()", we can't futher end downstream, since no rows more requested and it can lead to assertions. This case is shown in the new CollectIntegrationTest |
||
| requested = 0; | ||
|
|
||
| downstream().end(); | ||
| } | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
|
|
@@ -138,6 +143,7 @@ public static <Row> CollectNode<Row> createCountCollector(ExecutionContext<Row> | |
| @Override public void end() throws Exception { | ||
| assert downstream() != null; | ||
| assert waiting > 0; | ||
| assert requested > 0; | ||
|
|
||
| checkState(); | ||
|
|
||
|
|
@@ -146,10 +152,13 @@ public static <Row> CollectNode<Row> createCountCollector(ExecutionContext<Row> | |
| if (isClosed()) | ||
| return; | ||
|
|
||
| requested--; | ||
|
|
||
| downstream().push(collector.get()); | ||
|
|
||
| if (requested > 0) { | ||
| requested = 0; | ||
|
|
||
| downstream().push(collector.get()); | ||
| downstream().end(); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -282,6 +282,8 @@ private void onRequest() throws Exception { | |
| break; | ||
|
|
||
| case END: | ||
| requested = 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, new tests only cover CollectNode and SortAggregateNode changes. Changes to CNLJ and SortNode are made to be consistent with other code. We have a lot of nodes with: Sequential calls. These 4 nodes are only nodes with different logic. Change in SortNode it's not a bugfix, it's just code consistency, but CNLJ without |
||
|
|
||
| downstream().end(); | ||
| break; | ||
|
|
||
|
|
@@ -338,8 +340,11 @@ private void onEndLeft() throws Exception { | |
|
|
||
| state = State.END; | ||
|
|
||
| if (requested > 0) | ||
| if (requested > 0) { | ||
| requested = 0; | ||
|
|
||
| downstream().end(); | ||
| } | ||
| } | ||
| else { | ||
| prepareCorrelations(); | ||
|
|
@@ -469,8 +474,9 @@ private void join() throws Exception { | |
|
|
||
| state = State.END; | ||
|
|
||
| if (requested > 0) | ||
| downstream().end(); | ||
| requested = 0; | ||
|
|
||
| downstream().end(); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,8 +101,11 @@ public SortAggregateNode( | |
|
|
||
| source().request(IN_BUFFER_SIZE); | ||
| } | ||
| else if (waiting < 0) | ||
| else if (waiting < 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. Looks like a delayed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://github.com/apache/ignite/pull/13407/changes#r3674976662 comment. It's common behaviour for all nodes. |
||
| requested = 0; | ||
|
|
||
| downstream().end(); | ||
| } | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
|
|
@@ -163,8 +166,11 @@ else if (waiting < 0) | |
| doPush(); | ||
| } | ||
|
|
||
| if (requested > 0) | ||
| if (requested > 0) { | ||
| requested = 0; | ||
|
|
||
| downstream().end(); | ||
| } | ||
|
|
||
| grp = null; | ||
| prevRow = null; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,10 +205,11 @@ private void flush() throws Exception { | |
| } | ||
|
|
||
| if (reversed == null ? rows.isEmpty() : reversed.isEmpty()) { | ||
| if (requested > 0) | ||
| downstream().end(); | ||
| if (requested > 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new tests
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is only for code consistency. No bug here. |
||
| requested = 0; | ||
|
|
||
| requested = 0; | ||
| downstream().end(); | ||
| } | ||
| } | ||
| } | ||
| finally { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You 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. | ||
| */ | ||
|
|
||
| package org.apache.ignite.internal.processors.query.calcite.integration; | ||
|
|
||
| import org.apache.ignite.internal.processors.query.calcite.exec.rel.AbstractNode; | ||
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * Integration test for collect node. | ||
| */ | ||
| public class CollectIntegrationTest extends AbstractBasicIntegrationTest { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One dedicated with-join test only for row in-out and buffer processing. Based on a plan which can variate. Maybe we need a special node execution test, WDYT?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this integration test shows that problem is not artifitial |
||
| /** | ||
| * Tests that collect node correctly handles the case when downstream requests | ||
| * limited number of rows, where collect must push one row and then | ||
| * properly terminate downstream. | ||
| */ | ||
| @Test | ||
| public void testRequestLimitedRowsCountFromCollect() { | ||
| sql("CREATE TABLE t(a INT)"); | ||
|
|
||
| sql("INSERT INTO t (a) VALUES (?)", 0); | ||
|
|
||
| String sql = "SELECT /*+ CNL_JOIN */ ARRAY(SELECT a FROM t) FROM t LIMIT 1"; | ||
|
|
||
| assertQuery(sql).resultSize(1).check(); | ||
|
|
||
| /** | ||
| * The data source size of (buffer size + 1) is used to ensure that multiple batches are needed | ||
| * on right hand of CNLJ to process all input rows, in this case left hand is not requested | ||
| * immediately after endLeft() call. | ||
| */ | ||
| for (int i = 1; i < AbstractNode.IN_BUFFER_SIZE + 1; i++) | ||
| sql("INSERT INTO t (a) VALUES (?)", i); | ||
|
|
||
| assertQuery(sql).resultSize(1).check(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| import org.apache.ignite.cache.QueryIndexType; | ||
| import org.apache.ignite.cache.affinity.rendezvous.RendezvousAffinityFunction; | ||
| import org.apache.ignite.configuration.IgniteConfiguration; | ||
| import org.apache.ignite.internal.processors.query.calcite.exec.rel.AbstractNode; | ||
| import org.apache.ignite.internal.util.typedef.F; | ||
| import org.junit.Test; | ||
|
|
||
|
|
@@ -155,6 +156,31 @@ public void testNullsReordering() { | |
| .check(); | ||
| } | ||
|
|
||
| /** | ||
| * Tests that sort aggregate node correctly handles the case when input data | ||
| * ends exactly when the requested number of rows is satisfied. | ||
| */ | ||
| @Test | ||
| public void testRequestRowsAfterInputEnds() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It actually works witout fixes in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For |
||
| /** | ||
| * With input rows count equals to the buffer size, the last row completes both | ||
| * the input data and the requested count in the same cycle. This triggers | ||
| * a synchronous request() call from within push() to fill the buffer, and | ||
| * the node must properly handle the termination on the subsequent request() | ||
| * call rather than on end(). | ||
| */ | ||
| int bufSize = AbstractNode.IN_BUFFER_SIZE; | ||
|
|
||
| sql("CREATE TABLE t0(a INTEGER PRIMARY KEY, b INTEGER) WITH template=replicated," + atomicity()); | ||
|
|
||
| for (int i = 0; i < bufSize; i++) | ||
| sql("INSERT INTO t0 VALUES (?, ?)", i, i); | ||
|
|
||
| assertQuery("SELECT t1.a FROM t0 AS t1 JOIN (SELECT a, count(a) FROM t0 GROUP BY a) AS t2 ON t1.a = t2.a") | ||
| .resultSize(bufSize) | ||
| .check(); | ||
| } | ||
|
|
||
| /** | ||
| * @param c Cache. | ||
| * @param rows Rows count. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brw. Why di we reques more than even
rowsCnt(in case of fewrowsCnt)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This node always produce one row. But can collect it from unlimited data set, so it's worth to request from input more rows than requested from downstream.