For usability and speed, it seems like would be helpful if the result set is small, to have an immediate download instead of going through the batch process. It should be an option, because there may be cases where you always want it to go through the batch process.

Files: 
CommentFileSizeAuthor
#16 views_data_export-batch_only_on_large_export-1804624-16.patch1.98 KBdbehrman
PASSED: [[SimpleTest]]: [MySQL] 48 pass(es).
[ View ]
#15 views_data_export-batch_only_on_large_export-1804624-15.patch2.02 KBdbehrman
PASSED: [[SimpleTest]]: [MySQL] 48 pass(es).
[ View ]
#13 views_data_export-batch_only_on_large_export-1804624-13.patch2.02 KBdbehrman
PASSED: [[SimpleTest]]: [MySQL] 48 pass(es).
[ View ]
#10 views_data_export-batch_only_on_large_export-1804624-10.patch1.67 KBdbehrman
FAILED: [[SimpleTest]]: [MySQL] 48 pass(es), 0 fail(s), and 5 exception(s).
[ View ]
#8 views_data_export-batch_only_on_large_export-1804624-8.patch1.62 KBdbehrman
FAILED: [[SimpleTest]]: [MySQL] 48 pass(es), 0 fail(s), and 5 exception(s).
[ View ]
#5 views_data_export-batch_only_on_large_export-1804624-4.patch1.62 KBdbehrman
FAILED: [[SimpleTest]]: [MySQL] 48 pass(es), 0 fail(s), and 5 exception(s).
[ View ]

Comments

nadavoid’s picture

In addition to adding an option to the settings form, it seems like the relevant logic should live either in

views_data_export_plugin_display_export->is_batched()
or

views_data_export_plugin_display_export->execute()
and
views_data_export_plugin_display_export->render()

I'm encountering a bit of a chicken/egg scenario as I try to implement this. In the execute() function, it branches based on whether batching should occur, and what stage of batching the process is currently in. Before that branching occurs, I need to check to see how many total results there are. That requires another execute() to be called. I'm not sure how to do that without getting into recursion that I can't get out of.

nadavoid’s picture

I think that for this to work, I need to execute a one-off query in is_batched() and cache it. That query would need to come from the views configuration. The problem that I'm seeing with this approach is that $this->view->query is usually not populated when is_batched() is called. From what I can tell, the only way I know to get the values used in the query is to load and execute the display, which takes us back to the recursion issue mentioned earlier.

Is there another way to execute the query of a view, without actually execute()ing the view?

nadavoid’s picture

Getting closer...

I can populate the query property, but this interferes with the regular batch export mechanism.

if (empty($this->view->query)) {
  $this->view->build('views_data_export_2'); // get this from $this->view
}
if (!empty($this->view->built)) {
  $this->view->query->execute($this->view);
}

I tried placing that (also omitting the 2nd if statement) in is_batched() and in execute(), but the export doesn't happen. I see an "Export is starting up" then "Exporting 0% complete" and it stays there. So I think that's progress. I probably need to look in execute_initial() and execute_normal() next.

dbehrman’s picture

Created a patch to only use batch export when the total number of results to export are larger than the number in the batch.

dbehrman’s picture

StatusFileSize
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 48 pass(es), 0 fail(s), and 5 exception(s).
[ View ]
nadavoid’s picture

With the patch, it works like a charm. When the batch size is smaller than the total records, the batch process is invoked; otherwise I get an immediate download. Nice!

The only problem I see is that I get a notice when the batch is invoked.

Notice: Trying to get property of non-object in views_plugin_query_default->query() (line 1296 of/Users/david/Stuff/Web/Sites/d7/sites/all/modules/views/plugins/views_plugin_query_default.inc).

That's ->addTag('views_' . $this->view->name);

I'm going to try to get my debugger set up and see what's actually invoking that, but my first hunch would be that it's in if (!$this->view->query) {

nadavoid’s picture

I got my debugger working, and discovered that the notice happens from the new line 201: $count_query = $this->view->query->query()->countQuery();. At that point in views_plugin_query_default.inc, $this->view is null. Not sure why though.

dbehrman’s picture

StatusFileSize
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 48 pass(es), 0 fail(s), and 5 exception(s).
[ View ]

There are a few conditions where there is already a built query, so this will rebuild it to the appropriate query when checking for row count. I think the check for that was originally to prevent recursion, so instead I just added $this->building as a flag to skip the rebuild process when it's already in the process of rebuilding. This might also fix the issue with the notice in comment #6.

nadavoid’s picture

Assigned:nadavoid» Unassigned

The patch in @dbehrman's #8 works great too. The warnings I was seeing are gone, but they are replaced by a new warning. To avoid this new warning, change line 11 of the patch: replace !$this->building with !isset($this->building).

I think that to complete this, it still needs to be an option presented in the UI "Batch only if result set is larger than selected segment size", or at least update the description of Segment Size. Otherwise, people may wonder why, even though they selected batched exports, why batching may not be happening.

dbehrman’s picture

Status:Active» Needs review
StatusFileSize
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] 48 pass(es), 0 fail(s), and 5 exception(s).
[ View ]

Another small update to that patch, taking some advice from @nadavoid to prevent some notices. Also there was still an issue in some fringe cases where the query would not build properly. So there is a new condition that should prevent that (in all of my tests anyway).

Status:Needs review» Needs work

The last submitted patch, views_data_export-batch_only_on_large_export-1804624-10.patch, failed testing.

BassistJimmyJam’s picture

Looks like there are still some notices being thrown which is causing test failures.

dbehrman’s picture

Status:Needs work» Needs review
StatusFileSize
new2.02 KB
PASSED: [[SimpleTest]]: [MySQL] 48 pass(es).
[ View ]

Fixed that notice.

BassistJimmyJam’s picture

Status:Needs review» Needs work
+   * A boolean to ensure that the query build process is not recursive.
+   */
+  var $building = FALSE;
+
+  /**

Properties should be defined using the appropriate visibility keyword rather than var. I realize that other properties in this same class use var, perhaps as a carryover from Drupal 6 which is supposed to be PHP 4 compatible, but this does not conform to Drupal 7 coding standards (see "Visibility" under http://drupal.org/node/608152).

dbehrman’s picture

Status:Needs work» Needs review
StatusFileSize
new2.02 KB
PASSED: [[SimpleTest]]: [MySQL] 48 pass(es).
[ View ]

Changed var to protected

dbehrman’s picture

StatusFileSize
new1.98 KB
PASSED: [[SimpleTest]]: [MySQL] 48 pass(es).
[ View ]

I found a case where the count query would have no conditions and would count the entire table and continue to batch. This patch resolves that issue.

Commerce module did have an issue with this patch in retrieving the table name, but a patch already existed to fix that: http://drupal.org/files/1879260.robust_query_altering.patch