From: Aleksander Alekseev <a.alekseev@postgrespro.ru>
To: pgsql-hackers@postgresql.org
Cc: Victor Drobny <v.drobny@postgrespro.ru>
Subject: Re: new function for tsquery creartion
Date: Tue, 28 Nov 2017 14:39:16 +0000
Message-ID: <20171128143916.1377.5847.pgcf@coridan.postgresql.org> (raw)
In-Reply-To: <15517a04afd2be779e2e6d69611c348b@postgrespro.ru>
References: <fe931111ff7e9ad79196486ada79e268@postgrespro.ru>
<15517a04afd2be779e2e6d69611c348b@postgrespro.ru>
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Hi Victor,
I like the idea and I think it's a great patch. However in current shape it
requires some amount of reworking to meet PostgreSQL standards of code quality.
Particularly:
1. Many new procedures don't have a comment with a brief description. Ideally
every procedure should have not only a brief description but also a description
of every argument, return value and changes of global state if applicable.
2. I believe you could implement the has_prefix procedure just as a wrapper of
strstr().
3. I suggest to use snprintf instead of sprintf in a new code whenever
possible, especially if you are using %s - just to be on a safe side.
4. I noticed that your code affects the catalog. Did you check that your
changes will not cause problems during the migration from the older version of
PostgreSQL to the never one?
5. Tests for queryto_tsquery use only ASCII strings. I suggest to add a few
test that use non-ASCII characters as well, and a few corner cases like empty
string, string that contains only the stop-words, etc.
6. `make check-world` doesn't pass:
```
***************
*** 1672,1678 ****
(1 row)
set enable_seqscan = on;
-
--test queryto_tsquery function
select queryto_tsquery('My brand new smartphone');
queryto_tsquery
--- 1672,1677 ----
***************
*** 1784,1786 ****
--- 1783,1786 ----
---------------------------
'fat-rat' & 'fat' & 'rat'
(1 row)
+
```
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.alekseev@postgrespro.ru, v.drobny@postgrespro.ru
Subject: Re: new function for tsquery creartion
In-Reply-To: <20171128143916.1377.5847.pgcf@coridan.postgresql.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox