Re: Row pattern recognition

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