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

[n.pettik]

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 at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180405/08c64d63/attachment.html>