Re: Row pattern recognition

Hi Jian,

Thank you for explaining. I agree that the order
explains the message.

> Different ways of processing the navigation yield different error
> messages, and therefore the error position also differs.
> So i think this should be ok.

The error message should be decided from the user's side first, and
the implementation then built to produce it. That an approach was
chosen first does not justify the message it produces. If producing
the better message were costly, I would weigh that; here I expect the
cost to be small. So the question is which message serves the user.

I think one rule answers it. When two mistakes sit at the same
level, I see no reason to report one before the other. But a
mistake inside something that is already wrong should not come
first. Take PREV(val, FIRST(1)). A navigation call in the
offset is already the mistake. Your patch reports that FIRST(1)
has no column reference, so the user adds one, and only then
learns that the offset must be a run-time constant. Fixing
the offset removes FIRST(1) altogether. The nesting rows are a
structural case: PREV inside PREV is not allowed, and the code
before your patch and the other two engines say so first.

I ran these expressions on Oracle 23.26 and Trino 471, and on the
code before and after your patch ("before" and "yours"). "offset"
means the message is about the offset, "arg" that it is about the
argument of the inner call, and "nest" is a nesting error. "*" marks
a message about the inner mistake when an outer one is there. "**"
marks a message that differs from Oracle and Trino but is about
one of two mistakes at the same level, which I think is fine.

DEFINE A AS val > <expression>

expression Oracle Trino before yours
-- inner mistake inside a mistaken offset
PREV(val, FIRST(1)) offset offset offset arg*
PREV(val, PREV(1)) offset offset offset arg*
-- inner mistake inside a disallowed nesting
PREV(PREV(1), 1) nest nest nest arg*
FIRST(PREV(1), 1) nest nest nest arg*
-- no inner mistake
PREV(val, FIRST(val)) offset offset offset offset
PREV(val, val) offset offset offset offset
PREV(PREV(val)) nest nest nest nest
-- mistakes at the same level
PREV(FIRST(1), val) offset offset arg** arg**
PREV(1, val) offset offset arg** offset

In the first four rows the code before your patch reports the
outer mistake, and yours reports the inner one. Oracle and
Trino report the inner one in none of them. Oracle has no
column-reference check at all, since it accepts PREV(FIRST(1)),
so for the first two rows only Trino, which has the check, shows
an order. The standard does not say which error to report first,
so I do not claim their order as a rule. The last two rows are
mistakes at the same level, and I make no claim about them.

The expected output of your patch shows the same thing.
Seven cases change from a message about the outer mistake
to one about the inner one: six where the offset contains
a navigation call, such as PREV(v, FIRST(1) + 1) and
PREV(FIRST(v), LAST(1)), and PREV(FIRST(v, LAST(1)), 2),
where the nesting depth was reported before.

Those seven need to report the outer mistake, as before. As they
are, I would not include the patch in the series. The way to do it
follows from how ParseRPRNavCall works: it receives its arguments
already transformed, so it has to be told, while the offset is
being transformed, that this is an offset, and, for an argument,
which call it sits in and whether that nesting is allowed. I expect
the change to be small: a field in ParseState that is set while
the offset is transformed. PostgreSQL passes the context down the
same way for an aggregate in a window frame offset, which reports
"aggregate functions are not allowed in window ROWS". A name that
cannot be resolved still comes first there, as it must; this check
is about the shape of the call. If you would rather not make this
change, I will make it myself when I put the patch on top of the
next series. The rest of your patch can stay as it is.

Of the changed messages in the expected output, most only
move the caret or reword and are fine, and these seven got
worse. With them fixed, I will include the patch in the next
series after verifying its quality, as I said earlier.

Best regards,
Henson