Add support for subqueries in FROM and JOIN clauses in dynamic query
Crell - August 23, 2008 - 22:47
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Crell |
| Status: | closed |
Description
I believe this would make it possible to easily write much more efficient queries when doing wacky stuff like node access or certain complex filtering.
#1
And the patch!
This was actually quite easy once I got down to it. I've not tested it recursively, but it should work in theory. The included unit test (gotta love those) provides a sample of the syntax. Quite simply, you pass a $query object as the $table parameter in a join, and the rest just sorta happens. So no API changes otherwise.
I also added some better docs to some methods that I was modifying anyway. In the process I also realized that there was about 10 lines of pointlessly redundant code that I would have had to modify in both places, so I simply removed one of them and modified it in one place.
I still think that this will help us with node_access queries, but it is a good feature even if I'm wrong there.
#2
The title was incorrect. This does not support FROMs but aside from that, a very fine piece. FROM can be a followup issue.
#3
Some comments:
*
$table_string = '{' .$this->connection->escapeTable($table['table']) . '}';- concatenation problem right before $this* That test needs some serious comments. I have no idea what the heck is going on in there. :)
* the TestCase needs PHPDoc
#4
And we're back! Fixed the issues webchick raised in #3. I also discovered that yes, it DOES work for subqueries in the FROM statement TYVM. :-) So I added another unit test for that and documented both of them, including examples of what the resultant SQL should be. I also sprinkled more comments around various docblocks to make it clear that it does indeed work.
Let's get this in soon!
#5
And the patch file might be useful, too...
#6
In the comment for "protected $tables = array();" you rename $name_of_table to $table but you also add "If $name_of_table is a string..." which is inconsistent :)
Not sure why you are adding a line to modules/simpletest/tests/database_test.module ?
"// If the table is a subquery, compile it and integrate it into this query." doesn't seem to describe what happens (or I don't understand the code :D) I don't see it compiling anything there?
Besides these minor points the patch seems to work as advertised. Haven't been able to run the tests yet, having to problem with timeouts SimpleTest on my development install (not related to this patch). Going to look at that tomorrow.
BTW I'm really curious what this patch can do in the performance department once some queries/modules start using this!
#7
The added line is random flotsam from Eclispe, I think, probably removing an extra space or something.
The process of string-ifying the query is "compiling". That turns it into a prepared statement fragment. All of the query objects work that way except for DatabaseCondition, which has a compile() method instead to avoid circular references when it needs its connection object. That terminology is used a few places in the code, I think.
I'll try to fix the first doc issue and resubmit shortly. Someone else is welcome to beat me to it if they want, as I'll be out for a few days. :-)
#8
And here we go with the doc issue fixed.
#9
Rerolling for offsets and to kickstart the testbot...
#10
When applied in conjunction with #342693: SQLite broken: DatabaseStatementPrefetch iterator implements a scroll cursor, not a forward only one, the Select tests, subqueries test results in 12 passes, 0 fails, 0 exceptions, so it looks good! The try-catch statements in the new tests are not needed though (see #343631: DBTNG test cleanup), so those should be removed.
#11
I say RTBC. If this gets in first, then #343631: DBTNG test cleanup will be amended, if that gets in first, then we remove the try-catch. Consistency++
#12
I do also need to make a good note, that with both patches applied and one laptop nearly smoking for a half hour, every database test passes on sqlite: 628 passes, 0 fails, and 0 exceptions!
#13
Re-rolled since all try-catch statements were removed from database_test.test
#14
Looks like the bot likes it and there are no other changes, so back to RTBC. Thanks, Dave!
#15
The last submitted patch failed testing.
#16
Testing bot failure...
#17
Committed to CVS HEAD. Thanks Crell.
#18
Automatically closed -- issue fixed for two weeks with no activity.