[tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion

[n.pettik]

Generally, patch looks OK to me. I can provide only several minor
comments.

> On 2 May 2018, at 16:25, N.Tatunov <hollow653 at gmail.com> wrote:
> 
> Currently analyze directly on VIEW or
> analyze on db containing a VIEW leads to an
> assertion. This patch adds few appropriate
> check to fix this problem.


From my point of view, sentence like ‘patch solves this problem'
doesn’t seem to be informative enough.
So I would better describe how you fixed problem, sort of:
"ANALYZE statement directly on VIEW raises error,
meanwhile ANALYZE on the whole database just skips views.”
It would be great, if you rephrased somehow your last message.

Moreover, you are capable of using up to 72 chars in one line
for commit message body. Do not make lines be too short - it is not
elegant.

> -			if ((pTab = sqlite3LocateTable(pParse, 0, z)) != 0) {
> -				analyzeTable(pParse, pTab, 0);
> +			if ((pTab = sqlite3LocateTable(pParse, 0, z))) {

We use explicit NULL checks: != NULL.

> +				if (space_is_view(pTab))
> +					sqlite3ErrorMsg(pParse, "VIEWs aren't "
> +					"allowed to be analyzed”);

I would better use singular form: “VIEW isn’t …";

> +test:do_catchsql_test(
> +	"analyzeD-1.9",
> +	[[
> +		CREATE TABLE table1(id INT PRIMARY KEY, a INT);
> +		CREATE VIEW v AS SELECT * FROM table1;
> +		ANALYZE;
> +	]], {
> +		-- <analyzeD-1.9>
> +		0
> +		-- <analyzeD-1.9>
> +	})

Here I would also check that the name of view doesn’t
accidentally trap into stat spaces.

> +test:do_catchsql_test(
> +	"analyzeD-1.10",
> +	[[
> +		ANALYZE v;
> +	]], {
> +		-- <analyzeD-1.10>
> +		1, "VIEWs aren't allowed to be analyzed"
> +		-- <analyzeD-1.10>
> +	})

The same is here: also check that stat spaces don’t contain
statistics for table called‘V’.