Bug/57305 sorting by custom field has strong impact on performance for the project list

app/models/custom_field/order_statements.rb

@@ -27,152 +27,144 @@
 #++
 
 module CustomField::OrderStatements
-  # Returns a ORDER BY clause that can used to sort customized
-  # objects by their value of the custom field.
-  # Returns false, if the custom field can not be used for sorting.
-  def order_statements
+  # Returns the expression to use in ORDER BY clause to sort objects by their
+  # value of the custom field.
+  def order_statement
+    case field_format
+    when "string", "date", "bool", "link", "int", "float", "list", "user", "version"
+      "cf_order_#{id}.value"
+    end
+  end
+
+  # Returns the join statement that is required to sort objects by their value
+  # of the custom field.
+  def order_join_statement
     case field_format
-    when "list"
-      [order_by_list_sql]
     when "string", "date", "bool", "link"
-      [coalesce_select_custom_value_as_string]
-    when "int", "float"
-      # Make the database cast values into numeric
-      # Postgresql will raise an error if a value can not be casted!
-      # CustomValue validations should ensure that it doesn't occur
-      [select_custom_value_as_decimal]
+      join_for_order_by_string_sql
+    when "int"
+      join_for_order_by_int_sql
+    when "float"
+      join_for_order_by_float_sql
+    when "list"
+      join_for_order_by_list_sql
     when "user"
-      [order_by_user_sql]
+      join_for_order_by_user_sql
     when "version"
-      [order_by_version_sql]
+      join_for_order_by_version_sql
     end
   end
 
-  ##
-  # Returns the null handling for the given direction
-  def null_handling(asc)
-    return unless %w[int float].include?(field_format)
-
+  # Returns the ORDER BY option defining order of objects without value for the
+  # custom field.
+  def order_null_handling(asc)
     null_direction = asc ? "FIRST" : "LAST"
     Arel.sql("NULLS #{null_direction}")
   end
 
-  # Returns the grouping result
-  # which differ for multi-value select fields,
-  # because in this case we do want the primary CV values
+  # Returns the expression to use in GROUP BY (and ORDER BY) clause to group
+  # objects by their value of the custom field.
   def group_by_statement
-    return order_statements unless field_format == "list"
+    return unless can_be_used_for_grouping?
 
-    if multi_value?
-      # We want to return the internal IDs in the case of grouping
-      select_custom_values_as_group
-    else
-      coalesce_select_custom_value_as_string
-    end
+    order_statement
   end
 
-  private
-
-  def coalesce_select_custom_value_as_string
-    # COALESCE is here to make sure that blank and NULL values are sorted equally
-    <<-SQL.squish
-      COALESCE(#{select_custom_value_as_string}, '')
-    SQL
-  end
+  # Returns the expression to use in SELECT clause if it differs from one used
+  # to group by
+  def group_by_select_statement
+    return unless field_format == "list"
 
-  def select_custom_value_as_string
-    <<-SQL.squish
-      (
-        SELECT cv_sort.value
-          FROM #{CustomValue.quoted_table_name} cv_sort
-          WHERE #{cv_sort_only_custom_field_condition_sql}
-          LIMIT 1
-      )
-    SQL
+    # MIN needed to not add this column to group by, ANY_VALUE can be used when
+    # minimum required PostgreSQL becomes 16
+    "MIN(cf_order_#{id}.ids)"
   end
 
-  def select_custom_values_as_group
-    <<-SQL.squish
-      COALESCE(
-        (
-          SELECT string_agg(cv_sort.value, '.')
-            FROM #{CustomValue.quoted_table_name} cv_sort
-            WHERE #{cv_sort_only_custom_field_condition_sql}
-              AND cv_sort.value IS NOT NULL
-        ),
-        ''
-      )
-    SQL
-  end
+  # Returns the join statement that is required to group objects by their value
+  # of the custom field.
+  def group_by_join_statement
+    return unless can_be_used_for_grouping?
 
-  def select_custom_value_as_decimal
-    <<-SQL.squish
-      (
-        SELECT CAST(cv_sort.value AS decimal(60,3))
-          FROM #{CustomValue.quoted_table_name} cv_sort
-          WHERE #{cv_sort_only_custom_field_condition_sql}
-            AND cv_sort.value <> ''
-            AND cv_sort.value IS NOT NULL
-          LIMIT 1
-      )
-    SQL
+    order_join_statement
   end
 
-  def order_by_list_sql
-    columns = multi_value? ? "array_agg(co_sort.position ORDER BY co_sort.position)" : "co_sort.position"
-    limit = multi_value? ? "" : "LIMIT 1"
+  private
 
+  def can_be_used_for_grouping? = field_format.in?(%w[list date bool int float string link])
+
+  # Template for all the join statements.
+  #
+  # For single value custom fields the join ensures single value for every
+  # customized object using DISTINCT ON and selecting first value by id of
+  # custom value:
+  #
+  #   LEFT OUTER JOIN (
+  #     SELECT DISTINCT ON (cv.customized_id), cv.customized_id, xxx "value"
+  #       FROM custom_values cv
+  #       WHERE …
+  #       ORDER BY cv.customized_id, cv.id
+  #   ) cf_order_NNN ON cf_order_NNN.customized_id = …
+  #
+  # For multi value custom fields the GROUP BY and value aggregate function
+  # ensure single value for every customized object:
+  #
+  #   LEFT OUTER JOIN (
+  #     SELECT cv.customized_id, ARRAY_AGG(xxx ORDERY BY yyy) "value"
+  #       FROM custom_values cv
+  #       WHERE …
+  #       GROUP BY cv.customized_id, cv.id
+  #   ) cf_order_NNN ON cf_order_NNN.customized_id = …
+  #
+  def join_for_order_sql(value:, add_select: nil, join: nil, multi_value: false)
     <<-SQL.squish
-      (
-        SELECT #{columns}
-          FROM #{CustomOption.quoted_table_name} co_sort
-          INNER JOIN #{CustomValue.quoted_table_name} cv_sort
-            ON cv_sort.value IS NOT NULL AND cv_sort.value != '' AND co_sort.id = cv_sort.value::bigint
-          WHERE #{cv_sort_only_custom_field_condition_sql}
-          #{limit}
-      )
+      LEFT OUTER JOIN (
+        SELECT
+          #{multi_value ? '' : 'DISTINCT ON (cv.customized_id)'}
+            cv.customized_id
+            , #{value} "value"
+            #{", #{add_select}" if add_select}
+          FROM #{CustomValue.quoted_table_name} cv
+          #{join}
+          WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)}
+            AND cv.custom_field_id = #{id}
+            AND cv.value IS NOT NULL
+            AND cv.value != ''
+          #{multi_value ? 'GROUP BY cv.customized_id' : 'ORDER BY cv.customized_id, cv.id'}
+      ) cf_order_#{id}
+        ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id
     SQL
   end
 
-  def order_by_user_sql
-    columns_array = "ARRAY[cv_user.lastname, cv_user.firstname, cv_user.mail]"
+  def join_for_order_by_string_sql = join_for_order_sql(value: "cv.value")
 
-    columns = multi_value? ? "array_agg(#{columns_array} ORDER BY #{columns_array})" : columns_array
-    limit = multi_value? ? "" : "LIMIT 1"
+  def join_for_order_by_int_sql = join_for_order_sql(value: "cv.value::decimal(60)")
 
-    <<-SQL.squish
-      (
-        SELECT #{columns}
-          FROM #{User.quoted_table_name} cv_user
-          INNER JOIN #{CustomValue.quoted_table_name} cv_sort
-            ON cv_sort.value IS NOT NULL AND cv_sort.value != '' AND cv_user.id = cv_sort.value::bigint
-          WHERE #{cv_sort_only_custom_field_condition_sql}
-          #{limit}
-      )
-    SQL
+  def join_for_order_by_float_sql = join_for_order_sql(value: "cv.value::double precision")
+
+  def join_for_order_by_list_sql
+    join_for_order_sql(
+      value: multi_value? ? "ARRAY_AGG(co.position ORDER BY co.position)" : "co.position",
+      add_select: "#{multi_value? ? "ARRAY_TO_STRING(ARRAY_AGG(cv.value ORDER BY co.position), '.')" : 'cv.value'} ids",
+      join: "INNER JOIN #{CustomOption.quoted_table_name} co ON co.id = cv.value::bigint",
+      multi_value:
+    )
   end
 
-  def order_by_version_sql
-    columns = multi_value? ? "array_agg(cv_version.name ORDER BY cv_version.name)" : "cv_version.name"
-    limit = multi_value? ? "" : "LIMIT 1"
+  def join_for_order_by_user_sql
+    columns_array = "ARRAY[users.lastname, users.firstname, users.mail]"
 
-    <<-SQL.squish
-      (
-        SELECT #{columns}
-          FROM #{Version.quoted_table_name} cv_version
-          INNER JOIN #{CustomValue.quoted_table_name} cv_sort
-            ON cv_sort.value IS NOT NULL AND cv_sort.value != '' AND cv_version.id = cv_sort.value::bigint
-          WHERE #{cv_sort_only_custom_field_condition_sql}
-          #{limit}
-      )
-    SQL
+    join_for_order_sql(
+      value: multi_value? ? "ARRAY_AGG(#{columns_array} ORDER BY #{columns_array})" : columns_array,
+      join: "INNER JOIN #{User.quoted_table_name} users ON users.id = cv.value::bigint",
+      multi_value:
+    )
   end
 
-  def cv_sort_only_custom_field_condition_sql
-    <<-SQL.squish
-      cv_sort.customized_type='#{self.class.customized_class.name}'
-      AND cv_sort.customized_id=#{self.class.customized_class.quoted_table_name}.id
-      AND cv_sort.custom_field_id=#{id}
-    SQL
+  def join_for_order_by_version_sql
+    join_for_order_sql(
+      value: multi_value? ? "array_agg(versions.name ORDER BY versions.name)" : "versions.name",
+      join: "INNER JOIN #{Version.quoted_table_name} versions ON versions.id = cv.value::bigint",
+      multi_value:
+    )
   end
 end

app/models/custom_value/link_strategy.rb

@@ -28,7 +28,7 @@
 
 class CustomValue::LinkStrategy < CustomValue::FormatStrategy
   def typed_value
-    formatted_value
+    formatted_value if value.present?
   end
 
   def parse_value(val)

app/models/queries/projects/orders/custom_field_order.rb

@@ -58,10 +58,16 @@ def available?
   private
 
   def order(scope)
-    joined_statement = custom_field.order_statements.map do |statement|
-      Arel.sql("#{statement} #{direction}")
+    if (join_statement = custom_field.order_join_statement)
+      scope = scope.joins(join_statement)
     end
 
-    scope.order(joined_statement)
+    order_statement = "#{custom_field.order_statement} #{direction}"
+
+    if (null_handling = custom_field.order_null_handling(direction == :asc))
+      order_statement = "#{order_statement} #{null_handling}"
+    end
+
+    scope.order(Arel.sql(order_statement))
   end
 end

app/models/queries/work_packages/selects/custom_field_select.rb

@@ -32,21 +32,20 @@ def initialize(custom_field)
 
     @cf = custom_field
 
-    set_name!
-    set_sortable!
-    set_groupable!
-    set_summable!
-  end
-
-  def groupable_custom_field?(custom_field)
-    %w(list date bool int).include?(custom_field.field_format)
+    @name = custom_field.column_name.to_sym
+    @sortable = custom_field.order_statement
+    @sortable_join = custom_field.order_join_statement
+    @groupable = custom_field.group_by_statement
+    @groupable_join = custom_field.group_by_join_statement
+    @groupable_select = custom_field.group_by_select_statement
+    @summable = summable_statement
   end
 
   def caption
     @cf.name
   end
 
-  delegate :null_handling, to: :custom_field
+  def null_handling(...) = custom_field.order_null_handling(...)
 
   def custom_field
     @cf
@@ -68,32 +67,6 @@ def self.instances(context = nil)
 
   private
 
-  def set_name!
-    self.name = custom_field.column_name.to_sym
-  end
-
-  def set_sortable!
-    self.sortable = custom_field.order_statements || false
-  end
-
-  def set_groupable!
-    self.groupable = custom_field.group_by_statement if groupable_custom_field?(custom_field)
-    self.groupable ||= false
-  end
-
-  def set_summable!
-    self.summable = if %w(float int).include?(custom_field.field_format)
-                      select = summable_select_statement
-
-                      ->(query, grouped) {
-                        Queries::WorkPackages::Selects::WorkPackageSelect
-                          .scoped_column_sum(summable_scope(query), select, grouped && query.group_by_statement)
-                      }
-                    else
-                      false
-                    end
-  end
-
   def summable_scope(query)
     WorkPackage
       .where(id: query.results.work_packages)
@@ -105,9 +78,22 @@ def summable_scope(query)
 
   def summable_select_statement
     if custom_field.field_format == "int"
-      "COALESCE(SUM(value::BIGINT)::BIGINT, 0) #{name}"
+      "COALESCE(SUM(#{CustomValue.quoted_table_name}.value::BIGINT)::BIGINT, 0) #{name}"
+    else
+      "COALESCE(ROUND(SUM(#{CustomValue.quoted_table_name}.value::NUMERIC), 2)::FLOAT, 0.0) #{name}"
+    end
+  end
+
+  def summable_statement
+    if %w[float int].include?(custom_field.field_format)
+      select = summable_select_statement
+
+      ->(query, grouped) {
+        Queries::WorkPackages::Selects::WorkPackageSelect
+          .scoped_column_sum(summable_scope(query), select, grouped:, query:)
+      }
     else
-      "COALESCE(ROUND(SUM(value::NUMERIC), 2)::FLOAT, 0.0) #{name}"
+      false
     end
   end
 end

app/models/queries/work_packages/selects/property_select.rb

@@ -130,15 +130,12 @@ def caption
     shared_with_users: {
       sortable: false,
       groupable: false
-
     }
   }
 
   def self.instances(_context = nil)
-    property_selects.filter_map do |name, options|
-      next unless !options[:if] || options[:if].call
-
-      new(name, options.except(:if))
+    property_selects.map do |name, options|
+      new(name, options)
     end
   end
 end