Test for Alchemy-based full outer join

* Test using pytest framework that would catch bug in issue ibis-project#1772
(outer_join actually did a left join).
* Fixed outer join portion of test_joins test method.
* Removed unittest version of full outer test.
* Refactored MockConnection and MockAlchemyConnection.

In order for the test to catch this problem, it required changes to the
MockAlchemyConnection class. Previously that class's dialect and
_build_ast methods actually used implementations specific to Impala
(objects from `ibis.impala.compiler`). This meant that the `outer_join`
method compiled correctly in the sqlalchemy test because it was using
the Impala compiler, whose implementation did not have the bug.

To make clearer the similarities between Impala- and Alchemy-flavored
mock connections, I refactored so they both share a BaseMockConnection
class. However, I kept the name of the Impala-flavored one as
MockConnection because it is used in several other tests. I wasn't sure
if those other tests really should use Impala or Alchemy. As a to-do,
those other tests should be checked for which flavor they ought to use,
and the MockConnection should be renamed to MockImpalaConnection.

ibis/expr/tests/mocks.py

@@ -12,16 +12,27 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import abc
+import pytest
+
 from ibis.client import SQLClient
 from ibis.expr.schema import Schema
+import ibis.expr.types as ir
+import ibis.sql.alchemy as alch  # noqa: E402
+
 
+class BaseMockConnection(SQLClient, metaclass=abc.ABCMeta):
+    def __init__(self):
+        self.executed_queries = []
 
-class MockConnection(SQLClient):
     @property
+    @abc.abstractmethod
     def dialect(self):
-        from ibis.impala.compiler import ImpalaDialect
+        pass
 
-        return ImpalaDialect
+    @abc.abstractmethod
+    def _build_ast(self, expr, context):
+        pass
 
     _tables = {
         'alltypes': [
@@ -345,19 +356,11 @@ def dialect(self):
         ],
     }
 
-    def __init__(self):
-        self.executed_queries = []
-
     def _get_table_schema(self, name):
         name = name.replace('`', '')
         return Schema.from_tuples(self._tables[name])
 
-    def _build_ast(self, expr, context):
-        from ibis.impala.compiler import build_ast
-
-        return build_ast(expr, context)
-
-    def execute(self, expr, limit=None, params=None):
+    def execute(self, expr, limit=None, params=None, **kwargs):
         ast = self._build_ast_ensure_limit(expr, limit, params=params)
         for query in ast.queries:
             self.executed_queries.append(query.compile())
@@ -369,6 +372,52 @@ def compile(self, expr, limit=None, params=None):
         return queries[0] if len(queries) == 1 else queries
 
 
+class MockConnection(BaseMockConnection):
+    # TODO: Refactor/rename to MockImpalaConnection
+    # TODO: Should some tests using MockImpalaConnection really use MockAlchemyConnection instead?
+    @property
+    def dialect(self):
+        from ibis.impala.compiler import ImpalaDialect
+
+        return ImpalaDialect
+
+    def _build_ast(self, expr, context):
+        from ibis.impala.compiler import build_ast
+
+        return build_ast(expr, context)
+
+
+class MockAlchemyConnection(BaseMockConnection):
+    def __init__(self):
+        super().__init__()
+        sa = pytest.importorskip('sqlalchemy')
+        self.meta = sa.MetaData()
+
+    def table(self, name, database=None):
+        schema = self._get_table_schema(name)
+        return self._inject_table(name, schema)
+
+    def _inject_table(self, name, schema):
+        if name in self.meta.tables:
+            table = self.meta.tables[name]
+        else:
+            table = alch.table_from_schema(name, self.meta, schema)
+
+        node = alch.AlchemyTable(table, self)
+        return ir.TableExpr(node)
+
+    @property
+    def dialect(self):
+        from ibis.sql.alchemy import AlchemyDialect
+
+        return AlchemyDialect
+
+    def _build_ast(self, expr, context):
+        from ibis.sql.alchemy import build_ast
+
+        return build_ast(expr, context)
+
+
 class GeoMockConnection(SQLClient):
     @property
     def dialect(self):

ibis/sql/tests/test_sqlalchemy.py

@@ -22,9 +22,8 @@
 
 import ibis
 import ibis.expr.datatypes as dt
-import ibis.expr.types as ir
 import ibis.sql.alchemy as alch  # noqa: E402
-from ibis.expr.tests.mocks import MockConnection
+from ibis.expr.tests.mocks import MockAlchemyConnection
 from ibis.sql.tests.test_compiler import ExprTestCases  # noqa: E402
 from ibis.tests.util import assert_equal
 
@@ -34,25 +33,6 @@
 L = sa.literal
 
 
-class MockAlchemyConnection(MockConnection):
-    def __init__(self):
-        super().__init__()
-        self.meta = sa.MetaData()
-
-    def table(self, name):
-        schema = self._get_table_schema(name)
-        return self._inject_table(name, schema)
-
-    def _inject_table(self, name, schema):
-        if name in self.meta.tables:
-            table = self.meta.tables[name]
-        else:
-            table = alch.table_from_schema(name, self.meta, schema)
-
-        node = alch.AlchemyTable(table, self)
-        return ir.TableExpr(node)
-
-
 def _table_wrapper(name, tname=None):
     @property
     def f(self):
@@ -227,7 +207,7 @@ def test_joins(self):
                 region.left_join(nation, ipred),
                 rt.join(nt, spred, isouter=True),
             ),
-            (region.outer_join(nation, ipred), rt.outerjoin(nt, spred)),
+            (region.outer_join(nation, ipred), rt.outerjoin(nt, spred, full=True)),
         ]
         for ibis_joined, joined_sqla in fully_mat_joins:
             expected = sa.select([joined_sqla])
@@ -244,7 +224,7 @@ def test_joins(self):
             ),
             (
                 region.outer_join(nation, ipred).projection(nation),
-                rt.outerjoin(nt, spred),
+                rt.outerjoin(nt, spred, full=True),
             ),
         ]
         for ibis_joined, joined_sqla in subselect_joins:
@@ -269,6 +249,21 @@ def test_join_just_materialized(self):
 
         self._compare_sqla(joined, expected)
 
+    def test_full_outer_join(self):
+        """Testing full outer join separately due to previous issue with
+        outer join resulting in left outer join (issue #1773)"""
+        region = self.con.table('tpch_region')
+        nation = self.con.table('tpch_nation')
+
+        predicate = region.r_regionkey == nation.n_regionkey
+        joined = region.outer_join(
+            nation,
+            predicate
+        )
+        joined_sql_str = str(joined.compile())
+        assert 'full' in joined_sql_str.lower()
+        assert 'left' not in joined_sql_str.lower()
+
     def _sqla_tables(self, tables):
         result = []
         for t in tables: