On 7/26/23 14:21, Tatsuo Ishii wrote:
Attached is the v3 patch. In this patch following changes are made.
Excellent. Thanks!
A few quick comments:
- PERMUTE is still misspelled as PREMUTE
- PATTERN variables do not have to exist in the DEFINE clause. They are
considered TRUE if not present.
(1) I completely changed the pattern matching engine so that it
performs backtracking. Now the engine evaluates all pattern elements
defined in PATTER against each row, saving matched pattern variables
in a string per row. For example if the pattern element A and B
evaluated to true, a string "AB" is created for current row.
This continues until all pattern matching fails or encounters the end
of full window frame/partition. After that, the pattern matching
engine creates all possible "pattern strings" and apply the regular
expression matching to each. For example if we have row 0 = "AB" row 1
= "C", possible pattern strings are: "AC" and "BC".
If it matches, the length of matching substring is saved. After all
possible trials are done, the longest matching substring is chosen and
it becomes the width (number of rows) in the reduced window frame.
See row_is_in_reduced_frame, search_str_set and search_str_set_recurse
in nodeWindowAggs.c for more details. For now I use a naive depth
first search and probably there is a lot of rooms for optimization
(for example rewriting it without using
recursion). Suggestions/patches are welcome.
My own reviews will only focus on correctness for now. Once we get a
good set of regression tests all passing, I will focus more on
optimization. Of course, others might want to review the performance now.
Vik Fearing wrote:
I strongly disagree with this. Window function do not need to know
how the frame is defined, and indeed they should not.
WinGetFuncArgInFrame should answer yes or no and the window function
just works on that. Otherwise we will get extension (and possibly even
core) functions that don't handle the frame properly.
So I moved row_is_in_reduce_frame into WinGetFuncArgInFrame so that
those window functions are not needed to be changed.
(3) Window function rpr was removed. We can use first_value instead.
Excellent.
(4) Remaining tasks/issues.
- For now I disable WinSetMarkPosition because RPR often needs to
access a row before the mark is set. We need to fix this in the
future.
Noted, and agreed.
- I am working on making window aggregates RPR aware now. The
implementation is in progress and far from completeness. An example
is below. I think row 2, 3, 4 of "count" column should be NULL
instead of 3, 2, 0, 0. Same thing can be said to other
rows. Probably this is an effect of moving aggregate but I still
studying the window aggregation code.
This tells me again that RPR is not being run in the right place. All
windowed aggregates and frame-level window functions should Just Work
with no modification.
SELECT company, tdate, first_value(price) OVER W, count(*) OVER w FROM stock
WINDOW w AS (
PARTITION BY company
ORDER BY tdate
ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
AFTER MATCH SKIP PAST LAST ROW
INITIAL
PATTERN (START UP+ DOWN+)
DEFINE
START AS TRUE,
UP AS price > PREV(price),
DOWN AS price < PREV(price)
);
company | tdate | first_value | count
----------+------------+-------------+-------
company1 | 2023-07-01 | 100 | 4
company1 | 2023-07-02 | | 3
company1 | 2023-07-03 | | 2
company1 | 2023-07-04 | | 0
company1 | 2023-07-05 | | 0
company1 | 2023-07-06 | 90 | 4
company1 | 2023-07-07 | | 3
company1 | 2023-07-08 | | 2
company1 | 2023-07-09 | | 0
company1 | 2023-07-10 | | 0
company2 | 2023-07-01 | 50 | 4
company2 | 2023-07-02 | | 3
company2 | 2023-07-03 | | 2
company2 | 2023-07-04 | | 0
company2 | 2023-07-05 | | 0
company2 | 2023-07-06 | 60 | 4
company2 | 2023-07-07 | | 3
company2 | 2023-07-08 | | 2
company2 | 2023-07-09 | | 0
company2 | 2023-07-10 | | 0
In this scenario, row 1's frame is the first 5 rows and specified SKIP
PAST LAST ROW, so rows 2-5 don't have *any* frame (because they are
skipped) and the result of the outer count should be 0 for all of them
because there are no rows in the frame.
When we get to adding count in the MEASURES clause, there will be a
difference between no match and empty match, but that does not apply here.
I am going to add this thread to CommitFest and plan to add both of
you as reviewers. Thanks in advance.
My pleasure. Thank you for working on this difficult feature.
--
Vik Fearing
