Skip to content
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.

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

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

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

Conversation

toy
Copy link
Contributor

@toy toy commented Sep 27, 2024

Ticket

OP#57305 + OP#57554

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@toy toy force-pushed the bug/57305-sorting-by-custom-field-has-strong-impact-on-performance-for-the-project-list branch 4 times, most recently from f7fbb59 to 40034e9 Compare October 4, 2024 12:49
@toy toy force-pushed the bug/57305-sorting-by-custom-field-has-strong-impact-on-performance-for-the-project-list branch from 33ea0df to 29dac54 Compare October 4, 2024 13:14
@toy toy force-pushed the bug/57305-sorting-by-custom-field-has-strong-impact-on-performance-for-the-project-list branch 4 times, most recently from 74aaecc to 73daf9f Compare October 10, 2024 18:07
Comment on lines +86 to 98
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff may make it confusing, this is extracted from set_summable!

@toy toy force-pushed the bug/57305-sorting-by-custom-field-has-strong-impact-on-performance-for-the-project-list branch 7 times, most recently from 172eb20 to 3dcfa9c Compare October 16, 2024 12:38
@toy toy force-pushed the bug/57305-sorting-by-custom-field-has-strong-impact-on-performance-for-the-project-list branch from 89a556f to 5bd80f6 Compare October 18, 2024 17:24
@toy toy marked this pull request as ready for review October 18, 2024 17:26
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good functionality and also performance wise, in the tests I have conducted. It is now possible to group by way more fields which when looking at community is nice.

There are some issues with the added grouping options, though. Of those, the first is but a display issue. But the second one leads to entries being sorted into the wrong group.

Grouping by integer cf

image

Here, the grouping happens with an integer cf. The groups however are presented as floats.

Grouping by link cf

image

The backend correctly returns just one item in the group. The frontend still places more entries into the group.


# Template for all the join statements.
#
# For single value custom fields thejoin ensures single value for every
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo 'thejoin' -> 'the join'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants