From 3a6830d9fc5a121f05fbfd9f980582e6c392a0f2 Mon Sep 17 00:00:00 2001 From: Scott Hajek Date: Fri, 17 May 2019 07:46:05 -0400 Subject: [PATCH] Test for Alchemy-based full outer join * Test using pytest framework that would catch bug in issue #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 | 73 ++++++++++++++++++++++++++----- ibis/sql/tests/test_sqlalchemy.py | 41 ++++++++--------- ibis/tests/all/test_outer_join.py | 41 ----------------- 3 files changed, 79 insertions(+), 76 deletions(-) delete mode 100644 ibis/tests/all/test_outer_join.py diff --git a/ibis/expr/tests/mocks.py b/ibis/expr/tests/mocks.py index 17363e07bb84..bae2829fd7c5 100644 --- a/ibis/expr/tests/mocks.py +++ b/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): diff --git a/ibis/sql/tests/test_sqlalchemy.py b/ibis/sql/tests/test_sqlalchemy.py index f66892dd1829..5a6ebc11122e 100644 --- a/ibis/sql/tests/test_sqlalchemy.py +++ b/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: diff --git a/ibis/tests/all/test_outer_join.py b/ibis/tests/all/test_outer_join.py deleted file mode 100644 index d02976654eef..000000000000 --- a/ibis/tests/all/test_outer_join.py +++ /dev/null @@ -1,41 +0,0 @@ -import os -import unittest -import tempfile - -from ibis.sql.sqlite.client import SQLiteClient - - -class TestFullOuterJoin(unittest.TestCase): - def setUp(self): - filehandle, path = tempfile.mkstemp() - self.filehandle = filehandle - self.path = path - client = SQLiteClient(path, create=True) - self.client = client - - client.con.execute("DROP TABLE IF EXISTS test_left;") - client.con.execute("CREATE TABLE test_left(id_left integer, value_left integer);") - client.con.execute("DROP TABLE IF EXISTS test_right;") - client.con.execute("CREATE TABLE test_right(id_right integer, value_right integer);") - - self.left = client.table('test_left') - self.right = client.table('test_right') - - def tearDown(self): - self.client.con.dispose() - os.close(self.filehandle) - os.remove(self.path) - - def test_outer_join(self): - left = self.left - right = self.right - joined = left.outer_join(right, left.id_left == right.id_right) - sql_string = str(joined.compile()) - self.assertIn( - 'full outer', - sql_string.lower() - ) - - -if __name__ == '__main__': - unittest.main()