Hi Tatsuo, I found Trino uses "empty match" too [2]. So for SQL users, I guess > "empty match" is more familiar wording. >
Agreed. Since RPR is a feature defined by the SQL standard, using the standard's own terminology is the right choice: it lets users cross-reference the specification directly. That said, "zero-length match" is widely used in the regex literature (PCRE2, Perl, Python, Java, etc.), so developers researching the topic should treat the two terms as synonymous. In the SQL context, "empty match" is the preferred term. > > (a) Reorder within nodeWindowAgg.c: move the nfa_* functions up and > > keep the "API exposed to window functions" section at the bottom, > > matching master's layout. > > > > (b) Separate file under src/backend/executor/, keeping it close to > > nodeWindowAgg.c while making the boundary explicit. > > > > (c) A dedicated src/backend/rpr/ directory modeled on > > src/backend/regex/, giving the NFA engine its own namespace. > > This could also be an opportunity to consolidate the existing > > src/backend/optimizer/plan/rpr.c into the same directory. > > I prefer (a) or (b) for now, at least for the first commit. The reason > is, current nfa functions take a WindowAggState argument. If we prefer > (c), I think we need to change some of (or most of) nfa functions so > that they do not take the WindowAggState argument. What do you think? > That is a valid concern. (c) will be necessary in the long run, but MATCH_RECOGNIZE (R010) is not yet on the horizon, and restructuring the NFA layer now -- before that requirement is concrete -- carries a real risk of introducing defects during the refactoring. The risks outweigh the benefits at this stage. Instead, I'll try (b): a separate file under src/backend/executor/ can retain the WindowAggState argument while making the NFA boundary explicit, and the change is still reviewable. If (b) turns out to be harder than expected or surfaces unexpected issues, I'll fall back to (a). If (b) works out, I'll add a structured header comment to the new file to preserve the algorithm description for future reviewers. I am not familiar with ECPG. Do you know if ECPG has Window clause > tests? If ECPG does not have any Window clause tests, is it worth to > add RPR tests to ECPG? > I checked src/interfaces/ecpg/test/ and confirmed there are no Window clause tests there at all. If ECPG has no Window clause coverage, I see no reason to add RPR tests there at this stage. I think it would be better to leave ECPG tests out of this patch set for now. Best regards, Henson
