Skip to content

Fix group and aggregate selections (#1639) #649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abeverley wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Fix group and aggregate selections (#1639) #649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2f08bf
Fix group and aggregate selections (#1639)
abeverley Aug 29, 2026
a39443d
Fix historic aggregate with curval in view
abeverley Aug 30, 2026
8540ec0
Fix aggregate over multiple pages
abeverley Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions lib/GADS/Globe.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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 ||= '<blank>';
}

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) || '<blank>';
}

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;
Expand Down Expand Up @@ -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(<a href="/record/$id">$text</a>);
}

Expand Down
9 changes: 6 additions & 3 deletions lib/GADS/Graph/Data.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/GADS/Layout.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
18 changes: 7 additions & 11 deletions lib/GADS/Record.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
117 changes: 69 additions & 48 deletions lib/GADS/Records.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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 };
Expand Down Expand Up @@ -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};
}

Expand Down Expand Up @@ -3397,19 +3412,26 @@ 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, {
count => \1,
-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
Expand Down Expand Up @@ -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);
}
}
};
Expand All @@ -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)
Expand All @@ -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
);

Expand All @@ -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
Expand Down
Loading
Loading