From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id DEF066EC55; Tue, 13 Jul 2021 13:17:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DEF066EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626171446; bh=fmts9lBOzvoyk0Jah514Dng4mFdMos036IBUzAVwZQQ=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=E7bmsGyYaxxoWSQMQyvUzmh1lbAZwF+XLwHjSc8qdDH5jzDRYIjc16BW+BVVEznwF WYoAY2XQJl0ybSsJOXzilU1Vdo6raGlL6A2RrI+e52of3KVLpRspzM06kQz1MDi+p6 LGaHljzQ/Y20HPMouiJuxOvQQI1qu2dEI9t5hPFI= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8C0F16EC55 for ; Tue, 13 Jul 2021 13:17:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8C0F16EC55 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1m3FTe-0005Eh-Kn; Tue, 13 Jul 2021 13:17:23 +0300 To: Timur Safin Cc: tarantool-patches@dev.tarantool.org References: <03f701d777c4$47a28e90$d6e7abb0$@tarantool.org> Message-ID: Date: Tue, 13 Jul 2021 13:17:22 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <03f701d777c4$47a28e90$d6e7abb0$@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD97BB0EF39AD2B33D5CFD6F66580F08A9E8DA110284E1A7113182A05F5380850407C7C834C7408DFCAFE8EBD2B74C105B523AF5ABDBDAD6460A4C790B70303C8D5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7DB84ED444C624799EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063727BBC20C3D5F36038638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80DA20A5F08F7358E8774DF3242D7671D117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B62CFFCC7B69C47339089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A564FC868A480F40898B5A39AA83D8F5E671E1E69FD0E68C1CD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753753CEE10E4ED4A7410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3458239C6F30CE7ACFA6B4E6D20BF53282A5584DD56D6919C7A7078D16BDF8F2348FBC3FF49916C6741D7E09C32AA3244C6EE0809CEDACC0D27FF4E24F632522CE4DBEAD0ED6C55A80729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojAZDAgpmGsvale0q8fKeO1Q== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DAA3E2C77F2CBB26EB402AD1B8988937083D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 4/4] sql: make type mismatch errors more informative X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thank you for the review! My answer below. On 13.07.2021 11:51, Timur Safin wrote: > This is so much clearer now! > LGTM (though there is some minor complain) > > : From: imeevma@tarantool.org > : Subject: [PATCH v2 4/4] sql: make type mismatch errors more informative > : > ... > > : diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > : index c4375e1ea..cdb55f858 100644 > : --- a/src/box/sql/mem.c > : +++ b/src/box/sql/mem.c > : @@ -78,16 +78,17 @@ mem_str(const struct Mem *mem) > : case MEM_TYPE_NULL: > : return "NULL"; > : case MEM_TYPE_STR: > : - if (mem->n > STR_VALUE_MAX_LEN) > : - return tt_sprintf("'%.*s...", STR_VALUE_MAX_LEN, mem->z); > : - return tt_sprintf("'%.*s'", mem->n, mem->z); > : + if (mem->n <= STR_VALUE_MAX_LEN) > : + return tt_sprintf("string('%.*s')", mem->n, mem->z); > : + return tt_sprintf("string('%.*s...)", STR_VALUE_MAX_LEN, > : + mem->z); > > Hmm, hmm, see my notice below. > > : case MEM_TYPE_INT: > : - return tt_sprintf("%lld", mem->u.i); > : + return tt_sprintf("integer(%lld)", mem->u.i); > : case MEM_TYPE_UINT: > : - return tt_sprintf("%llu", mem->u.u); > : + return tt_sprintf("integer(%llu)", mem->u.u); > : case MEM_TYPE_DOUBLE: > : sql_snprintf(STR_VALUE_MAX_LEN, buf, "%!.15g", mem->u.r); > : - return tt_sprintf("%s", buf); > : + return tt_sprintf("double(%s)", buf); > : case MEM_TYPE_BIN: { > : int len = MIN(mem->n, STR_VALUE_MAX_LEN / 2); > : for (int i = 0; i < len; ++i) { > : @@ -97,22 +98,25 @@ mem_str(const struct Mem *mem) > : buf[2 * i + 1] = n < 10 ? ('0' + n) : ('A' + n - 10); > : } > : if (mem->n > len) > : - return tt_sprintf("x'%.*s...", len * 2, buf); > : - return tt_sprintf("x'%.*s'", len * 2, buf); > : + return tt_sprintf("varbinary(x'%.*s...)", len * 2, buf); > : + return tt_sprintf("varbinary(x'%.*s')", len * 2, buf); > > As for me it makes no much sense to generate not properly quoted > literal even if we mean that it's shortened and truncated. IMVHO > > Though there is no single proper form, because it's simple message, > and customer will probably get the reason why it looks so. My answer from previous review: I believe that "'" is actually part of value. This way we will get inconsistent value in case only part of it was printed. This is similar for what we do with array - in case it is too long we will not close it using "]". Also, it can be confusing because the quoted value with '...' is actually a valid STRING, however it may or may not be the same as the original value. If we don't add "" at the end, there will be no misunderstanding. > : } > : case MEM_TYPE_MAP: > : case MEM_TYPE_ARRAY: { > : const char *str = mp_str(mem->z); > : - if (strlen(str) <= STR_VALUE_MAX_LEN) > : - return str; > : - memcpy(buf, str, STR_VALUE_MAX_LEN); > : - return tt_sprintf("%.*s...", STR_VALUE_MAX_LEN, buf); > : + const char *type = mem_type_to_str(mem); > : + uint32_t len = strlen(str); > : + uint32_t minlen = MIN(STR_VALUE_MAX_LEN, len); > : + memcpy(buf, str, minlen); > : + if (len <= STR_VALUE_MAX_LEN) > : + return tt_sprintf("%s(%.*s)", type, minlen, buf); > : + return tt_sprintf("%s(%.*s...)", type, minlen, buf); > > > : } > : case MEM_TYPE_UUID: > : tt_uuid_to_string(&mem->u.uuid, buf); > : - return tt_sprintf("'%s'", buf); > : + return tt_sprintf("uuid('%s')", buf); > : case MEM_TYPE_BOOL: > : - return mem->u.b ? "TRUE" : "FALSE"; > : + return mem->u.b ? "boolean(TRUE)" : "boolean(FALSE)"; > : default: > : return "unknown"; > : } > > Regards, > Timur >