10.6-mdev-31744: frame_range_n_bottom assertion error

[bsrikanth-mariadb]

MDEV-31744: Assertion with COUNT(*) OVER (ORDER BY const RANGE BETWEEN...)

When the query uses several Window Functions:
SELECT
WIN_FUNC1() OVER (ORDER BY 'const', col1),
WIN_FUNC2() OVER (ORDER BY col1 RANGE BETWEEN CURRENT ROW
AND 5 FOLLOWING)
compare_window_funcs_by_window_specs() will try to get the Window Specs to
reuse the ORDER BY lists. If the lists produce the same order (like above)
Window Spec of the WIN_FUNC2 will reuse the ORDER BY list of WIN_FUNC1.

However, WIN_FUNC2 has a RANGE-type window frame. It expects to get
ORDER BY list with one element, which it will use to compute frame bounds.
Proving it with ORDER BY list from WIN_FUNC1 ('const', col1) was caused an
assertion failure

The fix is to:

• use the original ORDER BY list when constructing RANGE-type frames
• fix an apparent typo bug in compare_window_funcs_by_window_specs():
  assignment
  win_spec1->save_order_list= win_spec2->order_list;
  saved the order list from the wrong spec. Instead, take one from win_spec1.

[spetrunia]

It's interesting that the testcase requires using a VIEW...
When using a view, the ORDER clause is an Item_direct_view_ref which refers to an Item_field pointing to a field in the original table :

(gdb) p elem1->item[0]
  $23 = (Item_direct_view_ref *) 0x7fff2c91c4d0
(gdb) p elem1->item[0]->real_item()
  $24 = (Item_field *) 0x7fff2c01b218
(gdb) p elem1->item[0]->real_item()->field->table->alias.Ptr
  $25 = 0x7fff2c008668 "t"
(gdb) p elem1->item[0]->real_item()->const_item()
  $26 = true

When not using a VIEW (do SELECT ... FROM t) , the ORDER clause is an Item_temptable_field referring to a field temptable, and it is not considered constant :

(gdb) p elem1->item[0]
  $20 = (Item_temptable_field *) 0x7fff2c01b890
(gdb) p elem1->item[0]->const_item()
  $21 = false
(gdb) p elem1->item[0]->real_item()
  $22 = (Item_temptable_field *) 0x7fff2c01b890

[bsrikanth-mariadb]

It's interesting that the testcase requires using a VIEW... When using a view, the ORDER clause is an Item_direct_view_ref which refers to an Item_field pointing to a field in the original table :

(gdb) p elem1->item[0]
  $23 = (Item_direct_view_ref *) 0x7fff2c91c4d0
(gdb) p elem1->item[0]->real_item()
  $24 = (Item_field *) 0x7fff2c01b218
(gdb) p elem1->item[0]->real_item()->field->table->alias.Ptr
  $25 = 0x7fff2c008668 "t"
(gdb) p elem1->item[0]->real_item()->const_item()
  $26 = true

When not using a VIEW (do SELECT ... FROM t) , the ORDER clause is an Item_temptable_field referring to a field temptable, and it is not considered constant :

(gdb) p elem1->item[0]
  $20 = (Item_temptable_field *) 0x7fff2c01b890
(gdb) p elem1->item[0]->const_item()
  $21 = false
(gdb) p elem1->item[0]->real_item()
  $22 = (Item_temptable_field *) 0x7fff2c01b890

Even for table query, shouldn't elem1->item[0]->const_item() return as true?

[bsrikanth-mariadb]

It's interesting that the testcase requires using a VIEW... When using a view, the ORDER clause is an Item_direct_view_ref which refers to an Item_field pointing to a field in the original table :

(gdb) p elem1->item[0]
  $23 = (Item_direct_view_ref *) 0x7fff2c91c4d0
(gdb) p elem1->item[0]->real_item()
  $24 = (Item_field *) 0x7fff2c01b218
(gdb) p elem1->item[0]->real_item()->field->table->alias.Ptr
  $25 = 0x7fff2c008668 "t"
(gdb) p elem1->item[0]->real_item()->const_item()
  $26 = true

When not using a VIEW (do SELECT ... FROM t) , the ORDER clause is an Item_temptable_field referring to a field temptable, and it is not considered constant :

(gdb) p elem1->item[0]
  $20 = (Item_temptable_field *) 0x7fff2c01b890
(gdb) p elem1->item[0]->const_item()
  $21 = false
(gdb) p elem1->item[0]->real_item()
  $22 = (Item_temptable_field *) 0x7fff2c01b890

Even for table query, shouldn't elem1->item[0]->const_item() return as true?

Also, what is the need for a temporary table here?

[spetrunia]

A testcase without VIEWs:

create table t1 (a int);
insert into t1 values (1),(2);
select 
  count(*) over (order by 'abc','xyz'),  
  count(*) over (order by 123  RANGE BETWEEN CURRENT ROW AND 5 FOLLOWING) 
from t1; 

[bsrikanth-mariadb]

A testcase without VIEWs:

create table t1 (a int);
insert into t1 values (1),(2);
select 
  count(*) over (order by 'abc','xyz'),  
  count(*) over (order by 123  RANGE BETWEEN CURRENT ROW AND 5 FOLLOWING) 
from t1; 

sure, will include this testcase as well.

[spetrunia]

A testcase that even uses a meaningful ordering, not just "order by const" :

select 
  count(*) over (order by 'abc',a), 
  count(*) over (order by a RANGE BETWEEN CURRENT ROW AND 5 FOLLOWING)
from t1;

[spetrunia]

The commit title and text:

MDEV-31744: Initialize Frame_range_n_bottom correctly

The problem in the jira is that an assert was failing
during the creation of Frame_range_n_bottom. It assumes
the number of elements in the order list should be only 1.
However, in the failing query the number is 2.

This is happening because, when several windows functions are
present in the query, we compare the order lists in them using
compare_order_lists(), and if they are assumed equal then the
order_list in one spec is replaced with the order_list in other spec.
Later, when Frame_range_n_bottom is getting created using the
specified order_list, it is failing because of an assert.

For Frame_range_n_bottom, and Frame_range_n_top,
the fix here is to use the save_order_list instead of the order_list
present in the window_spec

Need to be improved.
The "The problem in the jira" doesn't carry any value. Generally the fix should not require one to look into Jira.
But here, the later phrases

the number of elements in the order list should be only 1.
However, in the failing query the number is 2.
only make sense when one has looked in the jira or in the testcase.

Trying to come up with another description...

MDEV-31744: Assertion with COUNT(*) OVER (ORDER BY const RANGE BETWEEN...)

When the query uses several Window Functions:
 SELECT 
   WIN_FUNC1() OVER (ORDER BY 'const', col1), 
   WIN_FUNC2() OVER (ORDER BY col1 RANGE BETWEEN CURRENT ROW 
                                                 AND 5 FOLLOWING)
compare_window_funcs_by_window_specs() will try to get the Window Specs to
reuse the ORDER BY lists. If the lists produce the same order (like above)
Window Spec of the WIN_FUNC2 will reuse the ORDER BY list of WIN_FUNC1.

However, WIN_FUNC2 has a RANGE-type window frame. It expects to get 
ORDER BY list with one element, which it will use to compute frame bounds.
Proving it with ORDER BY list from WIN_FUNC1 ('const', col1) was caused an 
assertion failure

The fix is to use the original ORDER BY list when constructing RANGE-type frames.

[spetrunia]

in get_frame_cursor(), please add a comment before the added code explaining why we use save_order_list.

[bsrikanth-mariadb]

in get_frame_cursor(), please add a comment before the added code explaining why we use save_order_list.

sure. done

[bsrikanth-mariadb]

A testcase that even uses a meaningful ordering, not just "order by const" :

select 
  count(*) over (order by 'abc',a), 
  count(*) over (order by a RANGE BETWEEN CURRENT ROW AND 5 FOLLOWING)
from t1;

sure. added this test.

[spetrunia]

Please add more to the commit comment:

- The fix is to use the original ORDER BY list when constructing RANGE-type frames.
+ The fix is to 
+ - use the original ORDER BY list when constructing RANGE-type frames
+ - fix an apparent typo bug in compare_window_funcs_by_window_specs():
+   assignment
+     win_spec1->save_order_list= win_spec2->order_list;
+   saved the order list from the wrong spec. Take one from win_spec1.

In the test case, please rename used tables to t1 and the view to v1.

(one more small input coming)

[spetrunia]

Looks good after the above is addressed.

Final question is, which version should this go into? I don't think we can put this into 10.6, as this is not a security issue.

In release builds, assert will not be checked. A release build will try to use wrong expression for the RANGE-type bound, which theoretically could cause a wrong query result. But that's it.

Considering this, I think this should go into 10.11.

[spetrunia]

Ok to push after the above is addressed.

[bsrikanth-mariadb]

moving the changes to 10.11 branch