[tarantool-patches] [tarantool-patches] Re: [patches] [PATCH] sql: fix non-working REPLACE optimization

["Bulat Niatshin" <niatshin@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]


Branch:
https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt  

Issue:
https://github.com/tarantool/tarantool/issues/3293  
>
>In some cases ON CONFLICT REPLACE/IGNORE can be optimized. If
>following conditions are true:
>1) Space has PRIMARY KEY index only.
>2) There are no foreign keys to other spaces.
>3) There are no delete triggers to be fired if conflict happens.
>4) The errt or action is REPLACE or IGNORE.
errt: typo.

Done.

>>Then uniqueness can be ensured by Tarantool only, without VDBE
>>bytecode. This patch contains a small fix for that +
>Replace ‘+’ with word. Don’t mention that it is ’small’ fix, let it be just fix.
Done.

>>related code was refactored a little bit and necessary comments
>The same: don’t use ‘little bit’ — it sounds like you left smth
>in a shitty state consciously.
Done.

>>+/*
>>+   * ABORT and DEFAULT error actions can be handled
>>+   * by Tarantool only without emitting VDBE
>Remove word ‘only’: it confuses little bit (IMHO). 
Done.

>>+
>>+/*
>>+   * Find out what action to take in case there is
>>+   * a uniqueness conflict.
>>+   */
>>+onError = pIdx->onError;
>>+
>Nit-picking: don’t put too many empty lines.
>In this particular case, you outline very elementary statement.
Done.

>>+/*
>>+   * override_error is an action which is directly
>>+   * specified by user in queries like
>>+   * INSERT OR <override_error> and therefore
>>+   * should have higher priority than indexes
>>+   * error actions.
>>+   */
>You put almost the same comment to on_error struct.
>Change it to more informative or remove.
Done, this comment was removed.

>>+int override_error = on_conflict->override_error;
>>+int optimized_error_action = on_conflict->optimized_on_conflict;
>If you used enum type, you wouldn’t have to add assertion below.
Done, all error actions are enums right now.

>>+/*
>>+   * override_error represents error action in queries like
>>+   * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE,
>>+   * which is strictly specified by user and therefore
>>+   * should have been used even when optimization is
>>+   * possible (uniqueness can be checked by Tarantool).
>>+   */
>This comment is almost copy of one from struct declaration.  
>Remove or rephrase.
Done, removed.

>>+struct on_conflict {
>>+/**
>>+   * Represents an error action in queries like
>>+   * INSERT/UPDATE OR <override_error>, which
>>+   * overrides all space constraints error actions.
>>+   */
>>+int override_error;
>Why do you declare type of this field as int,
>when it can take values from on_conflict_action enum?
Done.

>>+   * the value is ON_CONFLICT_ACTION_NONE, otherwise it is
>>+   * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE.
>>+   */
>>+int optimized_on_conflict;
>Your structure is already called ‘on_conflict’,
>so don’t repeat this phrase in field’s name.
>Also, the same as previous field: better declare it as enum.
Done.






-- 
Bulat Niatshin

[-- Attachment #2: Type: text/html, Size: 14884 bytes --]