[Tarantool-patches] [PATCH v2 4/4] sql: make type mismatch errors more informative

[Mergen Imeev]

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 at tarantool.org <imeevma at 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
>