maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Varun,

I was recently assigned as a reviewer for this patch:
http://lists.askmonty.org/pipermail/commits/2017-December/011777.html


I have some suggestions.

1. The MDEV says the proposed syntax is:

FOR GROUP ROWS DO

and you implemented:

FOR GROUP NEXT ROW DO

I think the syntax on MDEV looks better.

2. I don't like that you're adding a new uint member
   sp_instr_agg_cfetch::m_dest

   An instruction processing a 'FETCH GROUP NEXT ROW' statement
   does not need it.


3. The generated code:

+show function code f1;
+Pos	Instruction
+0	set total at 1 0
+1	jump_if_not 5(5) 1
+2	
+3	set total@1 total@1 + a@0
+4	jump 1
+5	freturn int total at 1

does not look nice.

- The command at position 2 must have a name.

- The command at position 1 looks like a hack.

  The underlying code is:

+    if (! it->val_bool() || thd->spcont->forced_error)

  So it is a conditional jump which never jumps on the condition,
  i.e. val_bool().
  It jumps only on thd->spcont->forced_error.


Please create a new class sp_instr_agg_cfetch_or_jump,
derive it from sp_instr,
and add m_dest to this class.

Please implement print() and execute() for it.
print() should display:
- the name of the command, "agg_cfetch_or_jump".
- the label where it jumps to when 'no rows' happens.

Note, sp_instr_agg_cfetch_or_jump will have almost nothing to share
with the old sp_instr_agg_cfetch.
So sp_instr_agg_cfetch_or_jump should derive from sp_instr,
not from sp_instr_agg_cfetch.

Please perform jump to a proper position on 'no more rows'
just inside  sp_instr_agg_cfetch_or_jump::execute()

So the code should look about like this:

Pos	Instruction
0	set total at 1 0
1       agg_cfetch_or_jump 4
2	set total@1 total@1 + a@0
3	jump 1
4	freturn int total at 1


4. As discussed on slack, the name for the command sp_instr_agg_cfetch
was probably a not good choise. The "c" in other commands like:
- sp_instr_copen
- sp_instr_cfetch
- sp_instr_cclose
is an abbreviation for the word "cursor".

sp_instr_agg_cfetch does not use any cursors (i.e. sp_cursor instances).

Please rename sp_instr_agg_cfetch to sp_instr_fetch_group_row.

And the new command can be named something like:

sp_instr_fetch_group_row_or_jump, instead of sp_instr_agg_cfetch_or_jump.




Greetings.