Hi, Nikita!
On Jan 18, Nikita Malyavin wrote:
> > > diff --git a/sql/table.cc b/sql/table.cc
> > > index ab1a04ccae4..87fa1ba4d61 100644
> > > --- a/sql/table.cc
> > > +++ b/sql/table.cc
> > > @@ -1228,6 +1228,14 @@ static const Type_handler
> > > *old_frm_type_handler(uint pack_flag,
> > > }
> > >
> > >
> > > +bool TABLE_SHARE::init_period_from_extra2(period_info_t &period,
> > > + const uchar *data)
> > > +{
> > > + period.start_fieldno= uint2korr(data);
> > > + period.end_fieldno= uint2korr(data + sizeof(uint16));
> >
> > not "sizeof(uint16)" but 2. I don't think it's guaranteed that
> > sizeof(uint16) == 2, but as you do uint2korr, you only read 2 bytes,
> > so data + 2 is always correct.
> >
> Yes, it's correct, and I've fixed it, but it wasn't about coorrectness
> -- but readability.
> Now what 4, or what is 8 -- is the question that the reader could
> сonsider. This numbers look magical.
> With using sizeof(uint16) the one could at least understand, that this
> is about several double-byte words read.
In a similar case I used something like
#define SIZE 2
#define uintSIZEkorr(X) uint2korr(X)
This way there are no magically-looking numbers. You can even
write `uint ## SIZE ## korr`, if you're feeling adventurous.
> The another problem is that sizeof(uint16) is already used in system
> versioning, few lines below, so the resulting code will look patchy
> here.
>
> > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
> > > + f->field_name.str, "VIRTUAL or GENERATED");
> >
> > just use "GENERATED", no English in the error message parameters, please.
> >
> VIRTUAL/GENERATED? What'd You say?
You cannot have parts of an English phrase as a parameter.
And "VIRTUAL" is just a subset of "GENERATED" anyway.
So, just write
my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
f->field_name.str, "GENERATED");
or, better,
my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
f->field_name.str, "GENERATED ALWAYS AS");
> > > + else if (f->flags & VERS_SYSTEM_FIELD)
> > > + {
> > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str,
> > > + f->flags & VERS_SYS_START_FLAG ? "ROW START" : "ROW END");
> >
> > No need to have a special case for that. Use the same error as for
> > vcol_info:
> >
> > if (f->vcol_info || f->flags & VERS_SYSTEM_FIELD)
> > {
> > my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
> > f->field_name.str, "GENERATED ALWAYS AS");
> >
> Looks not very much user-friendly I think. Let's leave the verbosity as it was
No, it's illogical. You have columns, like
a TIMESTAMP(6) GENERATED ALWAYS AS (NOW() + INTERVAL 1 DAY),
b TIMESTAMP(6) GENERATED ALWAYS AS ROW START
for the first column the error is
Period field %`s cannot be GENERATED
for the second column the error is
Period field %`s cannot be ROW START
Why? They're both GENERATED. You don't say
Period field %`s cannot be NOW() + INTERVAL 1 DAY
See? So just say in both cases that the period cannot be
"GENERATED ALWAYS AS". This is quite sufficient.
> > > +ER_PERIOD_TYPES_MISMATCH
> > > + eng "PERIOD FOR %`s fields types mismatch"
> > > +
> > > +ER_PERIOD_MAX_COUNT_EXCEEDED
> >
> > better say ER_MORE_THAN_ONE_PERIOD or something...
> > "max count exceeded" suggests there is some not-trivial
> > (and possibly variable) max count
> >
> > Or ER_PERIOD_ALREADY_DEFINED or ER_DUPLICATE_PERIOD etc.
> > "already defined" looks like a good one.
> >
> > And I've already commented (in another patch) on using English
> > words as arguments - it will look like a bug when the error message is
> > translated
> >
> I like ER_MORE_THAN_ONE_PERIOD more. IMO application-time periods
> designed in the way to support a possibility of several periods
> definition, so i like to treat it as "N periods max supported, but now
> N=1"
The standard clearly says there can be at most one application-time period.
So "Application-time period already defined" is just as fine.
When the new standard will allow for many application-time periods, we
can rewrite the error message. But don't hold your breath.
And, again, you cannot have "application" as a parameter.
Regards,
Sergei
Chief Architect MariaDB
and secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
