[patches] [autoinc 2/2] sql: regenerate opcodes after fixing autoinc
n.pettik
korablev at tarantool.org
Thu Feb 15 03:06:14 MSK 2018
Some tests are failed on your branch:
sql/update-with-nested-select.test.lua [ fail ]
Also, squash two commits into one: there is no need for
separe commit which does nothing but updates opcodes.
>+ int space_id = -1; /* space id of table */
>+ space_id = (int) SQLITE_PAGENO_TO_SPACEID(pTab->tnum);
>+
Why do you declare variable as int and then make cast?
Make it to be uint32_t. Moreover, I would defer computing space id
untill its real usage (after several hundred lines).
>+/* Opcode: NextValue P1 P2 * * *
Choose better name for this opcode, like NextSequenceValue or
NextSequenceId or NextAutoincValue or etc.
>+ * Synopsis: r[P2] = next val from seq r[P1]
IMO, it should be sort of
"next values from space's sequence, where space id is r[p1]".
>+ * Get next value for space from register P1 and write it into
>+ * register P2.
What next value for space? I guess, it should be
"Get next value from sequence attached to space with given space id."
>+ * P1 - contains space_id of necessary space with sequence
>+ * P2 - register number for output
We don't describe opcode arguments in this way.
>+ Opcode semantics assumes that space P1 has attached
>+ * sequence, otherwise assertion fault will be caused.
Sounds too brutally. I suggest to just end VDBE execution with
appropriate return code.
>+ int space_id = pOp->p1;
I would add set of assert to test pOp->p1 type and value.
>+ struct space * autoinc_space = space_by_id(space_id);
Move asterisk to variable name: struct space *space;
>+ struct sequence * space_seq = autoinc_space->sequence;
Same.
>+ pOut->n = 0;
I guess, it's too much. You set type of mem, so by definition it
can't have size of string different from 0.
>+ break;
>+}
Put empty string between opcodes.
More information about the Tarantool-patches
mailing list