On Tue, 2021-03-02 at 10:35 -0800, Zhihong Yu wrote:
> Hi,
Thanks for the review!
> + /* Make sure the the new slot is not dependent on the original tuple */
>
> There is duplicate 'the'.
Thanks, I'll add this for the next batch of updates.
> For neededColumnContextWalker(),
>
> + else if(var->varattno == 0) {
>
> I think the if following the else is not needed - I assume var->varattno
> wouldn't be negative.
> Similar comment for extract_scan_columns().
I think you can have system columns in the tree here -- a common
example that we run into with the `make check` suite is ctid. (To see
this, you can change the (> 0) check just above this code into a (!= 0)
check, and then take a look at the failing cases in the test suite.)
> + while ((col = bms_next_member(parent_cols, col)) >= 0)
> + {
> + Var *var = (Var *) list_nth(translated_vars, col - 1);
>
> If col is 0, do you still want to call list_nth() ?
The (col == 0) case is handled just above this, with
contains_whole_row_col() and get_ordinal_attnos() replacing any zero
columns with the entire user-column range. If one of those functions
fails to do its job due to programmer error, we'll assert in the call
to list_nth(), and I think that's what we want.
--Jacob
