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

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

[Bulat Niatshin]

[-- 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 --]

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

[n.pettik]

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

Consider formatting: it is almost unreadable. Don’t add extra tabs to quote.
Also, I don’t see provided changes: you should attach them to your letter,
send in reply to this letter or send patch ver.2.
I can’t give you any review, just because there is no subject of it.

Please, rebase on fresh 2.0 and send diff or whole patch again.
Thanks.

> On 4 Apr 2018, at 11:51, Bulat Niatshin <niatshin@tarantool.org> wrote:
> 
> 
> Branch:
> https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt <https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt> 
> 
> Issue:
> https://github.com/tarantool/tarantool/issues/3293 <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: 17238 bytes --]