SQLite broken: DatabaseStatementPrefetch iterator implements a scroll cursor, not a forward only one

I get fails in the sqlite search and user tests with this (plus #342366: SQLite badly replaces unamed placeholders without which sqlite currently doesn't even install properly) - they all seem to be in ordering tests, so I guess they are probably related to this patch rather than the other one?

I don't understand this well enough to comment on whether it makes sense, but I can at least confirm that with this patch SQLite works again and all tests pass.

Forgive me for saying the obvious, but this is why there's a NoRewindIterator, and I wonder if using it (vs. userland) wouldn't be faster. I kinda assume you guys have already looked and and dismissed the possibility, but I figured I'd give it a crack just in case.

I looked through, though, and I don't grok the db layer well enough (and sqlite hangs when I try to load anything other than the front page on my system, so stepping through to learn it isn't an option) to know where to tuck one in intelligently. The only thought I had, is a generic approach - just do it in the constructor for DatabaseStatementPrefetch:

  public function __construct(DatabaseConnection $connection, $query, Array $driver_options = array()) {
    $this->dbh = $connection;
    $this->queryString = $query;
    $this->driverOptions = $driver_options;
    return new NoRewindIterator($this);
  }

When I try it, the frontpage still loads like a champ, at least! Given that I dunno how the statement objects get passed around and used, though, I really don't know if this takes care of your problem or not. Patch attached just in case, though, so that the testbot can have a crack at it.

since when can you return something from a __construct?

Yeah...since never, it seems. lol@me. It felt like it was kinda suspect when writing it, but I felt like I'd seen it done somewhere before. Evidently not :P

What you could do is implement IteratorAggregate and in getIterator() return a new NoRewindIterator($this);... but wrapping an Iterator around an Iterator adds several level of indirection.

Yeah, that'd be the way to do it. I think the extra layer of indirection might be just fine though, primarily because the SPL iterator system was specifically designed to layer iterators within iterators in just this sort of way. In fact, that's why the NoRewindIterator extends IteratorIterator, not just implementing Iterator - it's an iterator for iterators. From SPL's perspective, using NoRewindIterator would probably be the 'correct' way of doing it, although that's a pretty fuzzy line when we're effectively designing our own foreach statements. I dunno if a performance benchmark would have anything substantive to say, but since I can't run it on my system properly, I can't say anything more substantive.

@sdboyer: can you still not get past the front page even if you apply #3 and reinstall? I can.

@John Morahan: Nope, I can't seem to get past the frontpage on a clean install even without Damien's patch is applied.

@sdboyer: right, neither can I. But how about with Damien's patch applied?

Wow. That's two idiotic things in a row. I'll try to not go for three here.

Yes, with the patch applied, it works.

LOL! Changing the title and priority to make it more obvious that SQLite is currently totally broken and this patch fixes it.

Sure. RTBC on #3. It looks sane as far as I understand it, and it's definitely better than what's there now :)

Ahh yeah. I wasn't thinking about memory consumption at all. I guess that counts as stupid thing #3 - so triple apologies.

+1

Yeah #3 is fine.

Yay this fixed the problem with sqlite on my install.

Re-posting patch in #3 in the hopes that testing bot will come back with greens.

Results from testing bot are all pass - testing bot is not reporting back here on testing results
http://testing.drupal.org/pifr/file/1/2217

From visual inspection of #20.

return $this->currentRow + array_values($this->currentRow);

I don't really grok what that line is doing... We're merging an array with itself non-destructively? How is that not a no-op?

For the rewind() method, should we perhaps return NULL explicitly? I'm OK with not bothering with an exception here, but it would be more self-documenting I think to explicitly return NULL rather than implicitly.

+    // Nothing to do: our DatabaseStatement can't be rewinded.

Grammar nazi: The colon should be a semi-colon, and the past tense of rewind is "rewound". (Although I could see the point debated here as it is the past tense of a method name, not the past tense of the English verb that method represents. I can live with it either way, but my brain parse errored on "rewinded".)

I don't quiet get the "take the first row out, then put it back in, then take it back out again" dance. If that's necessary OK, but I am not clear on why we are doing so.

Let's address Crell's review.

I can answer the easy ones :p

"We're merging an array with itself non-destructively? How is that not a no-op?" - array_values() produces an array indexed by numeric keys, which is then merged with the original associative array to satisfy PDO::FETCH_BOTH. Do you think this needs a comment? Or perhaps it would be more obvious if the PDO::FETCH_NUM case was moved above it?

I initially thought "rewinded" was OK as the past tense of the method name, but on reflection, method names don't really have a past tense. So yes this should change.

Ah! That's what it's doing! OK, that makes perfect sense now that I understand it, which is a sure-fire sign that it needs a comment, IMO. :-)

Incidentally I think there will still be a slight difference here, because the PHP FETCH_BOTH structure alternates between key and numeric values, I believe, while this code would put all keyed entries first and then all indexed values. That means if you try to iterate the record you'll get a different order. I don't think that's a problem, as if you're iterating a FETCH_BOTH record you're doing something very stupid anyway and I've frankly never even understood what use FETCH_BOTH has, and Drupal doesn't use it anywhere, but I am just mentioning it for posterity as a known non-issue. :-)

Patch and comments all looked good. This made all the db tests running on sqlite happy!

Committed to CVS HEAD. Thanks.