Performance - link latest record version (D1098) #595
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit improves performance by linking the current version of a record to its main stub, thereby making retrieval faster for records with lots of versions. The version retrieval functionality is retained for the purpose of historical view, which operates as it previously did. This commit also includes the addition of curval subfields to the y-axis of graphs (back-end only) which was needed to test some of the changes. It also includes sorting for grouping, which was needed for keeping the grouping tests working as before.
Contributor
droberts-ctrlo
left a comment
droberts-ctrlo
left a comment
There was a problem hiding this comment.
Aside from a few formatting issues, all appears fine.
| # Perform search construct twice, to ensure all value joins are consistent numbers | ||
| $self->_search_construct($decoded, ignore_perms => 1, user => $user); | ||
| push @searches, $self->_search_construct($decoded, ignore_perms => 1, user => $user); | ||
| $self->_search_construct($decoded, ignore_perms => 1, user => $user, current_version_only => 1); |
Contributor
There was a problem hiding this comment.
Should this be multiline to make it easier to read (and the line below)?
| else { | ||
| push @search, { 'layout.id' => \@columns_can_view }; | ||
| push @search, $self->record_later_search(search => 1); | ||
| push @search, $self->record_later_search(search => 1, current_version_only => $self->cvo_recordset); |
| my $search_query = $self->search_query(search => 1, sort => 1, linked => 1, %limit); # Need to call first to build joins | ||
| my @prefetches = $self->jpfetch(prefetch => 1, linked => 0, %limit); | ||
| my @linked_prefetch = $self->linked_hash(prefetch => 1, %limit); | ||
| my $search_query = $self->search_query(linked => 1, rewind => $self->rewind_recordset, %common); # Need to call first to build joins |
| $search_query = $self->search_query(search => 1, linked => 1); | ||
| my @joins = $self->jpfetch(search => 1, linked => 0); | ||
| my @linked = $self->linked_hash(search => 1, linked => 1); | ||
| $search_query = $self->search_query(search => 1, linked => 1, current_version_only => $self->cvo_values, rewind => $self->rewind_values); |
| else { | ||
| # record joins not needed, remove with fresh call | ||
| $search_query = $self->search_query(search => 1, linked => 1, no_record_later => 1); | ||
| $search_query = $self->search_query(search => 1, linked => 1, no_record_later => 1, current_version_only => $self->cvo_values, rewind => $self->rewind_values); |
| { | ||
| $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) |
| }, | ||
| ], | ||
| 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) }, |
| }, | ||
| }, | ||
| ); | ||
| 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); |
| } | ||
| # Standard single-value field - select directly, no need for a subquery | ||
| else { | ||
| $select = $self->fqvalue($column, %options, as_index => $as_index, prefetch => 1, group => 1, linked => 0, parent => $parent, retain_join_order => 1, drcol => $drcol, current_version_only => $self->cvo_values); |
| 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) |
pawelwlod
reviewed
Jan 12, 2026
| my $join = $jp->{column}->is_curcommon | ||
| ? $jp->{column}->sprefix | ||
| #: $is_cc | ||
| #? { $jp->{column}->sprefix => 'value' } |
| { | ||
| $select = $select->get_column($col_fq)->max_rs->as_query; | ||
| } | ||
| elsif ($operator eq 'distinct' || $operator eq 'count') |
There was a problem hiding this comment.
The condition $operator eq 'count' is checked in the elsif block above, meaning having the same condition in this line isn't potentially necessary?
| return $self->record_name(%options); | ||
| } | ||
| return $column->sprefix; | ||
| my $tn = $column->sprefix; |
There was a problem hiding this comment.
The variable name is potentially not clear?
| $tn .= "_alternative" if $options{alt}; | ||
| return $tn; | ||
| } | ||
| my $jn = $self->_join_number($column, %options); |
There was a problem hiding this comment.
The variable name is potentially not clear?
| if $debug; | ||
| my $n; | ||
| if ($j->{all_joins}) | ||
| my $n = _find($column, $j, $stash, %options); |
There was a problem hiding this comment.
The variable name is potentially not clear?
| trace "This join has other joins, checking..." | ||
| if $debug; | ||
| foreach my $j2 (@{$j->{all_joins}}) | ||
| foreach my $j2 (@{$j->{children}}) |
There was a problem hiding this comment.
The variable name is potentially not clear?
| trace "This join has other joins, checking..." | ||
| if $debug; | ||
| foreach my $j3 (@{$j2->{all_joins}}) # Replace with recursive function? | ||
| foreach my $j3 (@{$j2->{children}}) # Replace with recursive function? |
There was a problem hiding this comment.
The variable name is potentially not clear?
| } | ||
| elsif ($operator eq 'sum' || $operator eq 'max') # Default to max for sum of non-numeric columns | ||
| { | ||
| my $fn = (ref $column->tjoin(join_current_version => $self->cvo_values) eq 'HASH' ? 'value_2' : $column->field).".".$column->value_field; |
There was a problem hiding this comment.
Could this be a multiline potentially for better readability?
| error __"Invalid column ID for sort" | ||
| unless $col_order && $col_order->user_can('read'); | ||
| my $sort = { type => $params->get('order[0][dir]'), id => $col_order->id }; | ||
| my $sort = { type => $params->get('order[0][dir]'), id => $col_order->id, parent_id => $col_order->parent_id }; |
There was a problem hiding this comment.
Could this be potentially a multiline?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit improves performance by linking the current version of a record to its main stub, thereby making retrieval faster for records with lots of versions. The version retrieval functionality is retained for the purpose of historical view, which operates as it previously did.
This commit also includes the addition of curval subfields to the y-axis of graphs (back-end only) which was needed to test some of the changes.
It also includes sorting for grouping, which was needed for keeping the grouping tests working as before.