Convert INNER joins to LEFT joins for relationships without "Require this relationship"

Thank you to create an issue!

Relationships are optional by default. The question is whether we should fall back to the defined join type by default and provide an explicit "force optional"
or whether optional means optional all the time. The problem with the second behavior is that LEFT joins are often slower ...

It will also help us fix #1853524: Reintroduce Views integration for Book.

I just ran into this problem as well...

My temporary fix is to just unset the join type for the node_field_data table. Could we just do this in core? Is there any particular reason why the field-data joins should be inner joins instead of left joins?

function MODULE_views_data_alter(array &$data) {
  unset($data['node_field_data']['table']['join']['node']['type']);
}

Removed inner joins from all field-data tables.

Is there any particular reason why the field-data joins should be inner joins instead of left joins?

yeah it is required for translation of the entities.

Just a side-note: if we manage to fix this problem and allow optional relationships in some way, there is also a bug in the node-access system which can cause nodes to be filtered out similar to what the INNER JOIN currently does. Might be important to know for testing ;-) See #1969208: Query node access alter filtering out accesible nodes.

Attached patch adds a relationship handler which allows to configure the automatically added "implicit joins". This shouldn't break anything, but allows users to mark implicit relationships as "not required", thereby forcing them to LEFT instead of INNER joins.

I am testing the patch in #11. (My original goal is to work on #1969208: Query node access alter filtering out accesible nodes.) But I do not see where to configure the relationship. :-(

The idea is that you add a new relationship, e.g. "Comment: Comment data table (implicit)" when working on comments...

Unfortunately the patch does not work yet for data tables of relationships (e.g. the data table for "parent comments"). I'm trying to figure that out now... also trying to add tests ;-)

Seems we need some kind of additional test coverage, good catch!

Now it should work. Tests are still missing. Let's see if I broke anything. Go, testbot, go! :)

New day, let's try again.

Test failures should be fixed now. I have also added a test based on listing comments and parent comments in a view...

Minor code style fixes.

And again... ;)

Interdiffs my dear friend @bforchhammer are reviewers friend.

Hm, that test passes locally. Random testbot failure?

@jibran sorry, here's an interdiff for the changes since yesterday's patch. Does that help? ;-)

I guess those use statements should not have been removed... (not having them causes Twig_Error_Runtime exceptions for anonymous users).

As I said earlier, I came to this issue from #1969208: Query node access alter filtering out accesible nodes. I have been experimenting with the exported view that @TechNikh attached to #35 in that issue.

I have decided that

1. The UI for the patch in #29 (this issue) needs work.
2. This is the wrong approach anyway.

I think I am qualified to make the claim in #1. I am less sure about the one in #2, but if I am right about that, then my opinion on #1 is irrelevant.

This is how the patch from #29 works. I have a view with a relationship (an entity reference). I de-select "Require this relationship" but I still get an inner join. I have to add a new relationship of type Content: Content data table (implicit) "using" the relationship I have already added, and again de-select the "Require this relationship" option (which is enabled by default). If I had not read the comments on this issue (specifically #13) then I would not have guessed to do that.

Here is why I think the approach is wrong, and what is the right approach. After adding a relationship and selecting "Require this relationship" the query looks something like this, according to the Views preview:

SELECT ...
FROM
{node} node
INNER JOIN {node_field_data} ...
LEFT JOIN {node__field_related_pages} ...
INNER JOIN {node} ...
INNER JOIN {node_field_data} ...
WHERE ...
LIMIT ...

If I de-select "Require this relationship", then INNER JOIN {node} changes to LEFT JOIN {node}. If I apply the patch from #29 and de-select "Require this relationship" in the (implicit) relationship, then the second INNER JOIN {node_field_data} becomes LEFT JOIN {node_field_data}. If I understand @jibran's comment in #6 (and I am way out of my depth here) then this will break entity translation.

It seems to me that we always want to use an inner join on node_field_data, but we may then choose an inner or left join on that compound table. (Probably not the right term.) That is, in summary form,

SELECT ...
FROM
({node} node
  INNER JOIN {node_field_data} ...)
LEFT JOIN {node__field_related_pages} ...
INNER JOIN ({node} ...
  INNER JOIN {node_field_data} ...)
WHERE ...
LIMIT ...

The two indented joins should be hard-coded as INNER, but the other joins are INNER or LEFT depending on whether "Require this relationship" has been selected.

In case my summary leaves too much to the imagination, I have attached foo.sql.txt. Once the node__field_related_pages (entity reference) field has been added, along with a few test nodes, I get the expected results from drush sqlc < foo.sql.txt.

Here is an attempt to demonstrate the issue in a test. The existing view already creates 3 entities, 2 of which have an entity reference to another entity. When adding a relationship and filter, I'm expecting all 3 entities to show up (since the relationship is not required.) This is a work in progress from the code sprint, but I'll post it incase it helps.

Edit: This is not the right test. Here the joins are changed to left join; in this test case, it's the filter that is causing this test to fail.

@29 While testing, seems like it's required to add the implicit join first, before creating the node join.
@30 This is going to be hard, because AFAIK, Drupal API doesn't, yet, support the concept of parentheses around joins. I'm not even sure if it can be done in a relationship handler; I think it might be deeper in the database code.

@lokapujya:

Do you agree that my suggestion in #30, if we can get there, is the query we want?

Is this related to #2337509: Remove "@todo In theory we should use the data table as base table, as this would" from EntityViewsData? Maybe, instead of making complex joins (in the sense that some tables are joined and "parenthesized", and then the compound tables are joined with others) we could get the same results more simply, with fewer tables involved.

More directly: in the example above, do we need the node table at all, or is node_field_data all we need? The attached SQL file works just as well as the one I attached in #30.

I will add #2337509: Remove "@todo In theory we should use the data table as base table, as this would" from EntityViewsData as a related issue. If it is not actually related, then someone can remove it.

"Do you agree that my suggestion in #30, if we can get there, is the query we want?" That would work, but we might not need to go there. The issue becomes simpler if we don't have to deal with nested joins. I need to see more test cases to make sure we have all scenarios covered.

Maybe, we move forward with #29. It's the best solution we currently have. We can Create some tests for it. When we come across a test case that doesn't work, we can update the solution. Even if the solution in #29 has to be changed, the tests will still have some value.

I tried to write a test using entities in #31, but I couldn't import the view and test entities successfully into Drupal to modify. Maybe, I could create a test using a view with nodes, but then the simpletest will have to create the nodes and fields that are used by the view. Any ideas?

#2337509: Remove "@todo In theory we should use the data table as base table, as this would" from EntityViewsData was closed as a duplicate of the recently-created #2429447: Use data table as views base table, if available. so I am replacing the older issue with the newer one in the list of related issues.

Now that #2429447: Use data table as views base table, if available. landed, let's see if it simplifies this issue.

I am adding the Novice tag for the task of testing whether this issue has gone away thanks to #2429447: Use data table as views base table, if available.. I will update the issue summary.

Removing sprint weekend tag!!
As suggested by @YesCT

Sorry, these issues were actually worked on during the 2015 Global Sprint
Weekend https://groups.drupal.org/node/447258

Seems like D8 now uses a left join.

SELECT node_field_data.nid AS nid, node_field_data_node__field_related_pages.nid AS node_field_data_node__field_related_pages_nid
FROM 
{node_field_data} node_field_data
LEFT JOIN {node__field_related_pages} node__field_related_pages ON node_field_data.nid = node__field_related_pages.entity_id AND (node__field_related_pages.deleted = '0' AND node__field_related_pages.langcode = node_field_data.langcode)
LEFT JOIN {node_field_data} node_field_data_node__field_related_pages ON node__field_related_pages.field_related_pages_target_id = node_field_data_node__field_related_pages.nid

Then what about the backport to Views 7.x-3.x?

Well, it would be nice to track down how it got fixed in D8 so that maybe D7 views fix can be similar.