Cosmetic improvements for default text search parser's ts_headline code.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 9 Apr 2020 16:36:59 +0000 (12:36 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 9 Apr 2020 16:36:59 +0000 (12:36 -0400)
This code was woefully unreadable and under-commented.  Try to improve
matters by adding comments, using some macros to make complicated
if-tests more readable, using boolean type where appropriate, etc.
There are a couple of tiny coding improvements too, but this commit
includes (I hope) no behavioral change.

Nonetheless, back-patch as far as 9.6, because a followup bug-fixing
commit depends on this.

Discussion: https://postgr.es/m/16345-2e0cf5cddbdcd3b4@postgresql.org

src/backend/tsearch/wparser_def.c

index 898466fcefd64cae2845884c9a8126b4c6f38336..9a351ac9da4cce53c09a4cde32871c48aecbec06 100644 (file)
@@ -1915,6 +1915,12 @@ prsd_end(PG_FUNCTION_ARGS)
        PG_RETURN_VOID();
 }
 
+
+/*
+ * ts_headline support begins here
+ */
+
+/* token type classification macros */
 #define LEAVETOKEN(x)  ( (x)==SPACE )
 #define COMPLEXTOKEN(x) ( (x)==URL_T || (x)==NUMHWORD || (x)==ASCIIHWORD || (x)==HWORD )
 #define ENDPUNCTOKEN(x) ( (x)==SPACE )
@@ -1926,23 +1932,54 @@ prsd_end(PG_FUNCTION_ARGS)
 #define NONWORDTOKEN(x) ( (x)==SPACE || HLIDREPLACE(x) || HLIDSKIP(x) )
 #define NOENDTOKEN(x)  ( NONWORDTOKEN(x) || (x)==SCIENTIFIC || (x)==VERSIONNUMBER || (x)==DECIMAL_T || (x)==SIGNEDINT || (x)==UNSIGNEDINT || TS_IDIGNORE(x) )
 
+/*
+ * Macros useful in headline selection.  These rely on availability of
+ * "HeadlineParsedText *prs" describing some text, and "int shortword"
+ * describing the "short word" length parameter.
+ */
+
+/* Interesting words are non-repeated search terms */
+#define INTERESTINGWORD(j) \
+       (prs->words[j].item && !prs->words[j].repeated)
+
+/* Don't want to end at a non-word or a short word */
+#define BADENDPOINT(j) \
+       (NOENDTOKEN(prs->words[j].type) || prs->words[j].len <= shortword)
+
 typedef struct
 {
+       /* one cover (well, really one fragment) for mark_hl_fragments */
+       int32           startpos;               /* fragment's starting word index */
+       int32           endpos;                 /* ending word index (inclusive) */
+       int32           poslen;                 /* number of interesting words */
+       int32           curlen;                 /* total number of words */
+       bool            chosen;                 /* chosen? */
+       bool            excluded;               /* excluded? */
+} CoverPos;
+
+typedef struct
+{
+       /* callback data for checkcondition_HL */
        HeadlineWordEntry *words;
        int                     len;
 } hlCheck;
 
+
+/*
+ * TS_execute callback for matching a tsquery operand to headline words
+ */
 static bool
 checkcondition_HL(void *opaque, QueryOperand *val, ExecPhraseData *data)
 {
-       int                     i;
        hlCheck    *checkval = (hlCheck *) opaque;
+       int                     i;
 
+       /* scan words array for marching items */
        for (i = 0; i < checkval->len; i++)
        {
                if (checkval->words[i].item == val)
                {
-                       /* don't need to find all positions */
+                       /* if data == NULL, don't need to report positions */
                        if (!data)
                                return true;
 
@@ -2038,8 +2075,14 @@ hlCover(HeadlineParsedText *prs, TSQuery query, int *p, int *q)
        return false;
 }
 
+/*
+ * Apply suitable highlight marking to words selected by headline selector
+ *
+ * The words from startpos to endpos inclusive are marked per highlightall
+ */
 static void
-mark_fragment(HeadlineParsedText *prs, int highlight, int startpos, int endpos)
+mark_fragment(HeadlineParsedText *prs, bool highlightall,
+                         int startpos, int endpos)
 {
        int                     i;
 
@@ -2047,7 +2090,7 @@ mark_fragment(HeadlineParsedText *prs, int highlight, int startpos, int endpos)
        {
                if (prs->words[i].item)
                        prs->words[i].selected = 1;
-               if (highlight == 0)
+               if (!highlightall)
                {
                        if (HLIDREPLACE(prs->words[i].type))
                                prs->words[i].replace = 1;
@@ -2064,16 +2107,15 @@ mark_fragment(HeadlineParsedText *prs, int highlight, int startpos, int endpos)
        }
 }
 
-typedef struct
-{
-       int32           startpos;
-       int32           endpos;
-       int32           poslen;
-       int32           curlen;
-       int16           in;
-       int16           excluded;
-} CoverPos;
-
+/*
+ * split a cover substring into fragments not longer than max_words
+ *
+ * At entry, *startpos and *endpos are the (remaining) bounds of the cover
+ * substring.  They are updated to hold the bounds of the next fragment.
+ *
+ * *curlen and *poslen are set to the fragment's length, in words and
+ * interesting words respectively.
+ */
 static void
 get_next_fragment(HeadlineParsedText *prs, int *startpos, int *endpos,
                                  int *curlen, int *poslen, int max_words)
@@ -2081,17 +2123,17 @@ get_next_fragment(HeadlineParsedText *prs, int *startpos, int *endpos,
        int                     i;
 
        /*
-        * Objective: Generate a fragment of words between startpos and endpos
-        * such that it has at most max_words and both ends has query words. If
-        * the startpos and endpos are the endpoints of the cover and the cover
-        * has fewer words than max_words, then this function should just return
-        * the cover
+        * Objective: select a fragment of words between startpos and endpos such
+        * that it has at most max_words and both ends have query words. If the
+        * startpos and endpos are the endpoints of the cover and the cover has
+        * fewer words than max_words, then this function should just return the
+        * cover
         */
        /* first move startpos to an item */
        for (i = *startpos; i <= *endpos; i++)
        {
                *startpos = i;
-               if (prs->words[i].item && !prs->words[i].repeated)
+               if (INTERESTINGWORD(i))
                        break;
        }
        /* cut endpos to have only max_words */
@@ -2101,7 +2143,7 @@ get_next_fragment(HeadlineParsedText *prs, int *startpos, int *endpos,
        {
                if (!NONWORDTOKEN(prs->words[i].type))
                        *curlen += 1;
-               if (prs->words[i].item && !prs->words[i].repeated)
+               if (INTERESTINGWORD(i))
                        *poslen += 1;
        }
        /* if the cover was cut then move back endpos to a query item */
@@ -2111,7 +2153,7 @@ get_next_fragment(HeadlineParsedText *prs, int *startpos, int *endpos,
                for (i = *endpos; i >= *startpos; i--)
                {
                        *endpos = i;
-                       if (prs->words[i].item && !prs->words[i].repeated)
+                       if (INTERESTINGWORD(i))
                                break;
                        if (!NONWORDTOKEN(prs->words[i].type))
                                *curlen -= 1;
@@ -2119,8 +2161,14 @@ get_next_fragment(HeadlineParsedText *prs, int *startpos, int *endpos,
        }
 }
 
+/*
+ * Headline selector used when MaxFragments > 0
+ *
+ * Note: in this mode, highlightall is disregarded for phrase selection;
+ * it only controls presentation details.
+ */
 static void
-mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
+mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, bool highlightall,
                                  int shortword, int min_words,
                                  int max_words, int max_fragments)
 {
@@ -2156,7 +2204,7 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
 
                /*
                 * Break the cover into smaller fragments such that each fragment has
-                * at most max_words. Also ensure that each end of the fragment is a
+                * at most max_words. Also ensure that each end of each fragment is a
                 * query word. This will allow us to stretch the fragment in either
                 * direction
                 */
@@ -2173,12 +2221,13 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
                        covers[numcovers].endpos = endpos;
                        covers[numcovers].curlen = curlen;
                        covers[numcovers].poslen = poslen;
-                       covers[numcovers].in = 0;
-                       covers[numcovers].excluded = 0;
+                       covers[numcovers].chosen = false;
+                       covers[numcovers].excluded = false;
                        numcovers++;
                        startpos = endpos + 1;
                        endpos = q;
                }
+
                /* move p to generate the next cover */
                p++;
        }
@@ -2196,9 +2245,10 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
                 */
                for (i = 0; i < numcovers; i++)
                {
-                       if (!covers[i].in && !covers[i].excluded &&
-                               (maxitems < covers[i].poslen || (maxitems == covers[i].poslen
-                                                                                                && minwords > covers[i].curlen)))
+                       if (!covers[i].chosen && !covers[i].excluded &&
+                               (maxitems < covers[i].poslen ||
+                                (maxitems == covers[i].poslen &&
+                                 minwords > covers[i].curlen)))
                        {
                                maxitems = covers[i].poslen;
                                minwords = covers[i].curlen;
@@ -2208,7 +2258,7 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
                /* if a cover was found mark it */
                if (minI >= 0)
                {
-                       covers[minI].in = 1;
+                       covers[minI].chosen = true;
                        /* adjust the size of cover */
                        startpos = covers[minI].startpos;
                        endpos = covers[minI].endpos;
@@ -2235,8 +2285,8 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
                                        }
                                        posmarker = i;
                                }
-                               /* cut back startpos till we find a non short token */
-                               for (i = posmarker; i < startpos && (NOENDTOKEN(prs->words[i].type) || prs->words[i].len <= shortword); i++)
+                               /* cut back startpos till we find a good endpoint */
+                               for (i = posmarker; i < startpos && BADENDPOINT(i); i++)
                                {
                                        if (!NONWORDTOKEN(prs->words[i].type))
                                                curlen--;
@@ -2250,8 +2300,8 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
                                                curlen++;
                                        posmarker = i;
                                }
-                               /* cut back endpos till we find a non-short token */
-                               for (i = posmarker; i > endpos && (NOENDTOKEN(prs->words[i].type) || prs->words[i].len <= shortword); i--)
+                               /* cut back endpos till we find a good endpoint */
+                               for (i = posmarker; i > endpos && BADENDPOINT(i); i--)
                                {
                                        if (!NONWORDTOKEN(prs->words[i].type))
                                                curlen--;
@@ -2262,20 +2312,24 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
                        covers[minI].endpos = endpos;
                        covers[minI].curlen = curlen;
                        /* Mark the chosen fragments (covers) */
-                       mark_fragment(prs, highlight, startpos, endpos);
+                       mark_fragment(prs, highlightall, startpos, endpos);
                        num_f++;
                        /* exclude overlapping covers */
                        for (i = 0; i < numcovers; i++)
                        {
-                               if (i != minI && ((covers[i].startpos >= covers[minI].startpos && covers[i].startpos <= covers[minI].endpos) || (covers[i].endpos >= covers[minI].startpos && covers[i].endpos <= covers[minI].endpos)))
-                                       covers[i].excluded = 1;
+                               if (i != minI &&
+                                       ((covers[i].startpos >= covers[minI].startpos &&
+                                         covers[i].startpos <= covers[minI].endpos) ||
+                                        (covers[i].endpos >= covers[minI].startpos &&
+                                         covers[i].endpos <= covers[minI].endpos)))
+                                       covers[i].excluded = true;
                        }
                }
                else
                        break;
        }
 
-       /* show at least min_words we have not marked anything */
+       /* show at least min_words if we have not marked anything */
        if (num_f <= 0)
        {
                startpos = endpos = curlen = 0;
@@ -2285,13 +2339,17 @@ mark_hl_fragments(HeadlineParsedText *prs, TSQuery query, int highlight,
                                curlen++;
                        endpos = i;
                }
-               mark_fragment(prs, highlight, startpos, endpos);
+               mark_fragment(prs, highlightall, startpos, endpos);
        }
+
        pfree(covers);
 }
 
+/*
+ * Headline selector used when MaxFragments == 0
+ */
 static void
-mark_hl_words(HeadlineParsedText *prs, TSQuery query, int highlight,
+mark_hl_words(HeadlineParsedText *prs, TSQuery query, bool highlightall,
                          int shortword, int min_words, int max_words)
 {
        int                     p = 0,
@@ -2299,66 +2357,81 @@ mark_hl_words(HeadlineParsedText *prs, TSQuery query, int highlight,
        int                     bestb = -1,
                                beste = -1;
        int                     bestlen = -1;
-       int                     pose = 0,
+       int                     pose,
                                posb,
                                poslen,
                                curlen;
 
        int                     i;
 
-       if (highlight == 0)
+       if (!highlightall)
        {
+               /* examine all covers, select a headline using the best one */
                while (hlCover(prs, query, &p, &q))
                {
-                       /* find cover len in words */
+                       /*
+                        * Count words (curlen) and interesting words (poslen) within
+                        * cover, but stop once we reach max_words.  This step doesn't
+                        * consider whether that's a good stopping point.  posb and pose
+                        * are set to the start and end indexes of the possible headline.
+                        */
                        curlen = 0;
                        poslen = 0;
+                       posb = pose = p;
                        for (i = p; i <= q && curlen < max_words; i++)
                        {
                                if (!NONWORDTOKEN(prs->words[i].type))
                                        curlen++;
-                               if (prs->words[i].item && !prs->words[i].repeated)
+                               if (INTERESTINGWORD(i))
                                        poslen++;
                                pose = i;
                        }
 
-                       if (poslen < bestlen && !(NOENDTOKEN(prs->words[beste].type) || prs->words[beste].len <= shortword))
+                       /* XXX this optimization seems unnecessary and wrong */
+                       if (poslen < bestlen && !BADENDPOINT(beste))
                        {
-                               /* best already found, so try one more cover */
+                               /* better cover already found, so try next cover */
                                p++;
                                continue;
                        }
 
-                       posb = p;
                        if (curlen < max_words)
-                       {                                       /* find good end */
+                       {
+                               /*
+                                * We have room to lengthen the headline, so search forward
+                                * until it's full or we find a good stopping point.  We'll
+                                * reconsider the word at "q", then move forward.
+                                */
                                for (i = i - 1; i < prs->curwords && curlen < max_words; i++)
                                {
-                                       if (i != q)
+                                       if (i > q)
                                        {
                                                if (!NONWORDTOKEN(prs->words[i].type))
                                                        curlen++;
-                                               if (prs->words[i].item && !prs->words[i].repeated)
+                                               if (INTERESTINGWORD(i))
                                                        poslen++;
                                        }
                                        pose = i;
-                                       if (NOENDTOKEN(prs->words[i].type) || prs->words[i].len <= shortword)
+                                       if (BADENDPOINT(i))
                                                continue;
                                        if (curlen >= min_words)
                                                break;
                                }
-                               if (curlen < min_words && i >= prs->curwords)
-                               {                               /* got end of text and our cover is shorter
-                                                                * than min_words */
+                               if (curlen < min_words)
+                               {
+                                       /*
+                                        * Reached end of text and our headline is still shorter
+                                        * than min_words, so try to extend it to the left.
+                                        */
                                        for (i = p - 1; i >= 0; i--)
                                        {
                                                if (!NONWORDTOKEN(prs->words[i].type))
                                                        curlen++;
-                                               if (prs->words[i].item && !prs->words[i].repeated)
+                                               if (INTERESTINGWORD(i))
                                                        poslen++;
                                                if (curlen >= max_words)
                                                        break;
-                                               if (NOENDTOKEN(prs->words[i].type) || prs->words[i].len <= shortword)
+                                               if (BADENDPOINT(i))
                                                        continue;
                                                if (curlen >= min_words)
                                                        break;
@@ -2367,34 +2440,48 @@ mark_hl_words(HeadlineParsedText *prs, TSQuery query, int highlight,
                                }
                        }
                        else
-                       {                                       /* shorter cover :((( */
+                       {
+                               /*
+                                * Can't make headline longer, so consider making it shorter
+                                * if needed to avoid a bad endpoint.
+                                */
                                if (i > q)
                                        i = q;
                                for (; curlen > min_words; i--)
                                {
                                        if (!NONWORDTOKEN(prs->words[i].type))
                                                curlen--;
-                                       if (prs->words[i].item && !prs->words[i].repeated)
+                                       if (INTERESTINGWORD(i))
                                                poslen--;
                                        pose = i;
-                                       if (NOENDTOKEN(prs->words[i].type) || prs->words[i].len <= shortword)
-                                               continue;
-                                       break;
+                                       if (!BADENDPOINT(i))
+                                               break;
                                }
                        }
 
-                       if (bestlen < 0 || (poslen > bestlen && !(NOENDTOKEN(prs->words[pose].type) || prs->words[pose].len <= shortword)) ||
-                               (bestlen >= 0 && !(NOENDTOKEN(prs->words[pose].type) || prs->words[pose].len <= shortword) &&
-                                (NOENDTOKEN(prs->words[beste].type) || prs->words[beste].len <= shortword)))
+                       /*
+                        * Adopt this headline if it's the first, or if it has more
+                        * interesting words and isn't ending at a bad endpoint, or if it
+                        * replaces a bad endpoint with a good one (XXX even if it has
+                        * fewer interesting words?  Really?)
+                        */
+                       if (bestlen < 0 ||
+                               (poslen > bestlen && !BADENDPOINT(pose)) ||
+                               (!BADENDPOINT(pose) && BADENDPOINT(beste)))
                        {
                                bestb = posb;
                                beste = pose;
                                bestlen = poslen;
                        }
 
+                       /* move p to generate the next cover */
                        p++;
                }
 
+               /*
+                * If we found nothing acceptable, select min_words words starting at
+                * the beginning.
+                */
                if (bestlen < 0)
                {
                        curlen = 0;
@@ -2410,32 +2497,17 @@ mark_hl_words(HeadlineParsedText *prs, TSQuery query, int highlight,
        }
        else
        {
+               /* highlightall mode: headline is whole document */
                bestb = 0;
                beste = prs->curwords - 1;
        }
 
-       for (i = bestb; i <= beste; i++)
-       {
-               if (prs->words[i].item)
-                       prs->words[i].selected = 1;
-               if (highlight == 0)
-               {
-                       if (HLIDREPLACE(prs->words[i].type))
-                               prs->words[i].replace = 1;
-                       else if (HLIDSKIP(prs->words[i].type))
-                               prs->words[i].skip = 1;
-               }
-               else
-               {
-                       if (XMLHLIDSKIP(prs->words[i].type))
-                               prs->words[i].skip = 1;
-               }
-
-               prs->words[i].in = (prs->words[i].repeated) ? 0 : 1;
-       }
-
+       mark_fragment(prs, highlightall, bestb, beste);
 }
 
+/*
+ * Default parser's prsheadline function
+ */
 Datum
 prsd_headline(PG_FUNCTION_ARGS)
 {
@@ -2443,17 +2515,18 @@ prsd_headline(PG_FUNCTION_ARGS)
        List       *prsoptions = (List *) PG_GETARG_POINTER(1);
        TSQuery         query = PG_GETARG_TSQUERY(2);
 
-       /* from opt + start and end tag */
+       /* default option values: */
        int                     min_words = 15;
        int                     max_words = 35;
        int                     shortword = 3;
        int                     max_fragments = 0;
-       int                     highlight = 0;
+       bool            highlightall = false;
        ListCell   *l;
 
-       /* config */
+       /* Extract configuration option values */
        prs->startsel = NULL;
        prs->stopsel = NULL;
+       prs->fragdelim = NULL;
        foreach(l, prsoptions)
        {
                DefElem    *defel = (DefElem *) lfirst(l);
@@ -2474,12 +2547,12 @@ prsd_headline(PG_FUNCTION_ARGS)
                else if (pg_strcasecmp(defel->defname, "FragmentDelimiter") == 0)
                        prs->fragdelim = pstrdup(val);
                else if (pg_strcasecmp(defel->defname, "HighlightAll") == 0)
-                       highlight = (pg_strcasecmp(val, "1") == 0 ||
-                                                pg_strcasecmp(val, "on") == 0 ||
-                                                pg_strcasecmp(val, "true") == 0 ||
-                                                pg_strcasecmp(val, "t") == 0 ||
-                                                pg_strcasecmp(val, "y") == 0 ||
-                                                pg_strcasecmp(val, "yes") == 0);
+                       highlightall = (pg_strcasecmp(val, "1") == 0 ||
+                                                       pg_strcasecmp(val, "on") == 0 ||
+                                                       pg_strcasecmp(val, "true") == 0 ||
+                                                       pg_strcasecmp(val, "t") == 0 ||
+                                                       pg_strcasecmp(val, "y") == 0 ||
+                                                       pg_strcasecmp(val, "yes") == 0);
                else
                        ereport(ERROR,
                                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -2487,7 +2560,8 @@ prsd_headline(PG_FUNCTION_ARGS)
                                                        defel->defname)));
        }
 
-       if (highlight == 0)
+       /* in HighlightAll mode these parameters are ignored */
+       if (!highlightall)
        {
                if (min_words >= max_words)
                        ereport(ERROR,
@@ -2507,18 +2581,23 @@ prsd_headline(PG_FUNCTION_ARGS)
                                         errmsg("MaxFragments should be >= 0")));
        }
 
+       /* Apply appropriate headline selector */
        if (max_fragments == 0)
-               /* call the default headline generator */
-               mark_hl_words(prs, query, highlight, shortword, min_words, max_words);
+               mark_hl_words(prs, query, highlightall, shortword,
+                                         min_words, max_words);
        else
-               mark_hl_fragments(prs, query, highlight, shortword, min_words, max_words, max_fragments);
+               mark_hl_fragments(prs, query, highlightall, shortword,
+                                                 min_words, max_words, max_fragments);
 
+       /* Fill in default values for string options */
        if (!prs->startsel)
                prs->startsel = pstrdup("<b>");
        if (!prs->stopsel)
                prs->stopsel = pstrdup("</b>");
        if (!prs->fragdelim)
                prs->fragdelim = pstrdup(" ... ");
+
+       /* Caller will need these lengths, too */
        prs->startsellen = strlen(prs->startsel);
        prs->stopsellen = strlen(prs->stopsel);
        prs->fragdelimlen = strlen(prs->fragdelim);