Re: [Maria-developers] INET6, data type plugins
Hi Sergei,
Thanks for review. Sorry for delay with this.
I've been busy with the other tasks.
I won't reply to every your comment in this letter.
I skip all comments that I agree with, and those that I need
some time to think.
Only some critical and unclear follow below:
On 08/10/2014 03:22 PM, Sergei Golubchik wrote:
skip
=== modified file 'sql/field.h'
--- sql/field.h 2014-06-11 08:08:08 +
+++ sql/field.h 2014-07-01 10:38:12 +
@@ -59,6 +60,15 @@ enum Derivation
DERIVATION_EXPLICIT= 0
};
+
+/* Data type derivation */
+enum Type_derivation
+{
+ TYPE_DERIVATION_COERCIBLE= 1, // Non-typified literals
+ TYPE_DERIVATION_IMPLICIT= 0 // Fields, functions, typified literals
+};
Interesting. Does it affect the existing behavior?
Or you only use it for new types?
I think both alternatives are far from ideal: the former means
incompatibilities, the latter - that new types behave differently from old
ones. Still, given the choice, I'd prefer consistency over compatibility.
Currently my patch implements this only for the new data types.
But I've been thinking about the idea of type derivation for a long
time for the standard types as well.
I find that taking into account type derivation during type aggregation
makes behavior clearer in most cases.
Also, this approach forces using indexes in more cases, when you do:
WHERE field = literal
There should be three levels actually:
enum Type_derivation
{
TYPE_DERIVATION_COERCIBLE= 2, // Non-typified literals
TYPE_DERIVATION_IMPLICIT= 1, // Fields, functions, typified literals
TYPE_DERIVATION_EXPLICIT= 0, // Explicit CAST(expr AS type)
};
So when you do:
WHERE field=CAST(literal AS force_type)
comparison is done according to force_type,
no matter what the type of field is.
Some examples:
1. WHERE char_field=10
will compare as string.
Currently it compares as real.
2. WHERE int_field='10'
will compare as int.
Currently it compares as real.
3. WHERE int_field='string'
will return impossible condition,
as the literal does not cast to INT safely.
Currently it compares as real, and just wastes time,
because the result set is empty anyway.
4. WHERE int_field=10.1
will return impossible condition,
as the literal does not cast to INT safely.
Currently it compares as real (and wastes time again).
5. WHERE time_field='00:00:00'
will compare as TIME.
Currently it does the same. Nothing change.
6. WHERE string_field=TIME'00:00:00'
Open question. Some variants are possible:
a. typified literals are weaker than fields
(as it is still a literal on the right side)
b. typified literals are stronger than fields
(as mentioning the type is a sort of CAST)
c. typified literals have the same type strength with fields,
use the current rules that we have for the type aggregation now.
If we go (a) or (b), then a separate type derivation level is needed.
Currently it compares as TIME (in all cases when a TIME argument is given).
If we agree on this proposal with the idea of type derivation,
then it should be a separate task.
skip
I don't like the idea of fallback_field_type.
You have Field_type_joiner for new types, but you fallback to
Field::field_type_merge() for old types.
Not only. We need something to do when two different non-standard
types are being aggregated.
My idea was:
Data types will go in packages. INET6 and INET4 will be in the same
package and will know about each other, and will know how to aggregate
to each other (INET6 is a super-type for INET4).
But we also need to do something when we have two absolutely different
data types, which do not know each other.
1. We could return an error in such cases, which would be more SQL
standard strict behavior.
2. Or, we could use fallback_field_type(), which would be a more MariaDB
behavior (and more consistent with the standard data types).
So what would you prefer?
N1,
or N2 (have fallback_field_type(), but limit its use only for the cases
when two data types that do not know each other are being aggregated)?
I'd rather not distinguish
between old and new types. Just like we have with storage engines or
authentication - there are no native and plugin storage engines (or
authentication), *all* storage engines are plugins. So there's no code like
if (is_plugin(engine_type))
...
else
...
so, I prefer uniform code with no special checks for type handlers.
Implementations differ, but the interface is the same for all types.
skip
+ virtual bool varbinary_to_raw(const String *from, String *to) const = 0;
1. This most certainly needs a comment. It took me quite a while to
understand what this method is for and why str_to_raw isn't used instead.
A comment should say that it's used when a field binary representation is
specified in a query directly was 0x...
2. Still, I'm not sure this method is needed. It means a different
behavior for
where ipv6 = 0xHH...
and
create t1 (b binary(16)); insert t1 values
Re: [Maria-developers] [Commits] [s...@mariadb.org: Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0]
Hi Sergey, Thanks for the fix! I've merged your change into Groonga: https://github.com/groonga/groonga/commit/b3d38c5b9863cb7a9e93b9461c8b93fbfbfa79dc Thanks, -- kou In 20141009083146.GA3231@june [Commits] [s...@mariadb.org: Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0] on Thu, 9 Oct 2014 12:31:47 +0400, Sergey Vojtovich s...@mariadb.org wrote: Hi Kentoku, FYI: I just pushed this fix to 10.0. There're also some test failures on Power8 in mroonga/storage suite, mroonga/wrapper was 100% successful. Regards, Sergey - Forwarded message from Sergey Vojtovich s...@mariadb.org - Date: Thu, 9 Oct 2014 08:25:58 + (UTC) From: Sergey Vojtovich s...@mariadb.org To: comm...@mariadb.org Subject: [Commits] Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0 At lp:maria/10.0 revno: 4437 revision-id: s...@mariadb.org-20141009082545-nt183b7y1gjihfjp parent: pser...@askmonty.org-20141006112922-45pcytb993xcwh89 committer: Sergey Vojtovich s...@mariadb.org branch nick: 10.0-mroonga timestamp: Thu 2014-10-09 12:25:45 +0400 message: Fixed mroonga build failure on Power8: define generic gcc version of GRN_SET_64BIT. === modified file 'storage/mroonga/vendor/groonga/lib/groonga_in.h' --- a/storage/mroonga/vendor/groonga/lib/groonga_in.h 2014-09-20 15:33:45 + +++ b/storage/mroonga/vendor/groonga/lib/groonga_in.h 2014-10-09 08:25:45 + @@ -505,6 +505,9 @@ typedef int grn_cond; /* todo */ # define GRN_SET_64BIT(p,v) \ (void)atomic_swap_64(p, v) +# elif defined(__ATOMIC_SEQ_CST) /* GCC atomic builtins */ +# define GRN_SET_64BIT(p,v) \ + __atomic_store_n(p, v, __ATOMIC_SEQ_CST) # endif /* ATOMIC 64BIT SET */ # ifdef HAVE_MKOSTEMP ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits - End forwarded message - ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits ___ 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
Re: [Maria-developers] [Commits] [s...@mariadb.org: Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0]
Hi Kouhei, Kentoku, thanks for merging this fix. Test failures that I was talking about are now reproduced by buildbot: http://buildbot.askmonty.org/buildbot/builders/bintar-rhel6-p8-debug/builds/168/steps/test/logs/stdio From this log I can guess it has something to do with endianness: Power8 is big-endian. If that's the case little-endian bintar-trusty-p8-debug should run these tests successfully (but this build isn't ready yet). Is it something you will be interested fixing? Regards, Sergey On Thu, Oct 09, 2014 at 05:41:34PM +0900, Kouhei Sutou wrote: Hi Sergey, Thanks for the fix! I've merged your change into Groonga: https://github.com/groonga/groonga/commit/b3d38c5b9863cb7a9e93b9461c8b93fbfbfa79dc Thanks, -- kou In 20141009083146.GA3231@june [Commits] [s...@mariadb.org: Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0] on Thu, 9 Oct 2014 12:31:47 +0400, Sergey Vojtovich s...@mariadb.org wrote: Hi Kentoku, FYI: I just pushed this fix to 10.0. There're also some test failures on Power8 in mroonga/storage suite, mroonga/wrapper was 100% successful. Regards, Sergey - Forwarded message from Sergey Vojtovich s...@mariadb.org - Date: Thu, 9 Oct 2014 08:25:58 + (UTC) From: Sergey Vojtovich s...@mariadb.org To: comm...@mariadb.org Subject: [Commits] Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0 At lp:maria/10.0 revno: 4437 revision-id: s...@mariadb.org-20141009082545-nt183b7y1gjihfjp parent: pser...@askmonty.org-20141006112922-45pcytb993xcwh89 committer: Sergey Vojtovich s...@mariadb.org branch nick: 10.0-mroonga timestamp: Thu 2014-10-09 12:25:45 +0400 message: Fixed mroonga build failure on Power8: define generic gcc version of GRN_SET_64BIT. === modified file 'storage/mroonga/vendor/groonga/lib/groonga_in.h' --- a/storage/mroonga/vendor/groonga/lib/groonga_in.h 2014-09-20 15:33:45 + +++ b/storage/mroonga/vendor/groonga/lib/groonga_in.h 2014-10-09 08:25:45 + @@ -505,6 +505,9 @@ typedef int grn_cond; /* todo */ # define GRN_SET_64BIT(p,v) \ (void)atomic_swap_64(p, v) +# elif defined(__ATOMIC_SEQ_CST) /* GCC atomic builtins */ +# define GRN_SET_64BIT(p,v) \ + __atomic_store_n(p, v, __ATOMIC_SEQ_CST) # endif /* ATOMIC 64BIT SET */ # ifdef HAVE_MKOSTEMP ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits - End forwarded message - ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits ___ 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
Re: [Maria-developers] [Commits] [s...@mariadb.org: Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0]
FYI: tests on little-endian Power8 fail too, but differently: http://buildbot.askmonty.org/buildbot/builders/bintar-trusty-p8/builds/137/steps/test/logs/stdio On Thu, Oct 09, 2014 at 02:09:21PM +0400, Sergey Vojtovich wrote: Hi Kouhei, Kentoku, thanks for merging this fix. Test failures that I was talking about are now reproduced by buildbot: http://buildbot.askmonty.org/buildbot/builders/bintar-rhel6-p8-debug/builds/168/steps/test/logs/stdio From this log I can guess it has something to do with endianness: Power8 is big-endian. If that's the case little-endian bintar-trusty-p8-debug should run these tests successfully (but this build isn't ready yet). Is it something you will be interested fixing? Regards, Sergey On Thu, Oct 09, 2014 at 05:41:34PM +0900, Kouhei Sutou wrote: Hi Sergey, Thanks for the fix! I've merged your change into Groonga: https://github.com/groonga/groonga/commit/b3d38c5b9863cb7a9e93b9461c8b93fbfbfa79dc Thanks, -- kou In 20141009083146.GA3231@june [Commits] [s...@mariadb.org: Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0] on Thu, 9 Oct 2014 12:31:47 +0400, Sergey Vojtovich s...@mariadb.org wrote: Hi Kentoku, FYI: I just pushed this fix to 10.0. There're also some test failures on Power8 in mroonga/storage suite, mroonga/wrapper was 100% successful. Regards, Sergey - Forwarded message from Sergey Vojtovich s...@mariadb.org - Date: Thu, 9 Oct 2014 08:25:58 + (UTC) From: Sergey Vojtovich s...@mariadb.org To: comm...@mariadb.org Subject: [Commits] Rev 4437: Fixed mroonga build failure on Power8: define generic gcc version of in lp:maria/10.0 At lp:maria/10.0 revno: 4437 revision-id: s...@mariadb.org-20141009082545-nt183b7y1gjihfjp parent: pser...@askmonty.org-20141006112922-45pcytb993xcwh89 committer: Sergey Vojtovich s...@mariadb.org branch nick: 10.0-mroonga timestamp: Thu 2014-10-09 12:25:45 +0400 message: Fixed mroonga build failure on Power8: define generic gcc version of GRN_SET_64BIT. === modified file 'storage/mroonga/vendor/groonga/lib/groonga_in.h' --- a/storage/mroonga/vendor/groonga/lib/groonga_in.h 2014-09-20 15:33:45 + +++ b/storage/mroonga/vendor/groonga/lib/groonga_in.h 2014-10-09 08:25:45 + @@ -505,6 +505,9 @@ typedef int grn_cond; /* todo */ # define GRN_SET_64BIT(p,v) \ (void)atomic_swap_64(p, v) +# elif defined(__ATOMIC_SEQ_CST) /* GCC atomic builtins */ +# define GRN_SET_64BIT(p,v) \ + __atomic_store_n(p, v, __ATOMIC_SEQ_CST) # endif /* ATOMIC 64BIT SET */ # ifdef HAVE_MKOSTEMP ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits - End forwarded message - ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits ___ commits mailing list comm...@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits ___ 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 ___ 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
