Re: Row pattern recognition

Hi Jian,

Thank you for the v54-0001 refactoring.

On 2026-10-01 09:30 UTC, jian he wrote:
> define_walker is kind of ugly, so i did the attached refactoring,
> which make navigation expression check stay within ParseRPRNavCall.
> Overall I feel it is more neat than define_walker.
> It's based on v53.

I cannot take it in right now. The code I am working on changes the
same parts of parse_rpr.c and parse_func.c, and the patch does not
apply on top of it.

I did read the diff, including the expected output changes, and
looked at the error positions (the caret). Some get better and some
get worse. These are taken from the expected output in your patch.

Better: the caret moves to where the actual problem is. For a
nested navigation it moves from the outer call to the nested one.
For "must include at least one column reference" it moves from the
start of the call to the argument that lacks a column reference.

Before:
ERROR: PREV and NEXT cannot contain PREV or NEXT
LINE 7: DEFINE A AS price > PREV(PREV(price))
^
After:
ERROR: invalid row pattern navigation function form: PREV(PREV())
LINE 7: DEFINE A AS price > PREV(PREV(price))
^

Before:
ERROR: PREV and NEXT cannot contain PREV or NEXT
LINE 7: DEFINE A AS price > NEXT(price * PREV(price))
^
After:
ERROR: invalid row pattern navigation function form: NEXT(PREV())
LINE 7: DEFINE A AS price > NEXT(price * PREV(price))
^

Before:
ERROR: argument of row pattern navigation operation must include at
least one column reference
LINE 7: DEFINE A AS PREV(1) > 0
^
After:
ERROR: argument of row pattern navigation operation must include at
least one column reference
LINE 7: DEFINE A AS PREV(1) > 0
^

The caret stays on the outer call, but the message gets better. The
new message names PREV, so the user knows which call to fix. I think
this is a good direction for the error messages.

Before:
ERROR: row pattern navigation operation must be a direct argument of
the outer navigation
LINE 6: DEFINE A AS PREV(v + FIRST(v)) > 0
^
After:
ERROR: nested row pattern navigation operation must be the direct
argument of PREV
LINE 6: DEFINE A AS PREV(v + FIRST(v)) > 0
^

Worse: when a navigation call is inside an offset. A navigation call
in the offset position is already the mistake; what is inside it is
secondary. Before, the caret was on that call. Now the inner call is
checked first, so the error is about its argument and the caret is on
that argument. The message no longer says that the offset cannot
contain a navigation operation; when the inner call has no column
reference it asks for one, and adding one only leads to the run-time
constant error.

Before:
ERROR: row pattern navigation offset cannot contain a row pattern
navigation operation
LINE 6: DEFINE A AS PREV(val, FIRST(1)) > 0)
^
After:
ERROR: argument of row pattern navigation operation must include at
least one column reference
LINE 6: DEFINE A AS PREV(val, FIRST(1)) > 0)
^

Before:
ERROR: row pattern navigation offset cannot contain a row pattern
navigation operation
LINE 4: PATTERN (A+) DEFINE A AS PREV(v, NEXT(1, 0)) > 0);
^
After:
ERROR: argument of row pattern navigation operation must include at
least one column reference
LINE 4: PATTERN (A+) DEFINE A AS PREV(v, NEXT(1, 0)) > 0);
^

I think small improvements like the position of the caret are
good contributions, and they are needed now. We look at the code
from different angles: you from its structure, I from what users
see. I have found it hard to pay attention to small things like
this myself. I think improvements of this kind may be a form of
contribution that many contributors can easily make from here on.

Once the next patch series is posted, could you rebase this on top
of it? If it makes the error positions more helpful for users, that
would be an improvement. I will check then that it does not
introduce any defects.

As a rule, the review and testing have to start over from the
beginning whenever the code changes.
The round in progress is quite expensive.

Best regards,
Henson