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.

Sign up for GitHub

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

Increase solr page size for csv download. #4246

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Increase solr page size for csv download. #4246

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/models/report.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class << self

COLUMN_SELECTOR_COLUMN_SIZE = 3 # Helps display report columns in neatly lined up columns.
ROWS_PER_PAGE = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we are only getting back from Solr the specific fields needed for the report, this could be bumped up to 200 or 250 for even better performance for large reports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, this is the number of results when a user scrolls through the report in Argo. A larger number might make this laggier since a larger response is going to be slower (though they could scroll through more results before a new page is requested). Since there hasn't been a request to change the report in Argo and I don't know why this number was selected, I'm not inclined to make a change at this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjgiarlo looks like this is your code. Feel free to chime in if you feel this is worth further consideration.

Copy link
Member

@mjgiarlo mjgiarlo Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You're correct, @justinlittman. I'm not super inclined to tweak the UI at this point since folks are happy with it.

Are your changes deployed anywhere? I'd be happy to do a quick test of the UI view to ensure the 10K change doesn't affect it, unless you've already done this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked (in prod). The Report UI only pulls back 100 records.

Copy link
Member

Choose a reason for hiding this comment

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

Super, thank you!

ROWS_PER_PAGE_CSV = 10_000

REPORT_FIELDS = [
{
Expand Down Expand Up @@ -216,7 +217,7 @@ class << self
configure_blacklight do |config|
config.search_builder_class = ReportSearchBuilder # leave off faceting for report queries

config.default_solr_params[:rows] = ROWS_PER_PAGE
config.max_per_page = ROWS_PER_PAGE_CSV # Must be >= max number of rows want returned.
config.default_solr_params[:fl] = REPORT_FIELDS.collect { |f| f[:solr_fields] || f[:field] }.flatten.uniq.join(',')

config.sort_fields.clear
Expand Down Expand Up @@ -277,9 +278,10 @@ def report_data
# @return [Enumerator] data in CSV format
def to_csv
@params[:page] = 1
@params[:per_page] = ROWS_PER_PAGE
@params[:per_page] = ROWS_PER_PAGE_CSV
Enumerator.new do |yielder|
yielder << CSV.generate_line(@fields.map { |field| field.fetch(:label) }, force_quotes: true) # header row
(@response,) = search_results(params)
until @response.documents.empty?
report_data.each do |record|
yielder << CSV.generate_line(@fields.map { |field| record[field.fetch(:field)].to_s }, force_quotes: true)
Expand Down