From: | Zhihong Yu <zyu(at)yugabyte(dot)com> |
---|---|
To: | Rémi Lapeyre <remi(dot)lapeyre(at)lenstra(dot)fr> |
Cc: | David Steele <david(at)pgmasters(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, vignesh C <vignesh21(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Daniel Verite <daniel(at)manitou-mail(dot)org> |
Subject: | Re: Add header support to text format and matching feature |
Date: | 2021-04-11 13:28:21 |
Message-ID: | CALNJ-vRCei7pSp6-Atf3Zy3Y88UDzKEVTCxPxA4AVvUebxSfNg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sun, Apr 11, 2021 at 4:01 AM Rémi Lapeyre <remi(dot)lapeyre(at)lenstra(dot)fr>
wrote:
> >
> > Hi,
> > >> sure it matches what is expected and exit immediatly if it does not.
> >
> > Typo: immediately
> >
> > +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER
> file_server
> >
> > nit: since header is singular, you can name the table header_doesnt_match
> >
> > + from the one expected, or the name or case do not match, the copy
> will
> >
> > For 'the name or case do not match', either use plural for the subjects
> or change 'do' to doesn't
> >
>
> Thanks, I fixed both typos.
>
> > - opts_out->header_line = defGetBoolean(defel);
> > + opts_out->header_line = DefGetCopyHeader(defel);
> >
> > Existing method starts with lower case d, I wonder why the new method
> starts with upper case D.
> >
>
> I don’t remember why I used DefGetCopyHeader, should I change it?
>
> > + if (fldct < list_length(cstate->attnumlist))
> > + ereport(ERROR,
> > + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> > + errmsg("missing header")));
> >
> > The message seems to be inaccurate: the header may be there - it just
> misses some fields.
>
> I changed the error messages, they now are:
> ERROR: incomplete header, expected 3 columns but got 2
> ERROR: extra data after last expected header, expected 3 columns but
> got 4
>
> >
> > + * Represents whether the header must be absent, present or present and
> match.
> >
> > present and match: it seems present is redundant - if header is absent,
> how can it match ?
>
> This now reads "Represents whether the header must be absent, present or
> match.”.
>
> Cheers,
> Rémi
>
> >
> > Cheers
>
>
> >> This now reads "Represents whether the header must be absent, present
or match.”.
Since match shouldn't be preceded with be, I think we can say:
Represents whether the header must match, be absent or be present.
Cheers
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2021-04-11 14:13:23 | Re: PL/R regression on windows, but not linux with master. |
Previous Message | Andrew Dunstan | 2021-04-11 12:04:51 | Re: PL/R regression on windows, but not linux with master. |