HI,
> I've looked at this patch and it looks mostly fine, though I do not
> intend to commit it myself; perhaps Andrew will.
HI, Amit, thanks for review.
>
> A few minor things to improve:
>
> + If <literal>*</literal> is specified, it will be applied in all
> columns.
> ...
> + If <literal>*</literal> is specified, it will be applied in all
> columns.
>
> Please write "it will be applied in" as "the option will be applied to".
+1
>
> + bool force_notnull_all; /* FORCE_NOT_NULL * */
> ...
> + bool force_null_all; /* FORCE_NULL * */
>
> Like in the comment for force_quote, please add a "?" after * in the
> above comments.
+1
>
> + if (cstate->opts.force_notnull_all)
> + MemSet(cstate->opts.force_notnull_flags, true, num_phys_attrs
> * sizeof(bool));
> ...
> + if (cstate->opts.force_null_all)
> + MemSet(cstate->opts.force_null_flags, true, num_phys_attrs *
> sizeof(bool));
>
> While I am not especially opposed to using this 1-line variant to set
> the flags array, it does mean that there are now different styles
> being used for similar code, because force_quote_flags uses a for
> loop:
>
> if (cstate->opts.force_quote_all)
> {
> int i;
>
> for (i = 0; i < num_phys_attrs; i++)
> cstate->opts.force_quote_flags[i] = true;
> }
>
> Perhaps we could fix the inconsistency by changing the force_quote_all
> code to use MemSet() too. I'll defer whether to do that to Andrew's
> judgement.
Sure, let’s wait for Andrew and I will put everything in one pot then.
Zhang Mingli
https://www.hashdata.xyz
