Ensure custom ops have consistent typing behavior, boolean support

Refined the behavior of :meth:`.Operators.op` such that in all cases,
if the :paramref:`.Operators.op.is_comparison` flag is set to True,
the return type of the resulting expression will be
:class:`.Boolean`, and if the flag is False, the return type of the
resulting expression will be the same type as that of the left-hand
expression, which is the typical default behavior of other operators.
Also added a new parameter :paramref:`.Operators.op.return_type` as well
as a helper method :meth:`.Operators.bool_op`.

Change-Id: Ifc8553cd4037d741b84b70a9702cbd530f1a9de0
Fixes: #4063

9 files changed, 187 insertions, 20 deletions

diff --git a/doc/build/changelog/migration_12.rst b/doc/build/changelog/migration_12.rst
index 36b2819fa..4cfaf38c4 100644
--- a/doc/build/changelog/migration_12.rst
+++ b/doc/build/changelog/migration_12.rst
@@ -1207,6 +1207,56 @@ The reason post_update emits an UPDATE even for an UPDATE is now discussed at
 Key Behavioral Changes - Core
 =============================
 
+.. _change_4063:
+
+The typing behavior of custom operators has been made consistent
+----------------------------------------------------------------
+
+User defined operators can be made on the fly using the
+:meth:`.Operators.op` function.   Previously, the typing behavior of
+an expression against such an operator was inconsistent and also not
+controllable.
+
+Whereas in 1.1, an expression such as the following would produce
+a result with no return type (assume ``-%>`` is some special operator
+supported by the database)::
+
+    >>> column('x', types.DateTime).op('-%>')(None).type
+    NullType()
+
+Other types would use the default behavior of using the left-hand type
+as the return type::
+
+    >>> column('x', types.String(50)).op('-%>')(None).type
+    String(length=50)
+
+These behaviors were mostly by accident, so the behavior has been made
+consistent with the second form, that is the default return type is the
+same as the left-hand expression::
+
+    >>> column('x', types.DateTime).op('-%>')(None).type
+    DateTime()
+
+As most user-defined operators tend to be "comparison" operators, often
+one of the many special operators defined by Postgresql, the
+:paramref:`.Operators.op.is_comparison` flag has been repaired to follow
+its documented behavior of allowing the return type to be :class:`.Boolean`
+in all cases, including for :class:`.ARRAY` and :class:`.JSON`::
+
+    >>> column('x', types.String(50)).op('-%>', is_comparison=True)(None).type
+    Boolean()
+    >>> column('x', types.ARRAY(types.Integer)).op('-%>', is_comparison=True)(None).type
+    Boolean()
+    >>> column('x', types.JSON()).op('-%>', is_comparison=True)(None).type
+    Boolean()
+
+To assist with boolean comparison operators, a new shorthand method
+:meth:`.Operators.bool_op` has been added.    This method should be preferred
+for on-the-fly boolean operators::
+
+    >>> print(column('x', types.Integer).bool_op('-%>')(5))
+    x -%> :x_1
+
 
 .. _change_3785:
 
diff --git a/doc/build/changelog/unreleased_12/4063.rst b/doc/build/changelog/unreleased_12/4063.rst
new file mode 100644
index 000000000..2d79961ee
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4063.rst
@@ -0,0 +1,16 @@
+.. change::
+    :tags: bug, sql
+    :tickets: 4063
+
+    Refined the behavior of :meth:`.Operators.op` such that in all cases,
+    if the :paramref:`.Operators.op.is_comparison` flag is set to True,
+    the return type of the resulting expression will be
+    :class:`.Boolean`, and if the flag is False, the return type of the
+    resulting expression will be the same type as that of the left-hand
+    expression, which is the typical default behavior of other operators.
+    Also added a new parameter :paramref:`.Operators.op.return_type` as well
+    as a helper method :meth:`.Operators.bool_op`.
+
+    .. seealso::
+
+        :ref:`change_4063`
\ No newline at end of file
diff --git a/doc/build/core/custom_types.rst b/doc/build/core/custom_types.rst
index 64f91b23f..5384d0fd4 100644
--- a/doc/build/core/custom_types.rst
+++ b/doc/build/core/custom_types.rst
@@ -513,6 +513,15 @@ expression object produces a new SQL expression construct. Above, we
 could just as well have said ``self.expr.op("goofy")(other)`` instead
 of ``self.op("goofy")(other)``.
 
+When using :meth:`.Operators.op` for comparison operations that return a
+boolean result, the :paramref:`.Operators.op.is_comparison` flag should be
+set to ``True``::
+
+    class MyInt(Integer):
+        class comparator_factory(Integer.Comparator):
+            def is_frobnozzled(self, other):
+                return self.op("--is_frobnozzled->", is_comparison=True)(other)
+
 New methods added to a :class:`.TypeEngine.Comparator` are exposed on an
 owning SQL expression
 using a ``__getattr__`` scheme, which exposes methods added to
@@ -532,7 +541,6 @@ Using the above type::
     >>> print(sometable.c.data.log(5))
     log(:log_1, :log_2)
 
-
 Unary operations
 are also possible.  For example, to add an implementation of the
 PostgreSQL factorial operator, we combine the :class:`.UnaryExpression` construct
@@ -555,12 +563,12 @@ Using the above type::
     >>> print(column('x', MyInteger).factorial())
     x !
 
-See also:
+.. seealso::
+
+    :meth:`.Operators.op`
 
-:attr:`.TypeEngine.comparator_factory`
+    :attr:`.TypeEngine.comparator_factory`
 
-.. versionadded:: 0.8  The expression system was enhanced to support
-  customization of operators on a per-type level.
 
 
 Creating New Types
diff --git a/doc/build/core/tutorial.rst b/doc/build/core/tutorial.rst
index 432066980..b8a362daa 100644
--- a/doc/build/core/tutorial.rst
+++ b/doc/build/core/tutorial.rst
@@ -640,10 +640,17 @@ normally expected, using :func:`.type_coerce`::
     stmt = select([expr])
 
 
+For boolean operators, use the :meth:`.Operators.bool_op` method, which
+will ensure that the return type of the expression is handled as boolean::
+
+    somecolumn.bool_op('-->')('some value')
+
+.. versionadded:: 1.2.0b3  Added the :meth:`.Operators.bool_op` method.
+
 Operator Customization
 ----------------------
 
-While :meth:`.ColumnOperators.op` is handy to get at a custom operator in a hurry,
+While :meth:`.Operators.op` is handy to get at a custom operator in a hurry,
 the Core supports fundamental customization and extension of the operator system at
 the type level.   The behavior of existing operators can be modified on a per-type
 basis, and new operations can be defined which become available for all column
diff --git a/lib/sqlalchemy/sql/default_comparator.py b/lib/sqlalchemy/sql/default_comparator.py
index 4485c661b..a52bdbaed 100644
--- a/lib/sqlalchemy/sql/default_comparator.py
+++ b/lib/sqlalchemy/sql/default_comparator.py
@@ -81,6 +81,18 @@ def _boolean_compare(expr, op, obj, negate=None, reverse=False,
                                 negate=negate, modifiers=kwargs)
 
 
+def _custom_op_operate(expr, op, obj, reverse=False, result_type=None,
+                       **kw):
+    if result_type is None:
+        if op.return_type:
+            result_type = op.return_type
+        elif op.is_comparison:
+            result_type = type_api.BOOLEANTYPE
+
+    return _binary_operate(
+        expr, op, obj, reverse=reverse, result_type=result_type, **kw)
+
+
 def _binary_operate(expr, op, obj, reverse=False, result_type=None,
                     **kw):
     obj = _check_literal(expr, op, obj)
@@ -249,7 +261,7 @@ operator_lookup = {
     "div": (_binary_operate,),
     "mod": (_binary_operate,),
     "truediv": (_binary_operate,),
-    "custom_op": (_binary_operate,),
+    "custom_op": (_custom_op_operate,),
     "json_path_getitem_op": (_binary_operate, ),
     "json_getitem_op": (_binary_operate, ),
     "concat_op": (_binary_operate,),
diff --git a/lib/sqlalchemy/sql/operators.py b/lib/sqlalchemy/sql/operators.py
index f8731385b..a14afcb70 100644
--- a/lib/sqlalchemy/sql/operators.py
+++ b/lib/sqlalchemy/sql/operators.py
@@ -104,7 +104,9 @@ class Operators(object):
         """
         return self.operate(inv)
 
-    def op(self, opstring, precedence=0, is_comparison=False):
+    def op(
+            self, opstring, precedence=0, is_comparison=False,
+            return_type=None):
         """produce a generic operator function.
 
         e.g.::
@@ -145,6 +147,16 @@ class Operators(object):
          .. versionadded:: 0.9.2 - added the
             :paramref:`.Operators.op.is_comparison` flag.
 
+        :param return_type: a :class:`.TypeEngine` class or object that will
+          force the return type of an expression produced by this operator
+          to be of that type.   By default, operators that specify
+          :paramref:`.Operators.op.is_comparison` will resolve to
+          :class:`.Boolean`, and those that do not will be of the same
+          type as the left-hand operand.
+
+          .. versionadded:: 1.2.0b3 - added the
+             :paramref:`.Operators.op.return_type` argument.
+
         .. seealso::
 
             :ref:`types_operators`
@@ -152,12 +164,29 @@ class Operators(object):
             :ref:`relationship_custom_operator`
 
         """
-        operator = custom_op(opstring, precedence, is_comparison)
+        operator = custom_op(opstring, precedence, is_comparison, return_type)
 
         def against(other):
             return operator(self, other)
         return against
 
+    def bool_op(self, opstring, precedence=0):
+        """Return a custom boolean operator.
+
+        This method is shorthand for calling
+        :meth:`.Operators.op` and passing the
+        :paramref:`.Operators.op.is_comparison`
+        flag with True.
+
+        .. versionadded:: 1.2.0b3
+
+        .. seealso::
+
+            :meth:`.Operators.op`
+
+        """
+        return self.op(opstring, precedence=precedence, is_comparison=True)
+
     def operate(self, op, *other, **kwargs):
         r"""Operate on an argument.
 
@@ -197,9 +226,9 @@ class custom_op(object):
     """Represent a 'custom' operator.
 
     :class:`.custom_op` is normally instantiated when the
-    :meth:`.ColumnOperators.op` method is used to create a
-    custom operator callable.  The class can also be used directly
-    when programmatically constructing expressions.   E.g.
+    :meth:`.Operators.op` or :meth:`.Operators.bool_op` methods
+    are used to create a custom operator callable.  The class can also be
+    used directly when programmatically constructing expressions.   E.g.
     to represent the "factorial" operation::
 
         from sqlalchemy.sql import UnaryExpression
@@ -210,17 +239,28 @@ class custom_op(object):
                 modifier=operators.custom_op("!"),
                 type_=Numeric)
 
+
+    .. seealso::
+
+        :meth:`.Operators.op`
+
+        :meth:`.Operators.bool_op`
+
     """
     __name__ = 'custom_op'
 
     def __init__(
             self, opstring, precedence=0, is_comparison=False,
-            natural_self_precedent=False, eager_grouping=False):
+            return_type=None, natural_self_precedent=False,
+            eager_grouping=False):
         self.opstring = opstring
         self.precedence = precedence
         self.is_comparison = is_comparison
         self.natural_self_precedent = natural_self_precedent
         self.eager_grouping = eager_grouping
+        self.return_type = (
+            return_type._to_instance(return_type) if return_type else None
+        )
 
     def __eq__(self, other):
         return isinstance(other, custom_op) and \
diff --git a/lib/sqlalchemy/sql/sqltypes.py b/lib/sqlalchemy/sql/sqltypes.py
index d0dbc4881..5e357d39b 100644
--- a/lib/sqlalchemy/sql/sqltypes.py
+++ b/lib/sqlalchemy/sql/sqltypes.py
@@ -51,7 +51,7 @@ class _LookupExpressionAdapter(object):
             othertype = other_comparator.type._type_affinity
             lookup = self.type._expression_adaptations.get(
                 op, self._blank_dict).get(
-                othertype, NULLTYPE)
+                othertype, self.type)
             if lookup is othertype:
                 return (op, other_comparator.type)
             elif lookup is self.type._type_affinity:
@@ -2571,9 +2571,7 @@ class NullType(TypeEngine):
     class Comparator(TypeEngine.Comparator):
 
         def _adapt_expression(self, op, other_comparator):
-            if operators.is_comparison(op):
-                return op, BOOLEANTYPE
-            elif isinstance(other_comparator, NullType.Comparator) or \
+            if isinstance(other_comparator, NullType.Comparator) or \
                     not operators.is_commutative(op):
                 return op, self.expr.type
             else:
diff --git a/lib/sqlalchemy/sql/type_api.py b/lib/sqlalchemy/sql/type_api.py
index 4b561a705..69dd80938 100644
--- a/lib/sqlalchemy/sql/type_api.py
+++ b/lib/sqlalchemy/sql/type_api.py
@@ -93,6 +93,7 @@ class TypeEngine(Visitable):
             boolean comparison or special SQL keywords like MATCH or BETWEEN.
 
             """
+
             return op, self.type
 
         def __reduce__(self):
@@ -353,6 +354,10 @@ class TypeEngine(Visitable):
         return self.__class__.bind_expression.__code__ \
             is not TypeEngine.bind_expression.__code__
 
+    @staticmethod
+    def _to_instance(cls_or_self):
+        return to_instance(cls_or_self)
+
     def compare_values(self, x, y):
         """Compare two values for equality."""
 
@@ -634,7 +639,9 @@ class UserDefinedType(util.with_metaclass(VisitableCheckKWArg, TypeEngine)):
                 )
                 return self.type.adapt_operator(op), self.type
             else:
-                return op, self.type
+                return super(
+                    UserDefinedType.Comparator, self
+                )._adapt_expression(op, other_comparator)
 
     comparator_factory = Comparator
 
diff --git a/test/sql/test_operators.py b/test/sql/test_operators.py
index c18f9f9be..61295467d 100644
--- a/test/sql/test_operators.py
+++ b/test/sql/test_operators.py
@@ -2631,8 +2631,37 @@ class CustomOpTest(fixtures.TestBase):
         assert operators.is_comparison(op1)
         assert not operators.is_comparison(op2)
 
-        expr = c.op('$', is_comparison=True)(None)
-        is_(expr.type, sqltypes.BOOLEANTYPE)
+    def test_return_types(self):
+        some_return_type = sqltypes.DECIMAL()
+
+        for typ in [
+            sqltypes.NULLTYPE,
+            Integer(),
+            ARRAY(String),
+            String(50),
+            Boolean(),
+            DateTime(),
+            sqltypes.JSON(),
+            postgresql.ARRAY(Integer),
+            sqltypes.Numeric(5, 2),
+        ]:
+            c = column('x', typ)
+            expr = c.op('$', is_comparison=True)(None)
+            is_(expr.type, sqltypes.BOOLEANTYPE)
+
+            c = column('x', typ)
+            expr = c.bool_op('$')(None)
+            is_(expr.type, sqltypes.BOOLEANTYPE)
+
+            expr = c.op('$')(None)
+            is_(expr.type, typ)
+
+            expr = c.op('$', return_type=some_return_type)(None)
+            is_(expr.type, some_return_type)
+
+            expr = c.op(
+                '$', is_comparison=True, return_type=some_return_type)(None)
+            is_(expr.type, some_return_type)
 
 
 class TupleTypingTest(fixtures.TestBase):