[2.1.1] Allow JoinSource on the right hand side of joins #59

Closed
jhtwong opened this Issue Jun 3, 2011 · 5 comments

Comments

Projects
None yet
4 participants

jhtwong commented Jun 3, 2011

With the refactoring to use Arel::Nodes::JoinSource to model joins in ARel 2.1, it seems that we can no longer express joins where the right hand side of the join is another join. In particular, Arel::Visitors::ToSql currently assumes that a JoinSource is always the source of a Arel::Nodes::SelectCore and thus puts in the "FROM" keyword at all times.

To allow JoinSource to be usable as the RHS of a join, I worked around this by changing ToSql#visit_Arel_Nodes_SelectCore to:

def visit_Arel_Nodes_SelectCore o
  [
    "SELECT",
    (visit(o.top) if o.top),
    (visit(o.set_quantifier) if o.set_quantifier),
    ("#{o.projections.map { |x| visit x }.join ', '}" unless o.projections.empty?),
    ("FROM #{visit(o.source)}" if (o.source && (!o.is_a?(Arel::Nodes::JoinSource) || (!o.source.left && o.source.right.empty?)))),
    ("WHERE #{o.wheres.map { |x| visit x }.join ' AND ' }" unless o.wheres.empty?),
    ("GROUP BY #{o.groups.map { |x| visit x }.join ', ' }" unless o.groups.empty?),
    (visit(o.having) if o.having),
  ].compact.join ' '
end

...and ToSql#visit_Arel_Nodes_JoinSource to:

def visit_Arel_Nodes_JoinSource o
  return unless o.left || !o.right.empty?

  [
    (visit(o.left) if o.left),
    o.right.map { |j| visit j }.join(' ')
  ].compact.join ' '
end

At the same time, MySQL requires that a join appearing on the RHS of another join to be parenthesized even though it is unambiguous. So I had to change Arel::Visitors::MySQL to account for this:

def visit_Arel_Nodes_OuterJoin o
  left_result = case o.left
    when Arel::Nodes::JoinSource
      "(#{visit o.left})"
    else
      visit o.left
  end

  "LEFT OUTER JOIN #{left_result} #{visit o.right}"
end

def visit_Arel_Nodes_InnerJoin o
  left_result = case o.left
    when Arel::Nodes::JoinSource
      "(#{visit o.left})"
    else
      visit o.left
  end

  "INNER JOIN #{left_result} #{visit o.right if o.right}"
end
Owner

tenderlove commented Jun 27, 2011

Can you please provide a test that demonstrates the problem? From the sqlite grammar, it looks like join source nodes should point at multiple join nodes. The grammar doesn't allow a join source node to point at another join source node.

jhtwong commented Jun 27, 2011

For the SQL fragment:

FROM
  `epsilons`
  LEFT OUTER JOIN `joiners`
    ON `joiners`.`epsilon_id` = `epsilons`.`id`
  LEFT OUTER JOIN (  `betas`
                   INNER JOIN
                     (SELECT
                        DISTINCT `betas`.`id` AS `filtered_betas_id`
                      FROM
                        `betas`
                      WHERE
                        `betas`.`creator_id` = 5) `filtered_betas`
                   ON `betas`.`id` = `filtered_betas`.`filtered_betas_id`)
    ON `betas`.`id` = `joiners`.`beta_id`

...we would have a sqlite parse tree of:

* select-stmt
  * select-core
    * SELECT
    * ...
    * FROM
    * join-source
      * single-source
        * table-name : `epsilons`
      * join-op
        * LEFT OUTER JOIN
      * single-source
        * table-name : `joiners`
      * join-constraint
        * ON
        * expr : `joiners`.`epsilon_id` = `epsilons`.`id`
      * join-op
        * LEFT OUTER JOIN
      * single-source
        * (
        * join-source
          * single-source
            * table-name : `betas`
          * join-op
            * INNER JOIN
          * single-source
            * (
            * select-stmt
              * ...
            * )
            * table-alias : `filtered_betas`
          * join-constraint
            * ON
            * expr : `betas`.`id` = `filtered_betas`.`filtered_betas_id`
        * )
      * join-constraint
        * ON
        * `betas`.`id` = `joiners`.`beta_id`

So the grammar allows join-source --> single-source --> join-source. Since ARel doesn't model single-source directly, it seems therefore feasible that a JoinSource node should be allowed as the left of a Join node (in addition to being the source of a SelectCore node).

Owner

tenderlove commented Jun 27, 2011

Ah, makes sense. Thanks for explaining. I'll see what I can do to fix it.

tenderlove added a commit that referenced this issue Oct 31, 2011

Merge pull request #90 from JoelJuliano/patch-1
Allow using non-table alias as a rhs relation name, fix for #84 and #59

parkerl commented Feb 24, 2013

Looks like this issue could be closed?

Owner

rafaelfranca commented Feb 25, 2013

Right. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment