Re: Row pattern recognition

> CREATE TABLE stock (company TEXT, tdate DATE, price INTEGER);
> CREATE TEMP TABLE stock (company TEXT, tdate DATE, price INTEGER);
> SELECT count(*) over w FROM stock WINDOW w AS ( ROWS BETWEEN CURRENT
> ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS
> pg_temp.stock.price > 0 );
> SELECT count(*) over w FROM stock WINDOW w AS ( ROWS BETWEEN CURRENT
> ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS public.stock.price
>> 0 );
> SELECT count(*) over w FROM stock WINDOW w AS ( ROWS BETWEEN CURRENT
> ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS stock.price > 0 );
>
> The error messages for the above 3 SELECT queries are different.
> (pg_temp.stock.price, public.stock.price, stock.price) mean the same
> thing: column reference,
> Should we try to make the error messages consistent?

Tatsuo has already covered this one, and I'm with him: I'd keep the three
as they are.  They come from three different paths -- a schema-qualified
reference rejected inside DEFINE, a public.stock that simply isn't the
FROM-clause relation, and a range-variable qualification -- so a single
message can't describe all three accurately.

It's worth saying why beyond "different paths".  In the standard a row
pattern variable is itself a range variable -- ISO/IEC 19075-5 4.10 has them
"used to qualify column references" in MEASURES and DEFINE -- so a qualified
name
such as A.price denotes the price column at the rows mapped to pattern
variable A.  That notation collides directly with table/range-variable
qualification: A.price and stock.price are spelled identically, and a
one-component qualifier is ambiguous between a pattern variable and a
relation.  Sorting out that ambiguity is precisely what separates the three
paths above, which is the other reason I'd keep them distinct rather than
fold them into one message now.

We don't implement pattern-variable-qualified references yet -- today an
unqualified column is implicitly the universal row pattern variable, and
that is all DEFINE and MEASURES accept.  I'd like to design and build that
qualifier support once this commit lands; since it sits right on top of the
name resolution you're raising here, I'd genuinely welcome your help with
both the design and the implementation, if you're interested.

> "Range variable qualified expression" is non-standard that may confuse
users.
> To improve clarity and consistency, let's align this with the
> established error pattern:
>
> ERROR: invalid reference to FROM-clause entry for table "the_table"

"invalid reference to FROM-clause entry for table" only fits the middle
case (public.stock); routing (1) and (3) through it would misdescribe them.
On the wording, "range variable qualified" is not really non-standard -- it
is the standard's own framing, the same one above: a row pattern variable
is a range variable, and DEFINE qualification is exactly where that
distinction becomes load-bearing once qualifier resolution lands.  A generic
FROM-clause message would erase the very difference we will need to surface
then, so I'd keep the dedicated wording rather than fold it in.

> What do you all think about renaming validateRPRPatternVarCount to
> preprocessRPRPattern?

Agreed the name is too narrow -- besides checking the count against
RPR_VARID_MAX, the function collects the unique PATTERN variable names into
p_rpr_pattern_vars (the list transformColumnRef uses to spot a
pattern-variable qualifier, and for now to reject A.price).
 preprocessRPRPattern
feels too broad, though.  Rather than pin a name down now, I'd leave some
room: the function's role will shift as the qualifier work lands, so I'll
work out a fitting name as I write that patch.

> The attached v48-0001 mainly replaces PG_INT32_MAX with
> RPR_QUANTITY_INF, but it also includes other changes.
> See the commit message for detail.

Behavior-neutral and a clear improvement; I'll take it.  I'll go through the
details closely and fold in any fixups as I write the patch.

> The attached v48-0002 patch is more about miscellaneous refactoring.
> [...]
> Refactor buildRPRPattern to accept a WindowClause pointer directly.
> [...]
> collectDefineVariables is not needed [...] Also removed
tryUnwrapSingleChild
> [...]
> Slightly adjust variable limits error message.

Also good.  The helper inlining and the WindowClause signature both read as
clear wins; I'll sort out the variable-limit message wording and the rest of
the small details as I fold it.

Thanks again for the careful work.

Best regards,
Henson