> 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
