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.