[Rt-commit] rt branch 4.4/ticket-search-reduce-watcher-joins created. rt-4.4.5-9-gb6525004b2
BPS Git Server
git at git.bestpractical.com
Thu Dec 2 05:44:27 UTC 2021
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "rt".
The branch, 4.4/ticket-search-reduce-watcher-joins has been created
at b6525004b2800e4b8c2dc83445405a1013cb620e (commit)
- Log -----------------------------------------------------------------
commit b6525004b2800e4b8c2dc83445405a1013cb620e
Author: sunnavy <sunnavy at bestpractical.com>
Date: Thu Dec 2 04:05:53 2021 +0800
Add tests for shared joins in watcher bundle optimization
diff --git a/t/ticket/search_by_watcher.t b/t/ticket/search_by_watcher.t
index 4af5c3401a..6af511e077 100644
--- a/t/ticket/search_by_watcher.t
+++ b/t/ticket/search_by_watcher.t
@@ -334,6 +334,26 @@ diag 'Test bundle optimization';
[ sort map { $_->Id } $sales_alice_ticket, $engineer_bob_ticket ],
'Found both sales and enginner tickets'
);
+
+ my $requestor_bob_ticket = RT::Test->create_ticket(
+ Queue => $queue,
+ Subject => 'Sales bob',
+ Requestor => 'bob at localhost',
+ );
+
+ $tix = RT::Tickets->new( RT->SystemUser );
+ $tix->FromSQL(
+ qq{
+ Queue = '$queue' AND
+ ( Requestor.Name LIKE 'bob' OR CustomRole.{Engineer}.Name LIKE 'bob' )
+ }
+ );
+ is( $tix->Count, 2, 'Found correct number of tickets' );
+ is_deeply(
+ [ sort map { $_->Id } @{ $tix->ItemsArrayRef } ],
+ [ sort map { $_->Id } $requestor_bob_ticket, $engineer_bob_ticket ],
+ 'Found both bob tickets'
+ );
}
@tickets = ();
commit ba4a8caca7d9f9a229e28194c0ad6a66f2f6d1fa
Author: sunnavy <sunnavy at bestpractical.com>
Date: Thu Dec 2 03:50:53 2021 +0800
Share joins for items with same op/value in watcher bundling optimization
This is mainly to reduce JOINs for performance. E.g.
Queue = 'General' AND
( Requestor.Name LIKE 'root' OR AdminCc.Name LIKE 'root' OR
CustomRole.{foo}.Name LIKE 'root' )
Previously it would create 12 JOINs(4 JOINs for each role), now it only
creates 4 in total.
Even better, since key, op and value are all the same, we can remove the
duplicates of condition clauses, thus the condition part is like:
Users_4.Name LIKE '%root%'
instead of:
Users_4.Name LIKE '%root%' OR
Users_4.Name LIKE '%root%' OR
Users_4.Name LIKE '%root%'
diff --git a/lib/RT/SearchBuilder/Role/Roles.pm b/lib/RT/SearchBuilder/Role/Roles.pm
index c6dbcae6ed..18d4f8b8e6 100644
--- a/lib/RT/SearchBuilder/Role/Roles.pm
+++ b/lib/RT/SearchBuilder/Role/Roles.pm
@@ -102,9 +102,10 @@ sub _RoleGroupsJoin {
$args{'Class'} ||= $self->_RoleGroupClass;
my $name = $args{'Name'};
+ my $name_str = ref $name eq 'ARRAY' ? join( ', ', sort @$name ) : $name;
- return $self->{'_sql_role_group_aliases'}{ $args{'Class'} .'-'. $name }
- if $self->{'_sql_role_group_aliases'}{ $args{'Class'} .'-'. $name }
+ return $self->{'_sql_role_group_aliases'}{ $args{'Class'} .'-'. $name_str }
+ if $self->{'_sql_role_group_aliases'}{ $args{'Class'} .'-'. $name_str }
&& !$args{'New'};
# If we're looking at a role group on a class that "contains" this record
@@ -138,9 +139,10 @@ sub _RoleGroupsJoin {
FIELD => 'Name',
VALUE => $name,
CASESENSITIVE => 0,
+ ref $name eq 'ARRAY' ? ( OPERATOR => 'IN' ) : (),
) if $name;
- $self->{'_sql_role_group_aliases'}{ $args{'Class'} .'-'. $name } = $groups
+ $self->{'_sql_role_group_aliases'}{ $args{'Class'} .'-'. $name_str } = $groups
unless $args{'New'};
return $groups;
@@ -298,12 +300,14 @@ sub RoleLimit {
$args{FIELD} ||= $args{QUOTEVALUE} ? 'EmailAddress' : 'id';
- my ($groups, $group_members, $users);
- if ( $args{'BUNDLE'} and @{$args{'BUNDLE'}}) {
- ($groups, $group_members, $users) = @{ $args{'BUNDLE'} };
- } else {
- $groups = $self->_RoleGroupsJoin( Name => $type, Class => $class, New => !$type );
+ my ($group_name, $groups, $group_members, $users);
+ if ( $args{'BUNDLE'} ) {
+ ($group_name, $groups, $group_members, $users) = @{ $args{'BUNDLE'} };
+
+ # Shared watcher JOINs have the same op/value, so it's safe to skip if one exists already
+ return if ref $group_name eq 'ARRAY' && $groups;
}
+ $groups ||= $self->_RoleGroupsJoin( Name => $group_name || $type, Class => $class, New => !$type );
$self->_OpenParen( $args{SUBCLAUSE} ) if $args{SUBCLAUSE};
if ( $args{OPERATOR} =~ /^IS(?: NOT)?$/i ) {
@@ -483,8 +487,8 @@ sub RoleLimit {
}
}
$self->_CloseParen( $args{SUBCLAUSE} ) if $args{SUBCLAUSE};
- if ($args{BUNDLE} and not @{$args{BUNDLE}}) {
- @{$args{BUNDLE}} = ($groups, $group_members, $users);
+ if ($args{BUNDLE} and @{$args{BUNDLE}} == 1) {
+ push @{$args{BUNDLE}}, $groups, $group_members, $users;
}
return ($groups, $group_members, $users);
}
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 8b55a84879..151678122c 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -3288,12 +3288,13 @@ sub _parser {
return if $node->isLeaf;
return unless lc ($node->getNodeValue||'') eq "or";
my %refs;
+ my %op_value;
my @kids = grep {$_->{Meta}[0] eq "WATCHERFIELD"}
map {$_->getNodeValue}
grep {$_->isLeaf} $node->getAllChildren;
for (@kids) {
my $node = $_;
- my ($key, $subkey, $op) = @{$node}{qw/Key Subkey Op/};
+ my ($key, $subkey, $op, $value, $quote_value) = @{$node}{qw/Key Subkey Op Value QuoteValue/};
next if $node->{Meta}[1] and RT::Ticket->Role($node->{Meta}[1])->{Column};
next if $op =~ /^!=$|\bNOT\b/i;
next if $op =~ /^IS( NOT)?$/i and not $subkey;
@@ -3314,8 +3315,21 @@ sub _parser {
$name = $node->{Meta}[1] || '';
}
+ $quote_value //= 0;
+ push @{$op_value{"$op $quote_value $value"}}, [ $name, $node ] if $name;
# JOIN SQL for shallow and recursive items are different
- $node->{Bundle} = $refs{$name}{ $op =~ /^shallow/i ? 'shallow' : 'recursive' } ||= [];
+ $node->{Bundle} = $refs{$name}{ $op =~ /^shallow/i ? 'shallow' : 'recursive' } ||= [$name];
+ }
+
+ # Merge "Bundle" for items with the same op and value, to share watcher JOINs among them
+ for my $items ( values %op_value ) {
+ next if @$items < 2;
+ my @names = map { $_->[0] } @$items;
+ my @nodes = map { $_->[1] } @$items;
+ my @refs = \@names;
+ for my $node (@nodes) {
+ $node->{Bundle} = \@refs;
+ }
}
}
);
commit d0f5fd4e031534754cc4d90fc6a92e2da50a7e10
Author: sunnavy <sunnavy at bestpractical.com>
Date: Wed Dec 1 23:32:56 2021 +0800
Add tests for the watcher bundle optimization
This is to confirm custom roles and shallow searches work as expected.
diff --git a/t/ticket/search_by_watcher.t b/t/ticket/search_by_watcher.t
index 4fbc2617bb..4af5c3401a 100644
--- a/t/ticket/search_by_watcher.t
+++ b/t/ticket/search_by_watcher.t
@@ -266,5 +266,75 @@ my $nobody = RT::Nobody();
is($tix->Count, 2, "found ticket(s)");
}
+diag 'Test bundle optimization';
+{
+ my $sales = RT::CustomRole->new( RT->SystemUser );
+ my ( $ret, $msg ) = $sales->Create(
+ Name => 'Sales',
+ MaxValues => 0,
+ );
+ ok( $ret, "Created role: $msg" );
+ ( $ret, $msg ) = $sales->AddToObject( ObjectId => $q->id );
+ ok( $ret, "Added Sales to queue: $msg" );
+
+ my $engineer = RT::CustomRole->new( RT->SystemUser );
+ ( $ret, $msg ) = $engineer->Create(
+ Name => 'Engineer',
+ MaxValues => 0,
+ );
+ ok( $ret, "Created role: $msg" );
+ ( $ret, $msg ) = $engineer->AddToObject( ObjectId => $q->id );
+ ok( $ret, "Added Engineer to queue: $msg" );
+
+ my $alice = RT::User->new( RT->SystemUser );
+ $alice->LoadOrCreateByEmail('alice at localhost');
+
+ my $bob = RT::User->new( RT->SystemUser );
+ $bob->LoadOrCreateByEmail('bob at localhost');
+
+ my $sales_alice_ticket = RT::Test->create_ticket(
+ Queue => $queue,
+ Subject => 'Sales alice',
+ $sales->GroupType => 'alice at localhost'
+ );
+ my $engineer_bob_ticket = RT::Test->create_ticket(
+ Queue => $queue,
+ Subject => 'Engineer bob',
+ $engineer->GroupType => 'bob at localhost',
+ );
+
+ my $tix = RT::Tickets->new( RT->SystemUser );
+ $tix->FromSQL(qq{
+ Queue = '$queue' AND
+ ( CustomRole.{Sales}.Name LIKE 'alice' OR CustomRole.{Engineer}.Name LIKE 'bob' )
+ });
+
+ is( $tix->Count, 2, 'Found correct number of tickets' );
+ is_deeply(
+ [ sort map { $_->Id } @{ $tix->ItemsArrayRef } ],
+ [ sort map { $_->Id } $sales_alice_ticket, $engineer_bob_ticket ],
+ 'Found both sales and enginner tickets'
+ );
+
+ my $sales_group = RT::Test->load_or_create_group( 'Sales Team' );
+ ( $ret, $msg ) = $sales_group->AddMember( $bob->Id );
+ ok( $ret, 'Added bob to sales group' );
+
+ ( $ret, $msg ) = $engineer_bob_ticket->RoleGroup( $sales->GroupType )->AddMember( $sales_group->Id );
+ ok( $ret, 'Added sales group to bob ticket' );
+
+ $tix = RT::Tickets->new( RT->SystemUser );
+ $tix->FromSQL(qq{
+ Queue = '$queue' AND
+ ( CustomRole.{Sales}.Name SHALLOW LIKE 'alice' OR CustomRole.{Sales}.Name LIKE 'bob' )
+ });
+ is( $tix->Count, 2, 'Found correct number of tickets' );
+ is_deeply(
+ [ sort map { $_->Id } @{ $tix->ItemsArrayRef } ],
+ [ sort map { $_->Id } $sales_alice_ticket, $engineer_bob_ticket ],
+ 'Found both sales and enginner tickets'
+ );
+}
+
@tickets = ();
done_testing();
commit defb744ebee8a1314eac67188f137633cab18320
Author: sunnavy <sunnavy at bestpractical.com>
Date: Wed Dec 1 21:09:15 2021 +0800
Avoid unnecessary CachedGroupMembers join in watcher searches
When Users JOIN($users) exists already(cached in BUNDLE), which means we
can reuse it, thus we don't need any new joins.
diff --git a/lib/RT/SearchBuilder/Role/Roles.pm b/lib/RT/SearchBuilder/Role/Roles.pm
index e3eeb39af7..c6dbcae6ed 100644
--- a/lib/RT/SearchBuilder/Role/Roles.pm
+++ b/lib/RT/SearchBuilder/Role/Roles.pm
@@ -447,7 +447,7 @@ sub RoleLimit {
FIELD2 => 'id',
);
}
- else {
+ elsif ( !$users ) {
my $cgm_2 = $self->NewAlias('CachedGroupMembers');
my $group_members_2 = $self->Join(
TYPE => 'LEFT',
@@ -464,7 +464,7 @@ sub RoleLimit {
ENTRYAGGREGATOR => 'AND',
);
- $users ||= $self->Join(
+ $users = $self->Join(
TYPE => 'LEFT',
ALIAS1 => $group_members_2,
FIELD1 => 'MemberId',
commit 335cc6df13e014409c1626d9279890ccbbae47db
Author: sunnavy <sunnavy at bestpractical.com>
Date: Wed Dec 1 21:09:01 2021 +0800
Handle shallow/recursive searches differently in watcher bundling optimization
The bundling optimization is to reuse user JOINs, which are different
for shallow/recursive searches:
For shallow searches, the JOIN is like:
Group -> CachedGroupMembers -> Users
For recursive searches, as we need to search direct group members of
ticket role groups, the JOIN is like:
Group -> CachedGroupMembers -> CachedGroupMembers -> Users
Thus we need to separate them.
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 5ec0e0fe67..8b55a84879 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -3313,7 +3313,9 @@ sub _parser {
else {
$name = $node->{Meta}[1] || '';
}
- $node->{Bundle} = $refs{$name} ||= [];
+
+ # JOIN SQL for shallow and recursive items are different
+ $node->{Bundle} = $refs{$name}{ $op =~ /^shallow/i ? 'shallow' : 'recursive' } ||= [];
}
}
);
commit 8b8c7ce39b95bceef9c136285230d491550bb7e7
Author: sunnavy <sunnavy at bestpractical.com>
Date: Wed Dec 1 06:18:04 2021 +0800
Distinguish custom roles in watcher bundling optimization
Previously if you had multiple custom role items like:
Queue = 'General' AND ( CustomRole.{foo}.Name = 'alice' OR CustomRole.{bar}.Name = 'bob' )
It would wrongly return tickets with "alice" or "bob" in custom role
"foo". Technically, it's because $node->{Meta}[1] is empty for custom
role items. This commit uses group type to distinguish different custom
roles.
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 2cee04486a..5ec0e0fe67 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -3297,7 +3297,23 @@ sub _parser {
next if $node->{Meta}[1] and RT::Ticket->Role($node->{Meta}[1])->{Column};
next if $op =~ /^!=$|\bNOT\b/i;
next if $op =~ /^IS( NOT)?$/i and not $subkey;
- $node->{Bundle} = $refs{$node->{Meta}[1] || ''} ||= [];
+
+ my $name;
+ if ( $key eq 'CustomRole' ) {
+ if ( $subkey =~ /^\{(.+?)\}/ ) {
+ my $cr = RT::CustomRole->new(RT->SystemUser);
+ $cr->Load($1);
+ next unless $cr->Id;
+ $name = $cr->GroupType;
+ }
+ else {
+ next;
+ }
+ }
+ else {
+ $name = $node->{Meta}[1] || '';
+ }
+ $node->{Bundle} = $refs{$name} ||= [];
}
}
);
commit 0d29eedb6a70424a1d1b5c351bc9e0bf5022d29e
Author: sunnavy <sunnavy at bestpractical.com>
Date: Wed Dec 1 06:10:37 2021 +0800
Ignore the case of OR in watcher bundling optimization
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 348f66ea9a..2cee04486a 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -3286,7 +3286,7 @@ sub _parser {
sub {
my $node = shift;
return if $node->isLeaf;
- return unless ($node->getNodeValue||'') eq "OR";
+ return unless lc ($node->getNodeValue||'') eq "or";
my %refs;
my @kids = grep {$_->{Meta}[0] eq "WATCHERFIELD"}
map {$_->getNodeValue}
-----------------------------------------------------------------------
hooks/post-receive
--
rt
More information about the rt-commit
mailing list