• malff/marcsql@weblab.(none)'s avatar
    Bug#24532 (The return data type of IS TRUE is different from similar · 4e556b23
    malff/marcsql@weblab.(none) authored
      operations)
    
    Before this change, the boolean predicates:
    - X IS TRUE,
    - X IS NOT TRUE,
    - X IS FALSE,
    - X IS NOT FALSE
    were implemented by expanding the Item tree in the parser, by using a
    construct like:
    Item_func_if(Item_func_ifnull(X, <value>), <value>, <value>)
    
    Each <value> was a constant integer, either 0 or 1.
    
    A bug in the implementation of the function IF(a, b, c), in
    Item_func_if::fix_length_and_dec(), would cause the following :
    
    When the arguments b and c are both unsigned, the result type of the
    function was signed, instead of unsigned.
    
    When the result of the if function is signed, space for the sign could be
    counted twice (in the max() expression for a signed argument, and in the
    total), causing the member max_length to be too high.
    
    An effect of this is that the final type of IF(x, int(1), int(1)) would be
    int(2) instead of int(1).
    
    With this fix, the problems found in Item_func_if::fix_length_and_dec()
    have been fixed.
    
    While it's semantically correct to represent 'X IS TRUE' with
    Item_func_if(Item_func_ifnull(X, <value>), <value>, <value>),
    there are however more problems with this construct.
    
    a)
    Building the parse tree involves :
    - creating 5 Item instances (3 ints, 1 ifnull, 1 if),
    - creating each Item calls my_pthread_getspecific_ptr() once in the operator
      new(size), and a second time in the Item::Item() constructor, resulting
      in a total of 10 calls to get the current thread.
    Evaluating the expression involves evaluating up to 4 nodes at runtime.
    This representation could be greatly simplified and improved.
    
    b)
    Transforming the parse tree internally with if(ifnull(...)) is fine as long
    as this transformation is internal to the server implementation.
    With views however, the result of the parse tree is later exposed by the
    ::print() functions, and stored as part of the view definition.
    Doing this has long term consequences:
    
    1)
    The original semantic 'X IS TRUE' is lost, and replaced by the
    if(ifnull(...)) expression. As a result, SHOW CREATE VIEW does not restore
    the original code.
    
    2)
    Should a future version of MySQL implement the SQL BOOLEAN data type for
    example, views created today using 'X IS NULL' can be exported using
    mysqldump, and imported again. Such views would be converted correctly and
    automatically to use a BOOLEAN column in the future version.
    With 'X IS TRUE' and the current implementations, views using these
    "boolean" predicates would not be converted during the export/import, and
    would use integer columns instead.
    The difference traces back to how SHOW CREATE VIEW preserves 'X IS NULL' but
    does not preserve the 'X IS TRUE' semantic.
    
    With this fix, internal representation of 'X IS TRUE' booleans predicates
    has changed, so that:
    - dedicated Item classes are created for each predicate,
    - only 1 Item is created to represent 1 predicate
    - my_pthread_getspecific_ptr() is invoked 1 time instead of 10
    - SHOW CREATE VIEW preserves the original semantic, and prints 'X IS TRUE'.
    
    Note that, because of the fix in Item_func_if, views created before this fix
    will:
    - correctly use a int(1) type instead of int(2) for boolean predicates,
    - incorrectly print the if(ifnull(...), ...) expression in SHOW CREATE VIEW,
    since the original semantic (X IS TRUE) has been lost.
    - except for the syntax used in SHOW CREATE VIEW, these views will operate
    properly, no action is needed.
    
    Views created after this fix will operate correctly, and will preserve the
    original code semantic in SHOW CREATE VIEW.
    4e556b23
item_cmpfunc.h 44.5 KB