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.

CommentFileSizeAuthor
#13 subselect_from.patch8.5 KBDave Reid
#9 subselect_from.patch8.73 KBCrell
#8 subselect_from.patch8.74 KBCrell
#5 subselect_from.patch8.74 KBCrell
#1 subselect_from.patch5.57 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Active » Needs review
FileSize
subselect_from.patch5.57 KB

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.

chx’s picture

chx CreditAttribution: chx commented
Title: Add support for subqueries in FROM clause in dynamic query » Add support for subqueries in JOIN clauses in dynamic query
Status: Needs review » Reviewed & tested by the community

The title was incorrect. This does not support FROMs but aside from that, a very fine piece. FROM can be a followup issue.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

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

Crell’s picture

Crell CreditAttribution: Crell commented
Title: Add support for subqueries in JOIN clauses in dynamic query » Add support for subqueries in FROM and JOIN clauses in dynamic query
Status: Needs work » Needs review

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!

Crell’s picture

Crell CreditAttribution: Crell commented
FileSize
subselect_from.patch8.74 KB

And the patch file might be useful, too...

BartVB’s picture

BartVB CreditAttribution: BartVB commented
Status: Needs review » Needs work

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!

Crell’s picture

Crell CreditAttribution: Crell commented

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. :-)

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs work » Needs review
FileSize
subselect_from.patch8.74 KB

And here we go with the doc issue fixed.

Crell’s picture

Crell CreditAttribution: Crell commented
FileSize
subselect_from.patch8.73 KB

Rerolling for offsets and to kickstart the testbot...

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

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.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

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++

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

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!

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Reviewed & tested by the community » Needs review
FileSize
subselect_from.patch8.5 KB

Re-rolled since all try-catch statements were removed from database_test.test

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Looks like the bot likes it and there are no other changes, so back to RTBC. Thanks, Dave!

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Reviewed & tested by the community

Testing bot failure...

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks Crell.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.