diff --git a/lib/GADS/Globe.pm b/lib/GADS/Globe.pm index 485495183..3096882fd 100644 --- a/lib/GADS/Globe.pm +++ b/lib/GADS/Globe.pm @@ -363,8 +363,7 @@ sub _build_data $value_color = $record->get_column('id_count'); } else { - my $field = $self->color_col->field; - $field .= "_sum" if $self->color_col_operator eq 'sum'; + my $field = $self->records->aggregate_name($self->color_col, $self->color_col_parent, $self->color_col_operator); $value_color = $record->get_column($field); if (!$self->color_col->numeric) { @@ -377,23 +376,23 @@ sub _build_data if ($self->label_col) { $value_label = $self->label_col->type eq 'curval' - ? $self->_format_curcommon($self->label_col, $record) - : $record->get_column($self->label_col->field); + ? $self->_format_curcommon($self->label_col, $self->label_col_parent, $record) + : $record->get_column($self->records->aggregate_name($self->label_col, $self->label_col_parent, 'max')); $value_label ||= ''; } if ($self->group_col) { - my $field = $self->group_col->field; + my $field = $self->records->aggregate_name($self->group_col, $self->group_col_parent, 'max'); $field .= "_sum" if $self->group_col_operator eq 'sum'; $value_group = $self->group_col->type eq 'curval' - ? $self->_format_curcommon($self->group_col, $record) + ? $self->_format_curcommon($self->group_col, $self->group_col_parent, $record) : $record->get_column($field) || ''; } foreach my $column (@{$self->_columns_globe}) { - my $country = $record->get_column($column->field) + my $country = $record->get_column($self->records->aggregate_name($column, undef, 'max')) or next; push @this_countries, $country; @@ -625,10 +624,13 @@ sub uniq_join } sub _format_curcommon -{ my ($self, $column, $line) = @_; - $line->get_column($column->field) or return; - my $id = $line->get_column($column->field); - my $text = $column->format_value(map { $line->get_column($_->field) } @{$column->curval_fields}); +{ my ($self, $column, $parent, $line) = @_; + my $field = $self->records->aggregate_name($column, undef, 'max'); + $line->get_column($field) or return; + my $id = $line->get_column($field); + my $text = $column->format_value(map { + $line->get_column($self->records->aggregate_name($_, $parent, 'max')) + } @{$column->curval_fields}); qq($text); } diff --git a/lib/GADS/Graph/Data.pm b/lib/GADS/Graph/Data.pm index 170ba1074..4351b1d6b 100644 --- a/lib/GADS/Graph/Data.pm +++ b/lib/GADS/Graph/Data.pm @@ -717,7 +717,8 @@ sub _records_to_results my @for = $self->x_axis_range && $self->x_axis_col->type eq 'date' ? $self->x_axis_col : @$x; foreach my $x (@for) { - my $col = $x_daterange ? $x->epoch : $x->field; + my $col = $x_daterange ? $x->epoch + : $self->records->aggregate_name($x, $self->records->layout->column($self->x_axis_link), 'max'); my $x_value = $line->get_column($col); $x_value ||= $line->get_column("${col}_link") if !$x_daterange && $x->link_parent; @@ -752,7 +753,7 @@ sub _records_to_results ? $x->field : $self->y_axis_stack eq 'count' ? 'id_count' # Don't use field count as NULLs are not counted - : $self->y_axis_col->field."_".$self->y_axis_stack; + : $self->records->aggregate_name($self->y_axis_col, $self->y_axis_link_col, $self->y_axis_stack); my $val = $line->get_column($fname); # Add on the linked column from another datasheet, if applicable @@ -803,7 +804,9 @@ sub _group_date sub _format_curcommon { my ($self, $column, $line) = @_; $line->get_column($column->field) or return; - $column->format_value(map { $line->get_column($_->field) } @{$column->curval_fields}); + $column->format_value(map { + $line->get_column($self->records->aggregate_name($_, $column, 'max')) + } @{$column->curval_fields}); } sub _to_percent diff --git a/lib/GADS/Layout.pm b/lib/GADS/Layout.pm index bff65f70c..f7cb0ce18 100644 --- a/lib/GADS/Layout.pm +++ b/lib/GADS/Layout.pm @@ -1313,9 +1313,9 @@ sub position sub column { my ($self, $id, %options) = @_; - $id or return; + $id or return undef; my $column = $self->use_layout->columns_index->{$id} - or return; # Column does not exist + or return undef; # Column does not exist return if $options{permission} && !$column->user_can($options{permission}); $column; } diff --git a/lib/GADS/Record.pm b/lib/GADS/Record.pm index e940d1575..6fd59695b 100644 --- a/lib/GADS/Record.pm +++ b/lib/GADS/Record.pm @@ -119,6 +119,12 @@ has record => ( clearer => 1, ); +# A reference to the GADS::Records that this record belongs to +has records => ( + is => 'rw', + weak_ref => 1, +); + # Subroutine to create a slightly more advanced predication for "record" above sub has_record { my $self = shift; @@ -1321,19 +1327,9 @@ sub _transform_values { next if $column->internal; my $key = $self->linked_id && $column->link_parent ? $column->link_parent->field : $column->field; - # If this value was retrieved as part of a grouping, and if it's a sum, - # then the field key will be appended with "_sum". XXX Ideally we'd - # have a better way of knowing this has happened, but this should - # suffice for the moment. if ($self->is_group) { - if ($column->numeric) - { - $key = $key."_sum"; - } - elsif (!$self->group_cols->{$column->id}) { - $key = $key."_distinct"; - } + $key = $self->records->aggregate_name($column); } my $value = $self->linked_id && $column->link_parent ? $original->{$key} : $original->{$key}; $fields->{$column->id} = $self->_create_datum($column, $value); diff --git a/lib/GADS/Records.pm b/lib/GADS/Records.pm index 944ba122f..ddf4e9e9b 100644 --- a/lib/GADS/Records.pm +++ b/lib/GADS/Records.pm @@ -1057,7 +1057,7 @@ sub _me_created_value sub _current_ids_rs { my $self = shift; - $self->_current_rs->get_column('me.id'); + $self->_current_rs(@_)->get_column('me.id'); } sub _current_rs @@ -1159,7 +1159,7 @@ sub _resultset_search # therefore performance (Pg at least) has been shown to be better if we run # the ID subquery first and only pass the IDs in to the main query $search{'me.id'} = $options{is_group} - ? { -in => $self->_current_ids_rs->as_query } + ? { -in => $self->_current_ids_rs(%options)->as_query } : $self->current_ids; # Rewind clause added only if needed @@ -2225,8 +2225,6 @@ sub order_by ? @{$self->_sorts_limit} : @{$self->_sorts}; - my $group_cols = delete $options{group_cols}; - my @order_by; my %has_time; foreach my $s (@sorts) { @@ -2297,18 +2295,11 @@ sub order_by my $query; if ($options{group}) { - # Assume that if the column is appearing in the group_by - # that it will already have been added as a grouped column - # and therefore there is no need to add the is_grouped flag - # here. This relies on calling this order_by function after - # the grouped columns have been added. - my $agg = $self->add_aggregate($col_sort, 'max', - parent => $column_parent, - group_cols => $group_cols, - sort => 1, - %options, - ); - $query = { $type => $agg->{as} }; + $self->add_aggregate($col_sort, 'max', sort => 1, parent => $column_parent); + $self->add_aggregate($col_sort->link_parent, 'max', sort => 1, linked => 1, parent => $column_parent) + if $column->link_parent; + my $aggname = $self->aggregate_name($col_sort, $column_parent); + $query = { $type => $aggname }; } else { $query = { $type => $sort_name }; @@ -3247,40 +3238,64 @@ sub _build_aggregate_results sub _build_group_results { my ($self, %options) = @_; - # Build the full query first, to ensure that all join numbers etc are - # calculated correctly - my $search_query = $self->search_query(search => 1, sort => 1); - # Work out the field name to select, and the appropriate aggregate function my @select_fields; my @cols; my $view = $self->view; - # During building grouped results an aggregate field can be added via - # sorting or as a selected column. This needs to be cleared each time, as - # group results can require different aggregate functions - $self->clear_aggregate_fields; - my $is_table_group = !$self->isa('GADS::RecordsGraph') && !$self->isa('GADS::RecordsGlobe'); + # Initial pass of all the columns to retrieve in the grouped sql query if ($options{columns}) { + # Supplied by the calling function @cols = @{$options{columns}}; } elsif ($view && $view->is_group && $is_table_group) { - my %view_group_cols = map { $_->layout_id => $_->parent_id } @{$view->groups}; - @cols = map { - +{ - id => $_->id, - column => $_, - operator => $_->numeric ? 'sum' : exists $view_group_cols{$_->id} ? 'max' : 'distinct', - group => exists $view_group_cols{$_->id}, - parent_id => $view_group_cols{$_->id}, + # Columns configured by a view. + # Construct a hash of all the group cols, along with any parents if + # applicable. + my %view_group_cols; + foreach my $group_col (@{$view->groups}) + { + $view_group_cols{$group_col->layout_id} ||= []; + push @{$view_group_cols{$group_col->layout_id}}, $group_col->parent_id; + } + # Convert all the columns in the view, into a format usable by this + # function + foreach my $col (@{$self->columns_selected}, @{$self->columns_recalc_extra}) + { + my %c = ( + id => $col->id, + column => $col, + ); + # Is the selected column a grouped-by column? + if (my $parents = $view_group_cols{$col->id}) + { + # Add it with its parent (if applicable), flagged as a group + # column + foreach my $parent_id (@$parents) + { + push @cols, { + %c, + operator => $col->numeric ? 'sum' : 'max', + group => 1, + parent_id => $parent_id, # May be undef + }; + } } - } @{$self->columns_selected}, @{$self->columns_recalc_extra}; + else { + # Otherwise push on as normal + push @cols, { + %c, + operator => $col->numeric ? 'sum' : 'distinct', + }; + } + } } else { + # Configured in the creation of the object @cols = @{$self->columns}; } @@ -3397,12 +3412,15 @@ sub _build_group_results next if $options{aggregate} && $column->aggregate && $column->aggregate eq 'recalc'; - $self->add_aggregate($column, $op, + my %common = ( + prefetch => 1, search => 0, parent => $parent, - group_cols => \@group_cols, is_grouped => $col->{group} || $col->{drcol}, ); + $self->add_aggregate($column, $op, %common); + $self->add_aggregate($column->link_parent, $op, %common, linked => 1) + if $column->link_parent; } push @select_fields, { @@ -3410,6 +3428,10 @@ sub _build_group_results -as => 'id_count', }; + # Build the full query first, to ensure that all join numbers etc are + # calculated correctly + my $search_query = $self->search_query(search => 1, sort => 1); + # If we want to aggregate by month, we need to do some tricky conditional # summing. We can't do this with the abstraction layer, so need to resort # to literal SQL @@ -3585,7 +3607,7 @@ sub _build_group_results else { $self->add_group($col); } - push @g, $self->fqvalue($col, group => 1, search => 0, prefetch => 1, retain_join_order => 1, parent => $_->{parent}); + push @g, $self->fqvalue($col, group => 1, search => 0, prefetch => 0, retain_join_order => 1, parent => $_->{parent}, aggregate => 1); } } }; @@ -3607,26 +3629,25 @@ sub _build_group_results # The "multivalue" parameter below removes any multi-value columns. my %common = ( group => 1, - prefetch => 1, # Search not needed as performed with subquery search => 0, sort => 1, drcol => $drcol, current_version_only => $self->cvo_values, + linked => 0, + rewind => $self->rewind_values, ); my @jp_fetch = $self->jpfetch(%common, - multivalue => 0, - linked => 0, retain_join_order => 1, - aggregate => $options{aggregate}, + aggregate => 1, ); - my $order_by = $self->order_by(%common, group_cols => \@group_cols, retain_join_order => 1); - push @select_fields, map $_->{select}, @{$self->aggregate_fields}; + my $order_by = $self->order_by(%common, linked => 1, retain_join_order => 1); + push @select_fields, map $_->{select}, $self->aggregate_fields(rewind => $self->rewind_values, group_cols => \@group_cols); my $select = { select => [@select_fields], join => [ - $self->linked_hash(%common, retain_join_order => 1, aggregate => $options{aggregate}), + $self->linked_hash(%common, prefetch => 1, retain_join_order => 1, aggregate => $options{aggregate}), { $self->cvo_values ? ('current_version' => \@jp_fetch) @@ -3642,10 +3663,9 @@ sub _build_group_results my $result = $self->schema->resultset('Current')->search( # Outer search query so needs to match values being retrieved $self->_resultset_search( - sort => 0, - is_group => 1, - prefetch => 1, - current_version_only => $self->cvo_values, + %common, + is_group => 1, + aggregate => 1, ), $select ); @@ -3661,6 +3681,7 @@ sub _build_group_results push @all, GADS::Record->new( schema => $self->schema, record => $rec, + records => $self, # is_group affects what key is used by GADS::Record for the result # (e.g. _sum). This is a bit messy and should be defined better. We # force is_group to be 1 if calculating total aggregates, which diff --git a/lib/GADS/RecordsJoin.pm b/lib/GADS/RecordsJoin.pm index cdfa7be87..c87b7fdea 100644 --- a/lib/GADS/RecordsJoin.pm +++ b/lib/GADS/RecordsJoin.pm @@ -105,6 +105,15 @@ sub _add_jp # Check whether join is already in store, if so update trace __x"Check to see if it's already in the store" if $debug; + if (my $parent = $options{parent}) + { + # Make sure that parent join is added, otherwise field on its own + # is useless + my %options2 = %options; + delete $options2{parent}; + $self->_add_jp($parent, %options2); + } + foreach my $j ($self->_all_joins_recurse(@{$self->_jp_store})) { trace __x"Checking join {field}", field => $j->{column}->field @@ -115,12 +124,13 @@ sub _add_jp if $debug; if ( _compare_parents($options{parent}, $j->{parent}) ) { - $j->{prefetch} ||= $prefetch; - $j->{search} ||= $options{search}; - $j->{linked} ||= $options{linked}; - $j->{sort} ||= $options{sort}; - $j->{group} ||= $options{group}; - $j->{drcol} ||= $options{drcol}; + $j->{prefetch} ||= $prefetch; + $j->{search} ||= $options{search}; + $j->{linked} ||= $options{linked}; + $j->{sort} ||= $options{sort}; + $j->{group} ||= $options{group}; + $j->{drcol} ||= $options{drcol}; + $j->{aggregate} ||= $options{aggregate}; if ($column->is_curcommon && $prefetch) { $self->_add_children($j, $column, %options); @@ -147,6 +157,7 @@ sub _add_jp drcol => $options{drcol}, # Whether it's used as a daterange column on a graph x-axis column => $column, parent => $options{parent}, + aggregate => $options{aggregate}, }; # If it's a curval field then we need to account for any joins that are @@ -214,6 +225,18 @@ sub add_linked_join $self->_add_jp(@_, linked => 1); } +sub add_aggregate +{ my ($self, $column, $operator, %options) = @_; + $self->_add_jp($column, aggregate => $operator, %options); + if ($column->is_curcommon) + { + # Add on all child fields if a curcommon field, so that the full value + # can be displayed (not just its ID) + $self->add_aggregate($_, 'max', %options, parent => $column) + foreach grep !$_->internal, @{$column->curval_fields_retrieve}; + } +} + sub has_linked { my ($self, %options) = @_; # Check all joins, regardless of options, as we still need to add a linked @@ -267,6 +290,7 @@ sub _count_version_joins ($options{search} && $_->{search} && $_->{parent} && !$is_curcommon) # Search in child of curval || ($options{sort} && $_->{sort}) # sort is all children || ($options{group} && $_->{group} && $_->{parent} && !$is_curcommon) + || ($options{aggregate} && $_->{aggregate} && $_->{parent} && !$is_curcommon) || ($options{drcol} && $_->{drcol}) # prefetch is all children, but not when the curval has no fields || ($options{prefetch} && $_->{prefetch} && !$_->{parent} && @{$_->{children}}) @@ -365,7 +389,14 @@ sub _jpfetch foreach (@jpstore2) { next if exists $options{prefetch} && !$options{prefetch} && $_->{prefetch} && !$options{group} && !$options{drcol}; - push @joins, $self->_jpfetch_add(options => \%options, join => $_,); + # Remove aggregate joins where the aggregate value is retrieved using a + # subquery instead. If it is also added as a join, not only is it + # unnecessary, it can also result in double-counting in totals. + # Note: this is the same condition as in get_aggregate(), in terms of + # whether to create a subquery or return the direct field selection. + next + if $options{aggregate} && !$_->{group} && ($_->{column}->multivalue || ($_->{parent} && $_->{parent}->multivalue)); + push @joins, $self->_jpfetch_add(options => \%options, join => $_); } my @return; if ($options{limit} && @joins) @@ -410,6 +441,7 @@ sub _jpfetch_add || ($options->{sort} && $join->{sort}) || ($options->{group} && $join->{group}) || ($options->{drcol} && $join->{drcol}) + || ($options->{aggregate} && $join->{aggregate}) || ( # Include only aggregate columns if requested. This is used when a # records object has been built, but then only the aggregate columns @@ -440,6 +472,7 @@ sub _jpfetch_add || $options->{include_multivalue} || $_->{group} || $_->{drcol} + || $_->{aggregate} } @children if $options->{prefetch}; my $options = { @@ -797,20 +830,101 @@ sub fqvalue "$tn." . $value_field; } -has aggregate_fields => ( - is => 'ro', - lazy => 1, - clearer => 1, - builder => sub { [] }, -); +sub aggregate_fields +{ my ($self, %options) = @_; -sub add_aggregate + my @aggregate_fields; + + # Don't use _jpfetch() here as we need to look for all aggregate fields + # regardless of conditions. This is because we include aggregate fields + # that are rendered as subqueries which are otherwise completely + # independent from the overall query. + foreach my $jp (grep $_->{aggregate}, @{$self->_jp_store}) + { + my %opt = ( + is_grouped => $jp->{group}, + parent => $jp->{parent}, + group_cols => $options{group_cols}, + rewind => $options{rewind}, + ); + + my $column = $jp->{column}; + my $operator = $jp->{aggregate}; + push @aggregate_fields, $self->get_aggregate($column, $operator, %opt); + + # Also add linked column if required + push @aggregate_fields, $self->get_aggregate($column->link_parent, $operator, %opt, + is_linked => $column, + ) if $column->link_parent; + + if ($jp->{children} && @{$jp->{children}}) + { + foreach my $child (@{$jp->{children}}) + { + my $column2 = $child->{column}; + push @aggregate_fields, $self->get_aggregate($column2, $child->{aggregate}, %opt, parent => $column) + if $child->{aggregate}; + } + } + } + + @aggregate_fields; +} + +# Function to return the name of an aggregate field in the sql query. This is +# constructed of the parent ID (if applicable), then the field ID itself, then +# the aggregate function. +sub aggregate_name +{ my ($self, $column, $parent, $operator) = @_; + my $name = $column->field; + $name = $parent->field."_".$name + if $parent; + # It's optional to supply an aggregate operator, in which case the one + # configured in the stored field is used. + if (!$operator) + { + my $internal_col; + # Need to search the whole store, as using a higher-level function will + # normally exclude some fields. This is because an aggregate name is + # needed even if the field is never part of the main query (e.g. + # subquery, in the same way as the aggregate_fields() function) + foreach my $j (@{$self->_jp_store}) + { + if ($column->id == $j->{column}->id) + { + $internal_col = $j; + } + elsif ($parent && $parent->id == $j->{column}->id) + { + foreach my $child (@{$j->{children}}) + { + $internal_col = $child->{column} + if $child->{column}->id == $column->id; + } + } + } + $operator = $internal_col->{aggregate} + if $internal_col; + } + # Operator is not added for "max" (which is just the value itself, not + # normally an actual aggregate operator) + $name .= "_".$operator + if $operator && $operator ne 'max'; + $name; +} + +sub get_aggregate { my ($self, $column, $operator, %options) = @_; + $operator or panic "Missing operator"; + # Througout the course of this function, the operator used in the query may + # be different to the operator required. For example, if a subquery is + # used, the actual operator may be in the subquery, but the value of the + # subquery retrieved using "max" (so as to not to double-count). + my $original_operator = $operator; + # Whether this column appears in the group_by statement, therefore meaning - # it does not require a separate sql select statement. This can possibly be - # removed in the future by checking the fields that have been added as - # grouped. + # it does not require a separate sql select statement. my $is_grouped = delete $options{is_grouped}; my $is_linked = delete $options{is_linked}; # Whether there are any group_by statements at all in the planned sql query @@ -819,38 +933,23 @@ sub add_aggregate my $as_index = $self->group_values_as_index; my $drcol = !!$self->dr_column; - my ($existing) = grep { - $_->{column}->id == $column->id - && ( - (!$_->{parent} && !$parent) - || ($_->{parent} && $parent && $_->{parent}->id == $parent->id) - ) - && $_->{operator} ne $operator - } @{$self->aggregate_fields}; - - return $existing if $existing; - my $select; - my $as = $is_linked ? $is_linked->field : $column->field; - $as = $as.'_count' if $operator eq 'count'; - $as = $as.'_sum' if $operator eq 'sum'; - $as = $as.'_distinct' if $operator eq 'distinct' && !$is_grouped; - $as = $as.'_link' if $is_linked; # The select statement to get this column's value varies depending on # what we want to retrieve. If we're selecting a field with multiple # values, then we have to run this as a separate subquery, otherwise if # there are more than one multiple-value retrieval then that aggregates # will be counting multiple times for each set of multiple values (due - # to the multiple joins) + # to the multiple joins). - # Field is either multivalue or its parent is if (($column->multivalue || ($parent && $parent->multivalue)) && !$is_grouped) { - # Assume curval if it's a parent - we need to search the curval - # table for all the curvals that are part of the records retrieved. + # Field is either multivalue or its parent is: construct a subquery + # rather than selecting from the main query. if ($parent) { + # Assume curval if it's a parent - we need to search the curval + # table for all the curvals that are part of the records retrieved. my $f_rs = $self->schema->resultset('Curval')->search({ 'mecurval.record_id' => { # Match against main query's records (use cvo_values as @@ -863,7 +962,7 @@ sub add_aggregate # If retrieving from a previous point in time then add # required search criteria to only retrieve single correct # record - $self->cvo_values ? () : ('record_later.id' => undef), + $self->cvo_values ? () : ('record_later_alternative.id' => undef), $self->rewind_values ? ('record_single_alternative.created' => { '<=' => $self->dt_parser->format_datetime($self->rewind_values) }) : () @@ -877,7 +976,7 @@ sub add_aggregate $column->tjoin(join_current_version => 1) ]) : ('record_single_alternative' => [ - 'record_later', + 'record_later_alternative', $column->tjoin, ]) }, @@ -914,54 +1013,53 @@ sub add_aggregate } # Otherwise a standard subquery select for that type of field else { - # Also need to add the main search query, otherwise if we take - # all the field's values for each record, then we won't be - # filtering the non-matched ones in the case of multivalue - # fields. - # Need to include "group" as an option to the subquery, to - # ensure that the grouping column is added to match to the main - # query's group column. This does not apply if doing an overall - # aggregate though, as there is only a need to retrieve the - # overall results, not for each matching grouped row. If the - # "group" option is included unnecessarily, then this can cause - # joins of multiple-value fields which can include too many - # results in the aggregate. + # For the subquery, we also need to add the main search query, + # otherwise if we take all the field's values for each record, then + # we won't be filtering the non-matched ones in the case of + # multivalue fields. my $has_grouped = @group_cols; - my $searchq = $self->search_query(%options, - search => 1, - extra_column => $column, - linked => 0, - group => $has_grouped, + # First start with only records in the current overall query. Add + # on the various parts of the query that are normally required for + # this type of query. + my $searchq = [{ + 'mefield.id' => { -in => $self->_current_ids_rs->as_query }, + }]; + push @$searchq, $self->record_later_search( alt => 1, - alias => 'mefield', current_version_only => $self->cvo_values, - rewind => $self->rewind_values, + ); + push @$searchq, {$self->record_single_rewind(alt => 1, rewind => $options{rewind})} + if $options{rewind}; + # Now add onto the query additional filters to only match the + # relevant group that it is a part of (one aggregate field can have + # multiple rows in the resultant query, one for each grouped + # column). + my %common = ( + search => 0, + sort => 0, + prefetch => 0, + aggregate => 0, + current_version_only => $self->cvo_values, + group => $has_grouped, + # Overridden if required + linked => 0, + drcol => $drcol, ); foreach my $group (@group_cols) { push @$searchq, { $self->fqvalue($group->{column}, - %options, - search => 1, - as_index => $as_index, - linked => 0, - group => 1, - alt => 1, - extra_column => $group->{column}, - parent => $group->{parent}, - drcol => $drcol, - current_version_only => $self->cvo_values + %common, + as_index => $as_index, + alt => 1, + extra_column => $group->{column}, + parent => $group->{parent}, ) => { -ident => $self->fqvalue($group->{column}, - %options, - search => 1, - parent => $group->{parent}, - as_index => $as_index, - linked => 0, - group => 1, - extra_column => $group->{column}, - drcol => $drcol, - current_version_only => $self->cvo_values, + %common, + parent => $group->{parent}, + as_index => $as_index, + extra_column => $group->{column}, ) }, }; @@ -971,64 +1069,46 @@ sub add_aggregate { alias => 'mefield', join => [ - [$self->linked_hash(%options, - search => 1, - group => $has_grouped, - alt => 1, - extra_column => $column, - current_version_only => $self->cvo_values, + [$self->linked_hash( + %common, + alt => 1, + extra_column => $column, )], { $self->cvo_values ? ('current_version_alternative' => [ - $self->jpfetch(%options, - search => 1, - linked => 0, - group => $has_grouped, - extra_column => $column, - alt => 1, - current_version_only => 1, + $self->jpfetch( + %common, + extra_column => $column, + alt => 1, ) ]) : ('record_single_alternative' => [ # The (assumed) single record for the required version of current 'record_later_alternative', # The record after the single record (undef when single is latest) - $self->jpfetch(%options, - search => 1, - linked => 0, - group => $has_grouped, - extra_column => $column, - alt => 1, - current_version_only => 0, + $self->jpfetch( + %common, + extra_column => $column, + alt => 1, ), ]) }, ], select => { count => { distinct => $self->fqvalue($column, - %options, - search => 1, - as_index => $as_index, - linked => 0, - group => 1, - alt => 1, - extra_column => $column, - drcol => $drcol, - current_version_only => $self->cvo_values, + %common, + as_index => $as_index, + alt => 1, + extra_column => $column, )}, -as => 'sub_query_as', }, }, ); my $col_fq = $self->fqvalue($column, - %options, - search => 1, - as_index => $as_index, - linked => 0, - group => 1, - alt => 1, - extra_column => $column, - drcol => $drcol, - current_version_only => $self->cvo_values, + %common, + as_index => $as_index, + alt => 1, + extra_column => $column, ); if ($column->numeric && $operator eq 'sum') { @@ -1054,11 +1134,11 @@ sub add_aggregate # Standard single-value field - select directly, no need for a subquery else { $select = $self->fqvalue($column, - %options, + aggregate => 1, + rewind => $options{rewind}, as_index => $as_index, - prefetch => 1, group => 1, - linked => 0, + linked => !!$is_linked, parent => $parent, retain_join_order => 1, drcol => $drcol, @@ -1066,6 +1146,10 @@ sub add_aggregate ); } + my $as = $is_linked ? $self->aggregate_name($is_linked, undef, $original_operator) + : $self->aggregate_name($column, $parent, $original_operator); + $as = $as.'_link' if $is_linked; + my $overall_select = $operator eq 'distinct' ? { count => { distinct => $select }, @@ -1076,7 +1160,7 @@ sub add_aggregate -as => $as, }; - my $aggfield = { + return { column => $column, parent => $parent, select => $overall_select, @@ -1084,17 +1168,6 @@ sub add_aggregate as => $as, }; - push @{$self->aggregate_fields}, $aggfield; - - # Also add linked column if required - $self->add_aggregate($column->link_parent, $operator, - is_linked => $column, - parent => $parent, - group_cols => \@group_cols, - is_grouped => $is_grouped - ) if $column->link_parent; - - $aggfield; } sub _dump_child @@ -1112,15 +1185,16 @@ sub _dump_child my $parent_id = $child->{parent}->id; my $ret = " child is ".$child->{column}->id." (".$child->{column}->name.") => { - join => $join, - prefetch => $child->{prefetch}, - curval => $child->{curval}, - search => $child->{search}, - sort => $child->{sort}, - group => $child->{group}, - drcol => $child->{drcol}, - parent => $parent_id, - children => $children + join => $join, + prefetch => $child->{prefetch}, + aggregate => $child->{aggregate}, + curval => $child->{curval}, + search => $child->{search}, + sort => $child->{sort}, + group => $child->{group}, + drcol => $child->{drcol}, + parent => $parent_id, + children => $children },"; my $space = ' ' x $indent; $ret =~ s/^(.*)$/$space$1/mg; @@ -1146,15 +1220,16 @@ sub _dump_jp_store my $join = ref $jp->{join} ? $dd->Dump : $jp->{join}; chomp $join; $dumped .= " join for ".$jp->{column}->id." (".$jp->{column}->name.") => { - join => $join, - prefetch => $jp->{prefetch}, - search => $jp->{search}, - linked => $jp->{linked}, - sort => $jp->{sort}, - group => $jp->{group}, - drcol => $jp->{drcol}, - curval => $jp->{curval}, - children => $children + join => $join, + prefetch => $jp->{prefetch}, + aggregate => $jp->{aggregate}, + search => $jp->{search}, + linked => $jp->{linked}, + sort => $jp->{sort}, + group => $jp->{group}, + drcol => $jp->{drcol}, + curval => $jp->{curval}, + children => $children }, "; } diff --git a/t/004_aggregate.t b/t/004_aggregate.t index dfcefa1c2..cb8ee59ae 100644 --- a/t/004_aggregate.t +++ b/t/004_aggregate.t @@ -2,161 +2,200 @@ use Test::More; # tests => 1; use strict; use warnings; +use GADS::Filter; use GADS::Records; use Log::Report; use lib 't/lib'; use Test::GADS::DataSheet; -my $data = [ - { - string1 => 'foo1', - integer1 => 25, - integer2 => 50, - enum1 => [1,2], - }, - { - string1 => 'foo1', - integer1 => 50, - integer2 => 500, - enum1 => 2, - }, - { - string1 => 'foo2', - integer1 => 60, - integer2 => 60, - enum1 => 2, - }, - { - string1 => 'foo2', - integer1 => 70, - integer2 => 35, - enum1 => 3, - }, -]; - -my $sheet = Test::GADS::DataSheet->new( - data => $data, - multivalue => 1, - calc_code => "function evaluate (L1integer1, L1integer2) \n return (L1integer1[1] / L1integer2[1]) * 100 \n end", - column_count => { integer => 2 }, -); -my $schema = $sheet->schema; -my $layout = $sheet->layout; -$sheet->create_records; -my $columns = $sheet->columns; - -my $string1 = $columns->{string1}; -my $integer1 = $columns->{integer1}; -my $integer2 = $columns->{integer2}; -my $calc1 = $columns->{calc1}; -my $enum1 = $columns->{enum1}; - -my $view = GADS::View->new( - name => 'Aggregate view', - columns => [$string1->id, $integer1->id, $calc1->id, $enum1->id], - instance_id => $layout->instance_id, - layout => $layout, - schema => $schema, - user => $sheet->user, -); -$view->write; - -my $records = GADS::Records->new( - view => $view, - layout => $layout, - user => $sheet->user, - schema => $schema, -); - -my @results = @{$records->results}; -is(@results, 4, "Correct number of normal rows"); - -is($records->aggregate_results, undef, "No aggregate results initially"); - -$integer1->aggregate('sum'); -$integer1->write; -$calc1->aggregate('sum'); -$calc1->write; -$layout->clear; - -$records = GADS::Records->new( - view => $view, - layout => $layout, - user => $sheet->user, - schema => $schema, -); - -@results = @{$records->results}; -is(@results, 4, "Correct number of normal rows"); - -my $aggregate = $records->aggregate_results; - -is($aggregate->fields->{$integer1->id}->as_string, "205", "Correct total of integer values"); -is($aggregate->fields->{$calc1->id}->as_string, "360", "Correct total of calc values"); - -# Test of recalc aggregate type, whereby calc values are recalculated based on -# other aggregate fields. Do not include all required columns in the view - -# this should still work +foreach my $multivalue (0, 1) { - $calc1->aggregate('recalc'); - try { $calc1->write }; - my $e = qr/column integer2 does not have an aggregate defined/; - like($@, $e, "Cannot set recalc without all required aggregate fields"); - $integer2->aggregate('sum'); - $integer2->write; - $layout->clear; - $calc1->aggregate('recalc'); - $calc1->write; - $layout->clear; - $integer2->aggregate(''); - try { $integer2->write }; - $e = qr/aggregate on this column cannot be removed/; - like($@, $e, "Cannot remove aggregate with recalc in effect"); + my $data = [ + { + string1 => 'foo1', + integer1 => 25, + integer2 => 50, + enum1 => $multivalue ? [1,2] : 1, + person1 => 1, + }, + { + string1 => 'foo1', + integer1 => 50, + integer2 => 500, + enum1 => 2, + person1 => 1, + }, + { + string1 => 'foo2', + integer1 => 60, + integer2 => 60, + enum1 => 2, + person1 => 1, + }, + { + string1 => 'foo2', + integer1 => 70, + integer2 => 35, + enum1 => 3, + person1 => 1, + }, + ]; + + my $sheet = Test::GADS::DataSheet->new( + data => $data, + multivalue => $multivalue, + calc_code => $multivalue + ? "function evaluate (L1integer1, L1integer2) \n return (L1integer1[1] / L1integer2[1]) * 100 \n end" + : "function evaluate (L1integer1, L1integer2) \n return (L1integer1 / L1integer2) * 100 \n end", + column_count => { integer => 2 }, + ); + my $schema = $sheet->schema; + my $layout = $sheet->layout; + $sheet->create_records; + my $columns = $sheet->columns; - $records = GADS::Records->new( - view => $view, + my $string1 = $columns->{string1}; + my $integer1 = $columns->{integer1}; + my $integer2 = $columns->{integer2}; + my $calc1 = $columns->{calc1}; + my $enum1 = $columns->{enum1}; + my $person1 = $columns->{person1}; + + my $records = GADS::Records->new( layout => $layout, user => $sheet->user, schema => $schema, ); - @results = @{$records->results}; + my @results = @{$records->results}; is(@results, 4, "Correct number of normal rows"); - my $aggregate = $records->aggregate_results; - - is($aggregate->fields->{$integer1->id}->as_string, "205", "Correct total of integer values"); - is($aggregate->fields->{$calc1->id}->as_string, "32", "Correct total of calc values"); + is($records->aggregate_results, undef, "No aggregate results initially"); + $integer1->aggregate('sum'); + $integer1->write; $calc1->aggregate('sum'); $calc1->write; $layout->clear; -} - -# Perform test for multivalue field within set of records that will be -# aggregated, where the multivalue field is grouped. This checks for -# double-counting of rows, which we do want for each group, but not for the -# total aggregate -{ - $view->set_groups([$enum1->id]); - $view->write; - $records->clear; - @results = @{$records->results}; - is(@results, 3, "Correct number of normal rows"); - - # The sum of the groups adds up to more than the total aggregate. This is - # because one record appears in multiple groups, but is only counted once - # for the overall aggregate - is($results[0]->fields->{$integer1->id}->as_string, 25, "First grouping correct"); - is($results[1]->fields->{$integer1->id}->as_string, 135, "Second grouping correct"); - is($results[2]->fields->{$integer1->id}->as_string, 70, "Third grouping correct"); - - my $aggregate = $records->aggregate_results; - - is($aggregate->fields->{$integer1->id}->as_string, "205", "Correct total of integer values"); - is($aggregate->fields->{$calc1->id}->as_string, "360", "Correct total of calc values"); + # Perform tests with and without filters + foreach my $with_filter (0, 1) + { + my $rules = GADS::Filter->new( + as_hash => { + condition => 'OR', + rules => [ + { + id => $enum1->id, + type => 'string', + value => 'foo1', + operator => 'equal', + }, + { + id => $enum1->id, + type => 'string', + value => 'foo2', + operator => 'equal', + }, + ], + }, + ); + my $view = GADS::View->new( + name => 'Aggregate view', + columns => [$string1->id, $integer1->id, $calc1->id, $person1->id], + filter => $with_filter && $rules, + instance_id => $layout->instance_id, + layout => $layout, + schema => $schema, + user => $sheet->user, + ); + $view->set_sorts({fields => [$enum1->id], types => ['asc']}); + $view->write; + + my $records = GADS::Records->new( + view => $view, + layout => $layout, + user => $sheet->user, + schema => $schema, + ); + + @results = @{$records->results}; + is(@results, $with_filter ? 3 : 4, "Correct number of normal rows"); + + my $aggregate = $records->aggregate_results; + + is($aggregate->fields->{$integer1->id}->as_string, $with_filter ? 135 : 205, "Correct total of integer values"); + is($aggregate->fields->{$calc1->id}->as_string, $with_filter ? 160 : 360, "Correct total of calc values"); + + # Test of recalc aggregate type, whereby calc values are recalculated based on + # other aggregate fields. Do not include all required columns in the view - + # this should still work + { + $calc1->aggregate('recalc'); + try { $calc1->write }; + my $e = qr/column integer2 does not have an aggregate defined/; + like($@, $e, "Cannot set recalc without all required aggregate fields"); + $integer2->aggregate('sum'); + $integer2->write; + $layout->clear; + $calc1->aggregate('recalc'); + $calc1->write; + $layout->clear; + $integer2->aggregate(''); + try { $integer2->write }; + $e = qr/aggregate on this column cannot be removed/; + like($@, $e, "Cannot remove aggregate with recalc in effect"); + + $records = GADS::Records->new( + view => $view, + layout => $layout, + user => $sheet->user, + schema => $schema, + ); + + @results = @{$records->results}; + is(@results, $with_filter ? 3 : 4, "Correct number of normal rows"); + + my $aggregate = $records->aggregate_results; + + is($aggregate->fields->{$integer1->id}->as_string, $with_filter ? 135 : 205, "Correct total of integer values"); + is($aggregate->fields->{$integer2->id}->as_string, $with_filter ? 610 : 645, "Correct total of integer values"); + is($aggregate->fields->{$calc1->id}->as_string, $with_filter ? 22 : 32, "Correct total of calc values"); + + $calc1->aggregate('sum'); + $calc1->write; + $integer2->aggregate(''); + $integer2->write; + $layout->clear; + } + + # Perform test for multivalue field within set of records that will be + # aggregated, where the multivalue field is grouped. This checks for + # double-counting of rows, which we do want for each group, but not for the + # total aggregate + { + $view->set_groups([$enum1->id]); + $view->write; + $records->clear; + + @results = @{$records->results}; + is(@results, $with_filter ? 2 : 3, "Correct number of normal rows"); + + # For the multivalue, the sum of the groups adds up to more than the + # total aggregate. This is because one record appears in multiple + # groups, but is only counted once for the overall aggregate + is($results[0]->fields->{$integer1->id}->as_string, 25, "First grouping correct"); + is($results[1]->fields->{$integer1->id}->as_string, $multivalue ? 135 : 110, "Second grouping correct"); + is($results[2]->fields->{$integer1->id}->as_string, $with_filter ? 0 : 70, "Third grouping correct") + if !$with_filter; + + my $aggregate = $records->aggregate_results; + + is($aggregate->fields->{$integer1->id}->as_string, $with_filter ? 135 : 205, "Correct total of integer values"); + is($aggregate->fields->{$calc1->id}->as_string, $with_filter ? 160 : 360, "Correct total of calc values"); + } + } } # Large number of records (greater than default number of rows in table). Check @@ -200,7 +239,7 @@ is($aggregate->fields->{$calc1->id}->as_string, "360", "Correct total of calc va schema => $schema, ); - @results = @{$records->results}; + my @results = @{$records->results}; is(@results, 50, "Correct number of normal rows"); is($records->pages, 6, "Correct number of pages for large number of records"); diff --git a/t/004_group.t b/t/004_group.t index 4813046c0..025a73db3 100644 --- a/t/004_group.t +++ b/t/004_group.t @@ -13,7 +13,7 @@ foreach my $multivalue (0..1) # It doesn't make a lot of sense to test a lot of these values, as the grouping # of text fields is not really possible (instead, the max value is used). # However, add them to the tests, to check that if a user does add them to a - # grouping view that something unexpected doesn't happen + # grouping view that something unexpected doesn't happen. my $data = [ { string1 => 'foo1', @@ -23,6 +23,7 @@ foreach my $multivalue (0..1) enum1 => 8, tree1 => 12, curval1 => 1, + curval2 => 2, # Bar person1 => 1, }, { @@ -33,6 +34,7 @@ foreach my $multivalue (0..1) enum1 => $multivalue ? [7,9] : 7, tree1 => 12, curval1 => 1, + curval2 => 2, person1 => 1, }, { @@ -43,6 +45,7 @@ foreach my $multivalue (0..1) enum1 => 8, tree1 => 11, curval1 => 2, + curval2 => 1, # Foo person1 => 1, }, { @@ -53,6 +56,7 @@ foreach my $multivalue (0..1) enum1 => 8, tree1 => 11, curval1 => 2, + curval2 => 1, person1 => 1, }, ]; @@ -67,6 +71,7 @@ foreach my $multivalue (0..1) enum1 => $multivalue ? '3 unique' : '2 unique', tree1 => '1 unique', curval1 => '1 unique', + curval2 => '1 unique', }, { string1 => 'foo2', @@ -77,6 +82,7 @@ foreach my $multivalue (0..1) enum1 => '1 unique', tree1 => '1 unique', curval1 => '1 unique', + curval2 => '1 unique', }, ]; @@ -93,6 +99,8 @@ foreach my $multivalue (0..1) schema => $schema, curval => 2, curval_field_ids => [$curval_sheet->columns->{string1}->id], + # Add 2 curvals to test child fields of the same parent + column_count => { curval => 2}, multivalue => $multivalue, user_permission_override => 0, ); @@ -123,10 +131,12 @@ foreach my $multivalue (0..1) my $enum1 = $columns->{enum1}; my $tree1 = $columns->{tree1}; my $curval1 = $columns->{curval1}; + my $curval2 = $columns->{curval2}; my $view = GADS::View->new( name => 'Group view', - columns => [$string1->id, $integer1->id, $calc1->id, $date1->id, $daterange1->id, $enum1->id, $tree1->id, $curval1->id], + curval1 => 'Bar', + columns => [$string1->id, $integer1->id, $calc1->id, $date1->id, $daterange1->id, $enum1->id, $tree1->id, $curval1->id, $curval2->id], instance_id => $layout->instance_id, layout => $layout, schema => $schema, @@ -161,6 +171,7 @@ foreach my $multivalue (0..1) is($row->fields->{$enum1->id}, $expected->{enum1}, "Group enum correct"); is($row->fields->{$tree1->id}, $expected->{tree1}, "Group tree correct"); is($row->fields->{$curval1->id}, $expected->{curval1}, "Group curval correct"); + is($row->fields->{$curval2->id}, $expected->{curval2}, "Second group curval correct"); is($row->id_count, 2, "ID count correct"); } @@ -220,6 +231,7 @@ foreach my $multivalue (0..1) rag1 => 'b_red', calc1 => 50, curval1 => 'Bar', + curval2 => 'Bar', daterange1 => '2000-01-02 to 2001-03-03', person1 => 'User1, User1', }; @@ -297,14 +309,14 @@ foreach my $multivalue (0..1) # Test curval (subfield) $view = GADS::View->new( name => 'Group view curval subfield', - columns => [$integer1->id], + columns => [$curval1->id, $integer1->id], instance_id => $layout->instance_id, layout => $layout, schema => $schema, user => $sheet->user, ); - $view->set_sorts({fields => [$columns->{curval1}->id."_".$curval_sheet->columns->{string1}->id], types => ['desc']}); - $view->set_groups([$columns->{curval1}->id."_".$curval_sheet->columns->{string1}->id]); + $view->set_sorts({fields => [$columns->{curval2}->id."_".$curval_sheet->columns->{string1}->id], types => ['desc']}); + $view->set_groups([$columns->{curval1}->id."_".$curval_sheet->columns->{string1}->id, $columns->{curval2}->id."_".$curval_sheet->columns->{string1}->id]); $view->write; $records = GADS::Records->new( @@ -315,8 +327,8 @@ foreach my $multivalue (0..1) ); @results = @{$records->results}; is(@results, 2, "Correct number of rows for group by curval subfield"); - is($results[0]->fields->{$integer1->id}, '75', "Group by curval subfield first result correct"); - is($results[1]->fields->{$integer1->id}, '130', "Group by curval subfield second result correct"); + is($results[0]->fields->{$integer1->id}, '130', "Group by curval subfield first result correct"); + is($results[1]->fields->{$integer1->id}, '75', "Group by curval subfield second result correct"); # Try also sorting directly on the Records object # First with invalid subfield without parent - should error @@ -338,7 +350,7 @@ foreach my $multivalue (0..1) sort => { type => 'desc', id => $curval_sheet->columns->{string1}->id, - parent_id => $columns->{curval1}->id, + parent_id => $columns->{curval2}->id, }, view => $view, layout => $layout, @@ -347,8 +359,8 @@ foreach my $multivalue (0..1) ); @results = @{$records->results}; is(@results, 2, "Correct number of rows for group by curval subfield"); - is($results[0]->fields->{$integer1->id}, '75', "Group by curval subfield first result correct"); - is($results[1]->fields->{$integer1->id}, '130', "Group by curval subfield second result correct"); + is($results[0]->fields->{$integer1->id}, '130', "Group by curval subfield first result correct"); + is($results[1]->fields->{$integer1->id}, '75', "Group by curval subfield second result correct"); } diff --git a/t/004_rewind.t b/t/004_rewind.t index 25b53d13f..394e068bf 100644 --- a/t/004_rewind.t +++ b/t/004_rewind.t @@ -72,6 +72,9 @@ foreach my $multivalue (0..3) my $integer1 = $sheet->columns->{integer1}; my $curval1 = $sheet->columns->{curval1}; + $integer1->aggregate('sum'); + $integer1->write; + my $records = GADS::Records->new( user => $sheet->user, layout => $layout, @@ -136,6 +139,9 @@ foreach my $multivalue (0..3) is($record->fields->{$string1->id}->as_string, 'Foo1', "Correct old value for first record (2014)"); is($record->fields->{$curval1->id}->as_string, 'Bar1', "Correct old value for first record (2014)"); + my $aggregate = $records->aggregate_results; + is($aggregate->fields->{$integer1->id}->as_string, 10, "Correct aggregate value 2014"); + # Go back to second set (2015) $previous->add(years => 1); $records = GADS::Records->new( @@ -149,6 +155,9 @@ foreach my $multivalue (0..3) is($record->fields->{$string1->id}->as_string, 'Foo2', "Correct old value for first record (2015)"); is($record->fields->{$curval1->id}->as_string, 'Bar2', "Correct old value for first record (2015)"); + $aggregate = $records->aggregate_results; + is($aggregate->fields->{$integer1->id}->as_string, 20, "Correct aggregate value 2015"); + # And back to today $records = GADS::Records->new( user => $sheet->user, @@ -160,6 +169,9 @@ foreach my $multivalue (0..3) is($record->fields->{$string1->id}->as_string, 'Foo3', "Correct value for first record current date"); is($record->fields->{$curval1->id}->as_string, 'Bar3', "Correct value for first record current date"); + $aggregate = $records->aggregate_results; + is($aggregate->fields->{$integer1->id}->as_string, 130, "Correct aggregate value 2015"); + # Retrieve single record $record = GADS::Record->new( user => $sheet->user, diff --git a/t/012_graphs.t b/t/012_graphs.t index 06d69d8b0..f9c1584a3 100644 --- a/t/012_graphs.t +++ b/t/012_graphs.t @@ -174,7 +174,7 @@ foreach my $multivalue (0..1) x_axis => $columns->{string1}->id, y_axis => $calc2->id, #$columns->{calc2}->id, y_axis_stack => 'sum', - data => [[ 72, 26 ]], + data => [[ 72, 39 ]], xlabels => [qw/Bar FooBar/], rules => [ {