enable overflow for accu keys, to_analysis_urls, and submission_cmd_args #40

ens-bwalts · 2018-07-26T12:50:36Z

No description provided.

coveralls · 2018-07-26T12:59:03Z

Coverage increased (+3.7%) to 54.762% when pulling 8beee26 on experimental/overflow_fields into 121b02e on version/2.2.

muffato

First of all, very good effort to push the fix back to 1.9 ! It's brave of you :)

I like how transparent the change would be for the users / pipelines. Can you add analysis_ctrl_rule.condition_analysis_url to the list of overflow-able fields ?

Finally, would you be able to add a test ?

modules/Bio/EnsEMBL/Hive/DBSQL/ResourceDescriptionAdaptor.pm

muffato · 2018-07-27T09:44:30Z

modules/Bio/EnsEMBL/Hive/DBSQL/AccumulatorAdaptor.pm

+sub overflow_limit {
+    return {
+        'key_signature' => 255,
+        'struct_name'   => 255,


Not sure about overflowing struct_name. I see it as a static identifier / parameter name, like logic_name, pipeline_wide_parameters.param_name, etc. I also think they should be protected, but perhaps differently. Maybe introduce a size_limit method in the adaptors that define a maximum allowed size for some fields, and the base store method checks the values before doing the INSERT ? It might be possible to get those limits automatically through DBSQL::BaseAdaptor::_table_info_loader

struct_name needs protection because it's linked to to_analysis_url, which is itself protected. As for param_name, logic_name, and others that get set at init time - I'm not sure it's worth introducing a size limit which would throw an error, since the MySQL error that gets thrown is actually informative in this case.

It actually depends how the MySQL server has been configured. mysql-ens-compara-prod-1 complains:

DBD::mysql::st execute failed: Data too long for column 'logic_name' at row 1 at /home/matthieu/workspace/src/hive/2.5/modules/Bio/EnsEMBL/Hive/DBSQL/StatementHandle.pm line 140.

whereas mysql-ens-reg-prod-4 silently truncates the logic name to 256 characters

Remove struct_name ?

Have been experimenting (not pushed yet) with getting sizes in _table_info_loader(). Straightforward, but for many types COLUMN_SIZE returns null. These are types such as INT or TEXT, where either the size is very large, or that we are less concerned about. The bottom line being that the check against size_limit is not comprehensive without a lot of work (and somehow defining it for every column elsewhere in the code), but it does cover the cases that the ticket and this PR care about

modules/Bio/EnsEMBL/Hive/DBSQL/AccumulatorAdaptor.pm

muffato · 2018-07-27T09:58:12Z

modules/Bio/EnsEMBL/Hive/DBSQL/AccumulatorAdaptor.pm

+
+
+             } @$fields ];
+}


Did you override slicer here because AccumulatorAdaptor is the only NakedTableAdaptor that has overflowing and you wanted to keep NakedTableAdaptor's slicer simple and straightforward ?

It just felt surprising at the first glance. Maybe you can add a comment on top of NakedTableAdaptor::slicer indicating that should we need to move the overflow capability up, we can find it in AccumulatorAdaptor ?

Yes - I didn't want to impose the overhead on every inheriting Adaptor. However, the overhead is small, and for maintainability it's probably better to move it to the base class

muffato

General comment: the adaptors should implement default_overflow_limit, not overflow_limit

t/02.api/overflow.t

muffato · 2019-03-20T16:03:53Z

modules/Bio/EnsEMBL/Hive/DBSQL/NakedTableAdaptor.pm

-    return [ @$hashref{@$fields} ];
+    my $overflow_limit = $self->overflow_limit();
+
+    return [ map { eval { my $value = $hashref->{$_};


I know there is an eval in ObjectAdaptor too, but do we really need them ? I am concerned they could hide some errors

muffato · 2019-03-20T16:19:33Z

modules/Bio/EnsEMBL/Hive/DBSQL/NakedTableAdaptor.pm

+    my $overflow_limit = $self->overflow_limit();
+
+    return [ map { eval { my $value = $hashref->{$_};
+                          my $ol = $overflow_limit->{$_};


This is almost exactly the same as in ObjectAdaptor. I would prefer this to be in BaseAdaptor but I'm not sure how to do it cleanly, clearly and efficiently.

muffato · 2019-03-20T16:20:43Z

modules/Bio/EnsEMBL/Hive/DBSQL/AccumulatorAdaptor.pm

+sub overflow_limit {
+    return {
+        'key_signature' => 255,
+        'struct_name'   => 255,


Remove struct_name ?

…aseAdaptor

muffato

General comments across:

Adaptors should implement default_overflow_limit, not overflow_limit
Since we now get the maximum size from the database engine, the interesting part of the overflow-limit hash is really just the set of keys, since the values should be the same as the column size. Maybe on version/* branches, this should remain as it is in order to minimize the changes, but on master, what about replacing overflow_limit with overflow_list which would be the list of the columns allowed to overflow ? BaseAdaptor would read their maximum size from size_limit

On a completely separate note, what is your policy about column width ? There is a line in overflow.t with 161 characters, but line 628 in BaseAdaptor has been split (it would have been 118 characters otherwise). I find that the line-returns at lines 620 and 628 in BaseAdaptor break the flow

muffato · 2019-04-05T14:59:28Z

modules/Bio/EnsEMBL/Hive/DBSQL/BaseAdaptor.pm

+    }
+}
+
+sub slicer {


I like this slicer. It's got all the functionality we want, and it avoids duplicating code and logic.
In principle, though, this method in BaseAdaptor should not know about sub-clases (NakedTable vs Object) and how to treat them differently ($is_object). A purist would ask to introduce a method $self->get_field_value($sliceable, $field) that would be implemented as a hash lookup in NakedTableAdaptor and a method call in ObjectAdaptor. I don't know yet if I want this or not

muffato · 2019-04-05T15:24:46Z

modules/Bio/EnsEMBL/Hive/DBSQL/BaseAdaptor.pm

+        # the way slicer does column size checking. Likewise, PostgreSQL reports
+        # a user-defined TYPE_NAME for enums, rather than 'enum'. Therefore, if
+        # the DB is PostgreSQL, only set size_limit for varchars
+        unless (($driver eq 'pgsql') && !($column_type eq 'character varying')) {


Maybe a matter of taste, but I find an if would be way clearer

if (($driver ne 'pgsql') || ($column_type eq 'character varying')) {

muffato · 2019-04-05T15:38:01Z

modules/Bio/EnsEMBL/Hive/DBSQL/BaseAdaptor.pm

@@ -138,6 +139,26 @@ sub overflow_limit {
    return $self->{_overflow_limit} || $self->default_overflow_limit();
 }

+=head2 size_limit


The paradigm of BaseAdaptor is to have a default_ version of all those methods, that is only a getter, and then the normal version which is a getter/setter and falls back to the default_ version to set the initial value

muffato · 2019-04-05T16:00:16Z

modules/Bio/EnsEMBL/Hive/DBSQL/BaseAdaptor.pm

+        # a user-defined TYPE_NAME for enums, rather than 'enum'. Therefore, if
+        # the DB is PostgreSQL, only set size_limit for varchars
+        unless (($driver eq 'pgsql') && !($column_type eq 'character varying')) {
+            $size_limit{$column_name}  = $size_limit;


This will set a size limit for all column types, incl. integers, enums and timestamps, but I'm not sure these are correctly enforceable:

For integers, we get the size of the longest possible string that fits in. For instance, for the standard INT (32 bits) which ranges from -2^32 to 2^32-1, that's 11 characters, but in fact, not all 11-digit integers can fit there.

ENUMs are given the size of their longest element

TIMESTAMPs are given 14 characters, which I find scary. My text editor tells me that 2019-04-05 15:47:14 has 19 characters, so the API would start throwing when storing timestamps ?? Although these columns are most often set to CURRENT_TIMESTAMP in a way that bypasses slicer, I fear a corner case whereby eHive stores a previously-fetched object that already has a timestamp and the adaptor would now fail.

I would suggest to only populate size_limit for text fields.

nerdstrike · 2019-07-08T13:24:30Z

modules/Bio/EnsEMBL/Hive/DBSQL/AccumulatorAdaptor.pm

@@ -48,6 +48,13 @@ sub default_table_name {
 }


+sub overflow_limit {


It's odd to me how many times you overload the overflow_limit sub.

ens-bwalts requested a review from muffato July 26, 2018 13:03

muffato suggested changes Jul 27, 2018

View reviewed changes

ens-bwalts force-pushed the experimental/overflow_fields branch from a81f310 to 81dee94 Compare January 21, 2019 15:04

ens-bwalts changed the base branch from version/1.9 to version/2.2 January 21, 2019 15:04

ens-bwalts force-pushed the experimental/overflow_fields branch from 81dee94 to c25c96d Compare January 22, 2019 15:13

ens-bwalts added 3 commits March 15, 2019 11:10

enable overflow for accu keys, to_analysis_urls, and submission_cmd_args

1e85f0b

improvements to overflow handling

48b3e58

syntax for hashref element access changes between Perl versions

1beb0a5

ens-bwalts force-pushed the experimental/overflow_fields branch from c25c96d to 1beb0a5 Compare March 18, 2019 16:26

muffato suggested changes Mar 20, 2019

View reviewed changes

ens-bwalts added 2 commits April 3, 2019 09:54

Suggested improvements to overflow code, including moving slicer to B…

d657f9d

…aseAdaptor

When using PostgreSQL, only set a size_limit for VARCHAR columns

8beee26

ens-bwalts force-pushed the experimental/overflow_fields branch from 7bd2fdc to 8beee26 Compare April 3, 2019 12:48

mkszuba requested a review from mira13 April 5, 2019 12:46

mira13 approved these changes Apr 5, 2019

View reviewed changes

muffato suggested changes Apr 5, 2019

View reviewed changes

nerdstrike reviewed Jul 8, 2019

View reviewed changes

ens-bwalts added the no_remind label Jul 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable overflow for accu keys, to_analysis_urls, and submission_cmd_args #40

enable overflow for accu keys, to_analysis_urls, and submission_cmd_args #40

ens-bwalts commented Jul 26, 2018

coveralls commented Jul 26, 2018 •

edited

Loading

muffato left a comment

muffato Jul 27, 2018

ens-bwalts Aug 24, 2018

muffato Aug 31, 2018

muffato Mar 20, 2019

ens-bwalts Mar 29, 2019

muffato Jul 27, 2018

ens-bwalts Jul 30, 2018

muffato left a comment

muffato Mar 20, 2019

muffato Mar 20, 2019

muffato Mar 20, 2019

muffato left a comment

muffato Apr 5, 2019

muffato Apr 5, 2019

muffato Apr 5, 2019

muffato Apr 5, 2019

nerdstrike Jul 8, 2019

		@@ -48,6 +48,13 @@ sub default_table_name {
		}


		sub overflow_limit {

enable overflow for accu keys, to_analysis_urls, and submission_cmd_args #40

Are you sure you want to change the base?

enable overflow for accu keys, to_analysis_urls, and submission_cmd_args #40

Conversation

ens-bwalts commented Jul 26, 2018

coveralls commented Jul 26, 2018 • edited Loading

muffato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muffato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muffato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 26, 2018 •

edited

Loading