IGNITE-28925 SQL Calcite: Fix unexpected downstream().end() call for execution nodes - #13407
IGNITE-28925 SQL Calcite: Fix unexpected downstream().end() call for execution nodes#13407alex-plekhanov wants to merge 3 commits into
Conversation
94db3bf to
6e9a9bb
Compare
6e9a9bb to
731b84c
Compare
| * ends exactly when the requested number of rows is satisfied. | ||
| */ | ||
| @Test | ||
| public void testRequestRowsAfterInputEnds() { |
There was a problem hiding this comment.
It actually works witout fixes in the ALL and RANDOM tx. modes. Maybe we need assumeThat()
There was a problem hiding this comment.
For ALL and RANDOM plan is not the same as for NONE, for this reason plan checkers (matches method) are disabled for ALL and RANDOM. But result still should be valid for these tx modes even with another plan, so I think test is also helpful for these modes
| /** | ||
| * Integration test for collect node. | ||
| */ | ||
| public class CollectIntegrationTest extends AbstractBasicIntegrationTest { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
But this integration test shows that problem is not artifitial
| @@ -114,6 +114,11 @@ public static <Row> CollectNode<Row> createCountCollector(ExecutionContext<Row> | |||
|
|
|||
| if (waiting == 0) | |||
| source().request(waiting = IN_BUFFER_SIZE); | |||
There was a problem hiding this comment.
Brw. Why di we reques more than even rowsCnt (in case of few rowsCnt)?
There was a problem hiding this comment.
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.
|
|
||
| if (waiting == 0) | ||
| source().request(waiting = IN_BUFFER_SIZE); | ||
| else if (waiting < 0) { |
There was a problem hiding this comment.
It look like delayed downstream().end(). Why not in own void end()?
There was a problem hiding this comment.
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
|
|
||
| checkState(); | ||
|
|
||
| requested = rowsCnt; |
There was a problem hiding this comment.
If we do conditional requested = 0;, probably we should not assignt it here
There was a problem hiding this comment.
We need filled requested field before doPush
| if (reversed == null ? rows.isEmpty() : reversed.isEmpty()) { | ||
| if (requested > 0) | ||
| downstream().end(); | ||
| if (requested > 0) { |
There was a problem hiding this comment.
The new tests testRequestRowsAfterInputEnds() and CollectIntegrationTest seem to work witout this change. Why? Maybe we should add/fix some executions tests too.
There was a problem hiding this comment.
This change is only for code consistency. No bug here.
| source().request(IN_BUFFER_SIZE); | ||
| } | ||
| else if (waiting < 0) | ||
| else if (waiting < 0) { |
There was a problem hiding this comment.
Same. Looks like a delayed end(). Why not in own end(). It it some CLN node behaviour trait?
There was a problem hiding this comment.
See https://github.com/apache/ignite/pull/13407/changes#r3674976662 comment. It's common behaviour for all nodes.
| break; | ||
|
|
||
| case END: | ||
| requested = 0; |
There was a problem hiding this comment.
New testRequestRowsAfterInputEnds() test seems to work without any changes here. Is it an optimization? Do we need extra execution tests or at least comments?
There was a problem hiding this comment.
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:
requested = 0;
downstream().end();
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 requested = 0 can lead to the same problems as CollectNode and SortAggregateNode. I think it's possible to write reproducer for CNLJ, but maybe 2 tests is enoght to prove that requested = 0 should always be placed before downstream().end()?
# Conflicts: # modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java
2 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.