Skip to content

Commit ce40cca

Browse files
author
Alexander Barkov
committed
MDEV-9181 (NULLIF(count(table.col)), 0) gives wrong result on 10.1.x
Wrapping args[0] and args[2] into an Item_cache for aggregate functions.
1 parent 5092ab2 commit ce40cca

File tree

5 files changed

+281
-15
lines changed

5 files changed

+281
-15
lines changed

‎mysql-test/r/null.result

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1465,5 +1465,65 @@ Warnings:
14651465
Note 1003 select `test`.`t1`.`a` AS `a` from `test`.`t1` where ((`test`.`t1`.`a` = 2020) and ((case when 2020 = 2010 then NULL else `test`.`t1`.`a` end) = concat('2020',rand())))
14661466
DROP TABLE t1;
14671467
#
1468+
# MDEV-9181 (NULLIF(count(table.col)), 0) gives wrong result on 10.1.x
1469+
#
1470+
CREATE TABLE t1 (c1 varchar(50) DEFAULT NULL);
1471+
INSERT INTO t1 (c1) VALUES ('hello'), ('hello\r\n'), ('hello'),('hello');
1472+
SELECT NULLIF(COUNT(c1),0) FROM t1;
1473+
NULLIF(COUNT(c1),0)
1474+
4
1475+
SELECT CASE WHEN COUNT(c1)=0 THEN NULL ELSE COUNT(c1) END FROM t1;
1476+
CASE WHEN COUNT(c1)=0 THEN NULL ELSE COUNT(c1) END
1477+
4
1478+
SELECT NULLIF(COUNT(c1)+0,0) AS c1,NULLIF(CAST(COUNT(c1) AS SIGNED),0) AS c2,NULLIF(CONCAT(COUNT(c1)),0) AS c3 FROM t1;
1479+
c1 c2 c3
1480+
4 4 4
1481+
SELECT NULLIF(COUNT(DISTINCT c1),0) FROM t1;
1482+
NULLIF(COUNT(DISTINCT c1),0)
1483+
2
1484+
SELECT CASE WHEN COUNT(DISTINCT c1)=0 THEN NULL ELSE COUNT(DISTINCT c1) END FROM t1;
1485+
CASE WHEN COUNT(DISTINCT c1)=0 THEN NULL ELSE COUNT(DISTINCT c1) END
1486+
2
1487+
DROP TABLE t1;
1488+
CREATE TABLE t1 (
1489+
id INT NOT NULL,
1490+
c1 INT DEFAULT NULL
1491+
);
1492+
INSERT INTO t1 VALUES (1,1),(1,2),(2,3),(2,4);
1493+
SELECT NULLIF(COUNT(c1),0) AS c1,NULLIF(COUNT(c1)+0,0) AS c1_wrapped,CASE WHEN COUNT(c1) IS NULL THEN 0 ELSE COUNT(c1) END AS c1_case FROM t1 GROUP BY id;
1494+
c1 c1_wrapped c1_case
1495+
2 2 2
1496+
2 2 2
1497+
DROP TABLE t1;
1498+
CREATE TABLE t1 (a INT);
1499+
INSERT INTO t1 VALUES (1),(2),(3);
1500+
SET @a=0;
1501+
SELECT NULLIF(LAST_VALUE(@a:=@a+1,a),0) FROM t1;
1502+
NULLIF(LAST_VALUE(@a:=@a+1,a),0)
1503+
1
1504+
2
1505+
3
1506+
SELECT @a;
1507+
@a
1508+
6
1509+
SET @a=0;
1510+
SELECT NULLIF(AVG(a),0), NULLIF(AVG(LAST_VALUE(@a:=@a+1,a)),0) FROM t1;
1511+
NULLIF(AVG(a),0) NULLIF(AVG(LAST_VALUE(@a:=@a+1,a)),0)
1512+
2.0000 2.0000
1513+
SELECT @a;
1514+
@a
1515+
3
1516+
EXPLAIN EXTENDED SELECT NULLIF(a,0) FROM t1;
1517+
id select_type table type possible_keys key key_len ref rows filtered Extra
1518+
1 SIMPLE t1 ALL NULL NULL NULL NULL 3 100.00
1519+
Warnings:
1520+
Note 1003 select nullif(`test`.`t1`.`a`,0) AS `NULLIF(a,0)` from `test`.`t1`
1521+
EXPLAIN EXTENDED SELECT NULLIF(AVG(a),0) FROM t1;
1522+
id select_type table type possible_keys key key_len ref rows filtered Extra
1523+
1 SIMPLE t1 ALL NULL NULL NULL NULL 3 100.00
1524+
Warnings:
1525+
Note 1003 select nullif(<cache>(avg(`test`.`t1`.`a`)),0) AS `NULLIF(AVG(a),0)` from `test`.`t1`
1526+
DROP TABLE t1;
1527+
#
14681528
# End of 10.1 tests
14691529
#

‎mysql-test/t/null.test

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -909,6 +909,45 @@ EXPLAIN EXTENDED
909909
SELECT * FROM t1 WHERE a=2020 AND NULLIF(a,2010)=CONCAT('2020',RAND());
910910
DROP TABLE t1;
911911

912+
--echo #
913+
--echo # MDEV-9181 (NULLIF(count(table.col)), 0) gives wrong result on 10.1.x
914+
--echo #
915+
CREATE TABLE t1 (c1 varchar(50) DEFAULT NULL);
916+
INSERT INTO t1 (c1) VALUES ('hello'), ('hello\r\n'), ('hello'),('hello');
917+
SELECT NULLIF(COUNT(c1),0) FROM t1;
918+
SELECT CASE WHEN COUNT(c1)=0 THEN NULL ELSE COUNT(c1) END FROM t1;
919+
SELECT NULLIF(COUNT(c1)+0,0) AS c1,NULLIF(CAST(COUNT(c1) AS SIGNED),0) AS c2,NULLIF(CONCAT(COUNT(c1)),0) AS c3 FROM t1;
920+
SELECT NULLIF(COUNT(DISTINCT c1),0) FROM t1;
921+
SELECT CASE WHEN COUNT(DISTINCT c1)=0 THEN NULL ELSE COUNT(DISTINCT c1) END FROM t1;
922+
DROP TABLE t1;
923+
924+
CREATE TABLE t1 (
925+
id INT NOT NULL,
926+
c1 INT DEFAULT NULL
927+
);
928+
INSERT INTO t1 VALUES (1,1),(1,2),(2,3),(2,4);
929+
SELECT NULLIF(COUNT(c1),0) AS c1,NULLIF(COUNT(c1)+0,0) AS c1_wrapped,CASE WHEN COUNT(c1) IS NULL THEN 0 ELSE COUNT(c1) END AS c1_case FROM t1 GROUP BY id;
930+
DROP TABLE t1;
931+
932+
# Testing with side effects
933+
934+
CREATE TABLE t1 (a INT);
935+
INSERT INTO t1 VALUES (1),(2),(3);
936+
SET @a=0;
937+
SELECT NULLIF(LAST_VALUE(@a:=@a+1,a),0) FROM t1;
938+
SELECT @a;
939+
SET @a=0;
940+
SELECT NULLIF(AVG(a),0), NULLIF(AVG(LAST_VALUE(@a:=@a+1,a)),0) FROM t1;
941+
SELECT @a;
942+
943+
# There should not be cache in here:
944+
945+
EXPLAIN EXTENDED SELECT NULLIF(a,0) FROM t1;
946+
947+
# But there should be a cache in here:
948+
EXPLAIN EXTENDED SELECT NULLIF(AVG(a),0) FROM t1;
949+
950+
DROP TABLE t1;
912951

913952
--echo #
914953
--echo # End of 10.1 tests

‎sql/item.h

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3629,6 +3629,11 @@ class Used_tables_and_const_cache
36293629
used_tables_cache|= item->used_tables();
36303630
const_item_cache&= item->const_item();
36313631
}
3632+
void used_tables_and_const_cache_update_and_join(Item *item)
3633+
{
3634+
item->update_used_tables();
3635+
used_tables_and_const_cache_join(item);
3636+
}
36323637
/*
36333638
Call update_used_tables() for all "argc" items in the array "argv"
36343639
and join with the current cache.
@@ -3638,10 +3643,7 @@ class Used_tables_and_const_cache
36383643
void used_tables_and_const_cache_update_and_join(uint argc, Item **argv)
36393644
{
36403645
for (uint i=0 ; i < argc ; i++)
3641-
{
3642-
argv[i]->update_used_tables();
3643-
used_tables_and_const_cache_join(argv[i]);
3644-
}
3646+
used_tables_and_const_cache_update_and_join(argv[i]);
36453647
}
36463648
/*
36473649
Call update_used_tables() for all items in the list
@@ -3654,10 +3656,7 @@ class Used_tables_and_const_cache
36543656
List_iterator_fast<Item> li(list);
36553657
Item *item;
36563658
while ((item=li++))
3657-
{
3658-
item->update_used_tables();
3659-
used_tables_and_const_cache_join(item);
3660-
}
3659+
used_tables_and_const_cache_update_and_join(item);
36613660
}
36623661
};
36633662

@@ -5073,6 +5072,12 @@ class Item_cache: public Item_basic_constant
50735072
return (this->*processor)(arg);
50745073
}
50755074
virtual Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs);
5075+
void split_sum_func2_example(THD *thd, Item **ref_pointer_array,
5076+
List<Item> &fields, uint flags)
5077+
{
5078+
example->split_sum_func2(thd, ref_pointer_array, fields, &example, flags);
5079+
}
5080+
Item *get_example() const { return example; }
50765081
};
50775082

50785083

@@ -5177,6 +5182,30 @@ class Item_cache_str: public Item_cache
51775182
bool cache_value();
51785183
};
51795184

5185+
5186+
class Item_cache_str_for_nullif: public Item_cache_str
5187+
{
5188+
public:
5189+
Item_cache_str_for_nullif(THD *thd, const Item *item)
5190+
:Item_cache_str(thd, item)
5191+
{ }
5192+
Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs)
5193+
{
5194+
/**
5195+
Item_cache_str::safe_charset_converter() returns a new Item_cache
5196+
with Item_func_conv_charset installed on "example". The original
5197+
Item_cache is not referenced (neither directly nor recursively)
5198+
from the result of Item_cache_str::safe_charset_converter().
5199+
5200+
For NULLIF() purposes we need a different behavior:
5201+
we need a new instance of Item_func_conv_charset,
5202+
with the original Item_cache referenced in args[0]. See MDEV-9181.
5203+
*/
5204+
return Item::safe_charset_converter(thd, tocs);
5205+
}
5206+
};
5207+
5208+
51805209
class Item_cache_row: public Item_cache
51815210
{
51825211
Item_cache **values;

‎sql/item_cmpfunc.cc

Lines changed: 138 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2531,12 +2531,137 @@ bool Item_func_if::date_op(MYSQL_TIME *ltime, uint fuzzydate)
25312531
}
25322532

25332533

2534+
void Item_func_nullif::split_sum_func(THD *thd, Item **ref_pointer_array,
2535+
List<Item> &fields, uint flags)
2536+
{
2537+
if (m_cache)
2538+
{
2539+
flags|= SPLIT_SUM_SKIP_REGISTERED; // See Item_func::split_sum_func
2540+
m_cache->split_sum_func2_example(thd, ref_pointer_array, fields, flags);
2541+
args[1]->split_sum_func2(thd, ref_pointer_array, fields, &args[1], flags);
2542+
}
2543+
else
2544+
{
2545+
Item_func::split_sum_func(thd, ref_pointer_array, fields, flags);
2546+
}
2547+
}
2548+
2549+
2550+
void Item_func_nullif::update_used_tables()
2551+
{
2552+
if (m_cache)
2553+
{
2554+
used_tables_and_const_cache_init();
2555+
used_tables_and_const_cache_update_and_join(m_cache->get_example());
2556+
used_tables_and_const_cache_update_and_join(arg_count, args);
2557+
}
2558+
else
2559+
{
2560+
Item_func::update_used_tables();
2561+
}
2562+
}
2563+
2564+
2565+
25342566
void
25352567
Item_func_nullif::fix_length_and_dec()
25362568
{
25372569
if (!args[2]) // Only false if EOM
25382570
return;
25392571

2572+
DBUG_ASSERT(args[0] == args[2]);
2573+
THD *thd= current_thd;
2574+
if (args[0]->type() == SUM_FUNC_ITEM)
2575+
{
2576+
/*
2577+
NULLIF(l_expr, r_expr)
2578+
2579+
is calculated in the way to return a result equal to:
2580+
2581+
CASE WHEN l_expr = r_expr THEN NULL ELSE r_expr END.
2582+
2583+
There's nothing special with r_expr, because it's referenced
2584+
only by args[1] and nothing else.
2585+
2586+
l_expr needs a special treatment, as it's referenced by both
2587+
args[0] and args[2] initially.
2588+
2589+
args[0] and args[2] can be replaced:
2590+
2591+
- to Item_func_conv_charset by character set aggregation routines
2592+
- to a constant Item by equal field propagation routines
2593+
(in case of Item_field)
2594+
2595+
For aggregate functions we have to wrap the original args[0]/args[2]
2596+
into Item_cache (see MDEV-9181). In this case the Item_cache
2597+
instance becomes the subject to character set conversion instead of
2598+
the original args[0]/args[2], while the original args[0]/args[2] get
2599+
hidden inside the cache.
2600+
2601+
Some examples of what NULLIF can end up with after argument
2602+
substitution (we don't mention args[1] here for simplicity):
2603+
2604+
1. l_expr is not an aggragate function:
2605+
2606+
a. No conversion happened.
2607+
args[0] and args[2] were not replaced to something else
2608+
(i.e. neither by character set conversion, nor by propagation):
2609+
2610+
args[0] \
2611+
l_expr
2612+
args[2] /
2613+
2614+
b. Conversion of args[0] happened.
2615+
args[0] was replaced, e.g. to Item_func_conv_charset:
2616+
2617+
args[0] > Item_func_conv_charset \
2618+
l_expr
2619+
args[2] >------------------------/
2620+
2621+
c. Conversion of args[2] happened:
2622+
2623+
args[0] >------------------------\
2624+
l_expr
2625+
args[2] > Item_func_conv_charset /
2626+
2627+
d. Conversion of both args[0] and args[2] happened
2628+
(e.g. by equal field propagation).
2629+
2630+
args[0] > Item_int
2631+
args[2] > Item_int
2632+
2633+
2. In case if l_expr is an aggregate function:
2634+
2635+
a. No conversion happened:
2636+
2637+
args[0] \
2638+
Item_cache > l_expr
2639+
args[2] /
2640+
2641+
b. Conversion of args[0] happened:
2642+
2643+
args[0] > Item_func_conv_charset \
2644+
Item_cache > l_expr
2645+
args[2] >------------------------/
2646+
2647+
c. Conversion of args[2] happened:
2648+
2649+
args[0] >------------------------\
2650+
Item_cache > l_expr
2651+
args[2] > Item_func_conv_charset /
2652+
2653+
d. Conversion of both args[0] and args[2] happened.
2654+
(e.g. by equal expression propagation)
2655+
TODO: check if it's possible (and add an example query if so).
2656+
*/
2657+
m_cache= args[0]->cmp_type() == STRING_RESULT ?
2658+
new (thd->mem_root) Item_cache_str_for_nullif(thd, args[0]) :
2659+
Item_cache::get_cache(thd, args[0]);
2660+
m_cache->setup(current_thd, args[0]);
2661+
m_cache->store(args[0]);
2662+
m_cache->set_used_tables(args[0]->used_tables());
2663+
args[0]= args[2]= m_cache;
2664+
}
25402665
set_handler_by_field_type(args[2]->field_type());
25412666
collation.set(args[2]->collation);
25422667
decimals= args[2]->decimals;
@@ -2611,6 +2736,13 @@ void Item_func_nullif::print(String *str, enum_query_type query_type)
26112736
}
26122737

26132738

2739+
int Item_func_nullif::compare()
2740+
{
2741+
if (m_cache)
2742+
m_cache->cache_value();
2743+
return cmp.compare();
2744+
}
2745+
26142746
/**
26152747
@note
26162748
Note that we have to evaluate the first argument twice as the compare
@@ -2626,7 +2758,7 @@ Item_func_nullif::real_op()
26262758
{
26272759
DBUG_ASSERT(fixed == 1);
26282760
double value;
2629-
if (!cmp.compare())
2761+
if (!compare())
26302762
{
26312763
null_value=1;
26322764
return 0.0;
@@ -2641,7 +2773,7 @@ Item_func_nullif::int_op()
26412773
{
26422774
DBUG_ASSERT(fixed == 1);
26432775
longlong value;
2644-
if (!cmp.compare())
2776+
if (!compare())
26452777
{
26462778
null_value=1;
26472779
return 0;
@@ -2656,7 +2788,7 @@ Item_func_nullif::str_op(String *str)
26562788
{
26572789
DBUG_ASSERT(fixed == 1);
26582790
String *res;
2659-
if (!cmp.compare())
2791+
if (!compare())
26602792
{
26612793
null_value=1;
26622794
return 0;
@@ -2672,7 +2804,7 @@ Item_func_nullif::decimal_op(my_decimal * decimal_value)
26722804
{
26732805
DBUG_ASSERT(fixed == 1);
26742806
my_decimal *res;
2675-
if (!cmp.compare())
2807+
if (!compare())
26762808
{
26772809
null_value=1;
26782810
return 0;
@@ -2687,7 +2819,7 @@ bool
26872819
Item_func_nullif::date_op(MYSQL_TIME *ltime, uint fuzzydate)
26882820
{
26892821
DBUG_ASSERT(fixed == 1);
2690-
if (!cmp.compare())
2822+
if (!compare())
26912823
return (null_value= true);
26922824
return (null_value= args[2]->get_date(ltime, fuzzydate));
26932825
}
@@ -2696,7 +2828,7 @@ Item_func_nullif::date_op(MYSQL_TIME *ltime, uint fuzzydate)
26962828
bool
26972829
Item_func_nullif::is_null()
26982830
{
2699-
return (null_value= (!cmp.compare() ? 1 : args[2]->null_value));
2831+
return (null_value= (!compare() ? 1 : args[2]->null_value));
27002832
}
27012833

27022834

0 commit comments

Comments
 (0)