On Mon, Jul 15, 2024 at 4:38 PM Sutou Kouhei <kou@clear-code.com> wrote:
> I'm not familiar with related codes but here are my
> comments:
Thanks for reviewing this patchset!
> + * Fields valid for a GROUP RTE (else NULL/zero):
>
> There is only one field and it's LIST. So how about using
> the following?
>
> * A field valid for a GROUP RTE (else NIL):
Good point. I ended up with
* Fields valid for a GROUP RTE (else NIL):
... since this is the pattern used by other types of RTEs that have
only one field.
> If we want to reuse flatten_join_alias_vars_context for
> flatten_group_exprs(), how about renaming it?
> flatten_join_alias_vars() only uses
> flatten_join_alias_vars_context for now. So the name of
> flatten_join_alias_vars_context is meaningful. But if we
> want to flatten_join_alias_vars_context for
> flatten_group_exprs() too. The name of
> flatten_join_alias_vars_context is strange.
I think the current name should be fine. It's not uncommon that we
reuse the same structure intended for other functions within one
function.
> Can the "te->resname == NULL" case be happen? If so, how
> about adding a new test for the case?
It's quite common for te->resname to be NULL, such as when TargetEntry
is resjunk. I don't think a new case for this is needed. It should
already be covered in lots of instances in the current regression
tests.
> (BTW, is "unamed_col" intentional name? Is it a typo of
> "unnamed_col"?)
Yeah, it's a typo. I changed it to be "?column?", which is the
default name if FigureColname can't guess anything.
Here is an updated version of this patchset. I'm seeking the
possibility to push this patchset sometime this month. Please let me
know if anyone thinks this is unreasonable.
Thanks
Richard
