Hi Tatsuo, Two things before the next review round. Both aim at the same thing: keeping the v50 diff to what RPR genuinely changes, so the next round reviews RPR on its own merits rather than hunks that drifted in or touch pre-existing window code.
First, the cfbot is currently red on v49 across all platforms. The cause
is not RPR: master recently reworded the RESPECT/IGNORE NULLS error, which
left rpr_base's expected output stale, so the regression diff fails "make
check" everywhere. That is exactly the one-line fix you already agreed to
fold in; I have posted it here as v50-0021 so the series builds green on
its own. This one is required regardless.
Second, while going over the v49 diff against master I found several hunks
that are either unrelated to RPR or that touch pre-existing window code. I
split them into small patches so each can be decided on its own.
Patch list
----------
Every patch in this reply. revert-* removes the change from RPR;
optional-* re-applies the same change to master separately.
v50-0021 cfbot build fix (required regardless)
revert-0001 drop unrelated include and stray blank line (required
cleanup)
revert-0004 restore doc paragraph line wrapping (required
cleanup)
revert-0002 #2 drop the mark-position diagnostic (your call)
optional-0001 #2 land the diagnostic on master
revert-0003 #3 restore the funcname guard (your call)
optional-0002 #3 land the guard removal on master
revert-0005 #5 drop the EXCLUDE TIES tests (your call)
optional-0003 #5 land the tests on master
Required cleanups
-----------------
These only remove changes that crept in but have nothing to do with RPR.
I split them out so you can fold them into the feature patch; being
required cleanups, I recommend applying them:
revert-0001 drop an unrelated "nodes/plannodes.h" include and a stray
blank line in eval_windowaggregates()
revert-0004 restore the line wrapping of two pre-existing doc
paragraphs (advanced.sgml, ref/select.sgml)
Your call
---------
The next three touch pre-existing window code, so I would rather you decide
where each belongs. I prepared the building blocks so you can pick any
option:
#2 window_gettupleslot() mark-position elog -- add row positions
v49 extended the can't-happen error with the pos/markpos values.
(a) keep it in RPR -> apply neither patch
(b) drop it -> apply revert-0002
(c) land it on master separately -> revert-0002 + optional-0001
A tiny additive diagnostic; I lean to (c), but (a) is fine too.
#3 funcname guard in WinCheckAndInitializeNullTreatment()
v49 removed the "could not get function name" guard as unreachable.
(a) keep the removal in RPR -> apply neither patch
(b) restore the guard, drop it -> apply revert-0003
(c) remove it on master separately -> revert-0003 + optional-0002
The branch is genuinely unreachable, but it drops a deliberate guard
in pre-existing code, so of the three this one most warrants caution.
Preserving the guard via (b) or (c) looks safest, but I will leave the
decision to you.
#5 EXCLUDE TIES window-frame test coverage
Two nth_value/last_value cases. The accounting they exercise already
exists on master and the tests pass without RPR, so they are not RPR
coverage.
(a) keep them in RPR -> apply neither patch
(b) land them on master separately -> revert-0005 + optional-0003
I lean to (b).
For my part, I will carry out the deeper review against v50 with v50-0021
and revert-0001/0004 applied, plus whichever of the above you choose, as
the minimal green baseline.
Best,
Henson
v50-0021-adjust-rpr_base-expected-output.patch
Description: Binary data
revert-0001-remove-incidental-changes.patch
Description: Binary data
revert-0002-revert-mark-position-elog.patch
Description: Binary data
revert-0003-restore-funcname-guard.patch
Description: Binary data
revert-0004-restore-doc-line-wrapping.patch
Description: Binary data
revert-0005-remove-exclude-ties-tests.patch
Description: Binary data
optional-0001-include-row-positions-in-mark-position-error.patch
Description: Binary data
optional-0002-remove-unreachable-funcname-guard.patch
Description: Binary data
optional-0003-add-exclude-ties-test-coverage.patch
Description: Binary data
