Hi jian, Thanks for the concrete examples. Replies inline.
> The RPR window runs inside the subquery and produces rows; the outer > query then filters and sorts them, which is ordinary subquery > behavior that has nothing to do with RPR. Whether the rows come from > a subquery, a CTE, a JOIN, or a plain table is irrelevant to RPR, so > the above test adds no coverage beyond what the plain-table cases > already give. This is where I would like to explain how I think about these tests. They are black-box tests. They do not ask whether the RPR code and the subquery code are related; they ask whether the answer is right when the two meet. The Subquery and CTE section of rpr_base.sql is the mirror image of the tests where RPR sits on top of a subquery: here the window is inside the subquery or CTE and the outer query consumes its rows. The two directions may build different plans. And several of the defects this patch fixed turned up around the planner. For example, subquery pull-up could replace a navigation argument by a constant, so PREV() no longer meant the target row. Cases like that were not visible from inside RPR. I should also be honest about where I stand. I have worked with many other database systems, but my experience with the PostgreSQL planner is thin. So for everything outside RPR itself, the interplay with the planner and the deparser, I lean heavily on writing tests first (TDD), and I ask for your understanding on that. It is like walking over snow and probing ahead with a pole, because what lies under the snow may be a cliff edge or a crevasse rather than ground. The tests reach a little past the boundary of RPR on purpose, so that nothing slips through. I lay the test results out as a matrix of cases against variants, showing what works and what does not, and use it to find what needs fixing and to decide which way to fix it. So the tests are the most important clues I have. A request to cut the number of tests feels to me like being asked to put down the probe I depend on before I move ahead. > Moving tests from one file to another does not solve the problem. We > should first try harder to remove unnecessary test queries. > Consolidating all the error cases into one place would also be a > good idea. Moving tests only moves the size around, but I do not agree that removing has to come first. To avoid missing defects, I draw the scope of verification a little wide at the boundary of RPR, and I weigh the risk of missing a defect by misjudging that boundary slightly more than the size of the tests. On gathering the error cases: the top-level classification of these files is by test category (the parser layer and the planner layer are listed at the top of rpr_base.sql), and the error cases are already categories of their own there, Error Cases Tests and Error Limit Tests. The other error cases stay in the category they test, such as quantifiers or navigation functions, next to the valid cases they contrast with. Across the RPR test files, about 40% of the ERROR outputs are in those dedicated sections and the rest are inside the categories. Some of the same errors are tested in more than one of these places, so I went through the overlap. By code coverage, many of them are redundant: removing the 70 candidates one by one loses no line or branch coverage. As black-box tests, though, they differ in the input they give (operator, quantifier form, clause, arity, boundary value), and only one of the 70 was the same input under a different table name. A single duplicate test does not seem worth a change by itself. > In src/test/regress/sql/rpr_base.sql, I saw comments like > ``` > -- Complex Multi-Level Nesting > -- Pattern: (((A B) | C)+ D)+ > ``` > I don't think the above comments are really any helpful. The pattern > is the same as the SELECT query below. If the test query changes, > the above comments will become stale. It also occupied an > unnecessary blank line. As for the comments, I am preparing a local branch in which the comments and the documentation are translated into my native language. After I post the next patch series, I will go through the comments one by one with that branch while I review the whole patch, and this kind of comment will be looked at then. Best regards, Henson
