Add option to skip ordering when using DISTINCT

When using the DISTINCT workaround, we had to reorder the results
because some instances depended on the breadth-first ordering of
results.

This is not required in all cases so we add an option to skip this so
that we can optimize some queries that are executed very frequently.

lib/gitlab/object_hierarchy.rb

@@ -74,9 +74,16 @@ module Gitlab
           read_only(model.from(Arel::Nodes::As.new(recursive_query.arel, objects_table)).order(depth: hierarchy_order))
         else
           recursive_query = base_and_ancestors_cte(upto).apply_to(model.all)
-          recursive_query = recursive_query.reselect(*recursive_query.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
-          recursive_query = model.from(Arel::Nodes::As.new(recursive_query.arel, objects_table))
-          read_only(remove_depth_and_maintain_order(recursive_query, hierarchy_order: hierarchy_order))
+
+          if skip_ordering?
+            recursive_query = recursive_query.distinct
+          else
+            recursive_query = recursive_query.reselect(*recursive_query.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
+            recursive_query = model.from(Arel::Nodes::As.new(recursive_query.arel, objects_table))
+            recursive_query = remove_depth_and_maintain_order(recursive_query, hierarchy_order: hierarchy_order)
+          end
+
+          read_only(recursive_query)
         end
       else
         recursive_query = base_and_ancestors_cte(upto, hierarchy_order).apply_to(model.all)
@@ -100,9 +107,16 @@ module Gitlab
           read_only(model.from(Arel::Nodes::As.new(base_cte.arel, objects_table)).order(depth: :asc))
         else
           base_cte = base_and_descendants_cte.apply_to(model.all)
-          base_cte = base_cte.reselect(*base_cte.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
-          base_cte = model.from(Arel::Nodes::As.new(base_cte.arel, objects_table))
-          read_only(remove_depth_and_maintain_order(base_cte, hierarchy_order: :asc))
+
+          if skip_ordering?
+            base_cte = base_cte.distinct
+          else
+            base_cte = base_cte.reselect(*base_cte.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
+            base_cte = model.from(Arel::Nodes::As.new(base_cte.arel, objects_table))
+            base_cte = remove_depth_and_maintain_order(base_cte, hierarchy_order: :asc)
+          end
+
+          read_only(base_cte)
         end
       else
         read_only(base_and_descendants_cte(with_depth: with_depth).apply_to(model.all))
@@ -173,6 +187,13 @@ module Gitlab
       false
     end
 
+    # Skips the extra ordering when using distinct on the namespace queries
+    def skip_ordering?
+      return options[:skip_ordering] if options.key?(:skip_ordering)
+
+      true
+    end
+
     # Remove the extra `depth` field using an INNER JOIN to avoid breaking UNION queries
     # and ordering the rows based on the `depth` column to maintain the row order.
     #

spec/lib/gitlab/object_hierarchy_spec.rb

@@ -3,14 +3,16 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::ObjectHierarchy do
-  let!(:parent) { create(:group) }
-  let!(:child1) { create(:group, parent: parent) }
-  let!(:child2) { create(:group, parent: child1) }
+  let_it_be(:parent) { create(:group) }
+  let_it_be(:child1) { create(:group, parent: parent) }
+  let_it_be(:child2) { create(:group, parent: child1) }
+
+  let(:options) { {} }
 
   shared_context 'Gitlab::ObjectHierarchy test cases' do
     describe '#base_and_ancestors' do
       let(:relation) do
-        described_class.new(Group.where(id: child2.id)).base_and_ancestors
+        described_class.new(Group.where(id: child2.id), options: options).base_and_ancestors
       end
 
       it 'includes the base rows' do
@@ -22,13 +24,13 @@ RSpec.describe Gitlab::ObjectHierarchy do
       end
 
       it 'can find ancestors upto a certain level' do
-        relation = described_class.new(Group.where(id: child2)).base_and_ancestors(upto: child1)
+        relation = described_class.new(Group.where(id: child2), options: options).base_and_ancestors(upto: child1)
 
         expect(relation).to contain_exactly(child2)
       end
 
       it 'uses ancestors_base #initialize argument' do
-        relation = described_class.new(Group.where(id: child2.id), Group.none).base_and_ancestors
+        relation = described_class.new(Group.where(id: child2.id), Group.none, options: options).base_and_ancestors
 
         expect(relation).to include(parent, child1, child2)
       end
@@ -40,7 +42,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
       describe 'hierarchy_order option' do
         let(:relation) do
-          described_class.new(Group.where(id: child2.id)).base_and_ancestors(hierarchy_order: hierarchy_order)
+          described_class.new(Group.where(id: child2.id), options: options).base_and_ancestors(hierarchy_order: hierarchy_order)
         end
 
         context ':asc' do
@@ -63,7 +65,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#base_and_descendants' do
       let(:relation) do
-        described_class.new(Group.where(id: parent.id)).base_and_descendants
+        described_class.new(Group.where(id: parent.id), options: options).base_and_descendants
       end
 
       it 'includes the base rows' do
@@ -75,7 +77,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
       end
 
       it 'uses descendants_base #initialize argument' do
-        relation = described_class.new(Group.none, Group.where(id: parent.id)).base_and_descendants
+        relation = described_class.new(Group.none, Group.where(id: parent.id), options: options).base_and_descendants
 
         expect(relation).to include(parent, child1, child2)
       end
@@ -87,7 +89,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
       context 'when with_depth is true' do
         let(:relation) do
-          described_class.new(Group.where(id: parent.id)).base_and_descendants(with_depth: true)
+          described_class.new(Group.where(id: parent.id), options: options).base_and_descendants(with_depth: true)
         end
 
         it 'includes depth in the results' do
@@ -106,14 +108,14 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#descendants' do
       it 'includes only the descendants' do
-        relation = described_class.new(Group.where(id: parent)).descendants
+        relation = described_class.new(Group.where(id: parent), options: options).descendants
 
         expect(relation).to contain_exactly(child1, child2)
       end
     end
 
     describe '#max_descendants_depth' do
-      subject { described_class.new(base_relation).max_descendants_depth }
+      subject { described_class.new(base_relation, options: options).max_descendants_depth }
 
       context 'when base relation is empty' do
         let(:base_relation) { Group.where(id: nil) }
@@ -136,13 +138,13 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#ancestors' do
       it 'includes only the ancestors' do
-        relation = described_class.new(Group.where(id: child2)).ancestors
+        relation = described_class.new(Group.where(id: child2), options: options).ancestors
 
         expect(relation).to contain_exactly(child1, parent)
       end
 
       it 'can find ancestors upto a certain level' do
-        relation = described_class.new(Group.where(id: child2)).ancestors(upto: child1)
+        relation = described_class.new(Group.where(id: child2), options: options).ancestors(upto: child1)
 
         expect(relation).to be_empty
       end
@@ -150,7 +152,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#all_objects' do
       let(:relation) do
-        described_class.new(Group.where(id: child1.id)).all_objects
+        described_class.new(Group.where(id: child1.id), options: options).all_objects
       end
 
       it 'includes the base rows' do
@@ -166,13 +168,13 @@ RSpec.describe Gitlab::ObjectHierarchy do
       end
 
       it 'uses ancestors_base #initialize argument for ancestors' do
-        relation = described_class.new(Group.where(id: child1.id), Group.where(id: non_existing_record_id)).all_objects
+        relation = described_class.new(Group.where(id: child1.id), Group.where(id: non_existing_record_id), options: options).all_objects
 
         expect(relation).to include(parent)
       end
 
       it 'uses descendants_base #initialize argument for descendants' do
-        relation = described_class.new(Group.where(id: non_existing_record_id), Group.where(id: child1.id)).all_objects
+        relation = described_class.new(Group.where(id: non_existing_record_id), Group.where(id: child1.id), options: options).all_objects
 
         expect(relation).to include(child2)
       end
@@ -210,6 +212,19 @@ RSpec.describe Gitlab::ObjectHierarchy do
       expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
       expect(child2.self_and_ancestors.to_sql).to include("DISTINCT")
     end
+
+    context 'when the skip_ordering option is set' do
+      let(:options) { { skip_ordering: true } }
+
+      it_behaves_like 'Gitlab::ObjectHierarchy test cases'
+
+      it 'does not include ROW_NUMBER()' do
+        query = described_class.new(Group.where(id: parent.id), options: options).base_and_descendants.to_sql
+
+        expect(query).to include("DISTINCT")
+        expect(query).not_to include("ROW_NUMBER()")
+      end
+    end
   end
 
   context 'when the use_distinct_in_object_hierarchy feature flag is disabled' do