Re: [HACKERS] Typo Patch
CharSyam wrote: > Yes, Some typos problems, It is hard to figure them out. > But in this patch, guranteed is just misspelling of guaranteed. I agree with Robert: the sentence as a whole makes no sense: * We need to check all possible distances, so reset Lpos * to guaranteed not yet satisfied position. It's hard to justify changing a single character when what we should be doing is rewriting the comment completely. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo Patch
On Tue, Jul 5, 2016 at 11:54 AM, Robert Haas wrote:
> On Sat, Jul 2, 2016 at 12:17 PM, CharSyam wrote:
> > I fixed typos. and attached patch for this.
> > Thanks.
> >
> > I only changed comments only in src/backend/utils/adt/tsvector_op.c
>
> Well, that's definitely a typo. However, typo or no typo, it's hard
> for me to figure out with any certainty what that sentence actually
> means.
>
Given that "curitem->qoperator.distance" is constant.
Also, both loops go from low to high position (over the same scale) with
the inner loop stopping as soon as it moves beyond the position of the
outer loop - namely that the left/inner item is always positioned to the
left of the right/outer operand within the document being search.
Regardless of whether there is a match at this meeting point increasing the
outer loop's position will not cause any of the previously checked (at the
lower positions) inner loop items to match where they did not before. I
say this but I'm concerned that for sufficiently large values of
curitem->qoperator.distance a given left operand could match multiple right
operands since the distance is a maximum extent.
Thus, in the case of a match the current Lpos needs to be checked again
during the subsequent iterator of the outer loop.
The "else if (Rpos - Lpos < distance) break" block though is oddly
commented/written since distance is a maximum - shouldn't this be treated
as a match as well?
Since, for sufficiently large values of "distance", a single left operand
could cause multiple right operands to be matched when the less-than
condition matches we need to leave Lpos alone and try the next Rpos against
it. For the greater than (default) and the equal cases we can skip
rechecking the current Lpos.
The comment in question can be removed - we're not actually resetting
anything here. The use of LposStart is not really needed - just make sure
to leave Lpos in the correct position (i.e. optional increment on all
paths) and the inner while loop will do the correct thing.
It seems a bit odd to be keying off of the RIGHT operand and returning its
position when left-to-right languages would consider the position of the
leading word to be reported.
Code Comment Suggestion:
For each physically positioned right-side operand iterate over each
instance of the left-side operand to locate one within the specified
distance. As soon as a match is found move onto the next right-operand and
continue searching starting with the last checked left-side operand. Note
that for an exact distance match the current left-side operand can be
skipped over.
For some graphical imagery consider how a Slinky operates. Hold the left
side, move the right side, release the left and let it collapse; repeat.
In this case, though, the collapsing can be stopped mid-stream since the
distance check has an inequality.
The following shouldn't be taken as an actual patch submission but rather a
codification of the above.
diff --git a/src/backend/utils/adt/tsvector_op.c
b/src/backend/utils/adt/tsvector_op.c
index 242b7e1..fefaca5 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -1375,7 +1375,6 @@ TS_phrase_execute(QueryItem *curitem,
ExecPhraseData Ldata = {0, false, NULL},
Rdata = {0, false, NULL};
WordEntryPos *Lpos,
- *LposStart,
*Rpos,
*pos_iter = NULL;
@@ -1423,15 +1422,17 @@ TS_phrase_execute(QueryItem *curitem,
* ExecPhraseData->data can point to the tsvector's WordEntryPosVector
*/
+ /*
+ * For each physically positioned right-side operand iterate over each
+ * instance of the left-side operand to locate one within the specified
+ * distance. As soon as a match is found move onto the next right-operand
+ * and continue searching starting with the last checked left-side
operand.
+ * Note that for an exact distance match the current left-side operand
+ * can be skipped over.
+ */
Rpos = Rdata.pos;
- LposStart = Ldata.pos;
while (Rpos < Rdata.pos + Rdata.npos)
{
- /*
- * We need to check all possible distances, so reset Lpos
- * to guranteed not yet satisfied position.
- */
- Lpos = LposStart;
while (Lpos < Ldata.pos + Ldata.npos)
{
if (WEP_GETPOS(*Rpos) - WEP_GETPOS(*Lpos) ==
@@ -1449,7 +1450,7 @@ TS_phrase_execute(QueryItem *curitem,
* could not satisfy distance for any other right
* position
*/
- LposStart = Lpos + 1;
+ Lpos++
break;
}
else
@@ -1462,16 +1463,26 @@ TS_phrase_execute(QueryItem *curitem,
}
}
- else if (WEP_GETPOS(*Rpos) <= WEP_GETPOS(*Lpos) ||
- WEP_GETPOS(*Rpos) - WEP_GETPOS(*Lpos) <
+ else if (WEP_GETPOS(*Rpos) <= WEP_GETPOS(*Lpos)
+ {
+ /*
+ * No Increment - we are beyond the current right operand
+ * so its possible this left operand could match the
next right
+ * operand.
+ */
+ break;
+ }
+ else if (WEP_GETPOS(*Rpos) - WEP_GETPOS(*Lpos) <
curitem->qoperator.distance)
{
+ /* THIS SHOULD BE A MATCH? */
/*
- * Go to the n
Re: [HACKERS] Typo Patch
Yes, Some typos problems, It is hard to figure them out. But in this patch, guranteed is just misspelling of guaranteed. Thanks. 2016-07-06 0:54 GMT+09:00 Robert Haas : > On Sat, Jul 2, 2016 at 12:17 PM, CharSyam wrote: > > I fixed typos. and attached patch for this. > > Thanks. > > > > I only changed comments only in src/backend/utils/adt/tsvector_op.c > > Well, that's definitely a typo. However, typo or no typo, it's hard > for me to figure out with any certainty what that sentence actually > means. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [HACKERS] Typo Patch
On Sat, Jul 2, 2016 at 12:17 PM, CharSyam wrote: > I fixed typos. and attached patch for this. > Thanks. > > I only changed comments only in src/backend/utils/adt/tsvector_op.c Well, that's definitely a typo. However, typo or no typo, it's hard for me to figure out with any certainty what that sentence actually means. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo patch
Tom Lane wrote: > Heikki Linnakangas writes: > > Thanks, committed. Except for this one: > > > - * *Only* a frozen-for-read tape can be seeked. > > + * *Only* a frozen-for-read tape can be sought. > > > It's true that the past tense of "seek" is "sought", but it feels a bit > > weird to me in this context. This is a comment on a function called > > "seek", and it's not clear to me that it should conjugate > > like the normal "seek" verb. > > I agree that "sought" is not the word to use here, but the existing > wording isn't very good English either. Perhaps "Seeking is only allowed > on frozen-for-read tapes"? It would be great if you could find a solution for words such as "shutdowned" or "backuped". (I don't know that we use the latter in our code, but I've seen it around. The former we even have in an enum somewhere.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo patch
Heikki Linnakangas writes: > Thanks, committed. Except for this one: > - * *Only* a frozen-for-read tape can be seeked. > + * *Only* a frozen-for-read tape can be sought. > It's true that the past tense of "seek" is "sought", but it feels a bit > weird to me in this context. This is a comment on a function called > "seek", and it's not clear to me that it should conjugate > like the normal "seek" verb. I agree that "sought" is not the word to use here, but the existing wording isn't very good English either. Perhaps "Seeking is only allowed on frozen-for-read tapes"? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo patch
On 05/20/2015 07:29 PM, CharSyam wrote: Hi, I changed typos error. and attached patch for this. Thanks you. I only changed comments only Thanks, committed. Except for this one: --- src/backend/utils/sort/logtape.c +++ src/backend/utils/sort/logtape.c @@ -926,7 +926,7 @@ LogicalTapeBackspace(LogicalTapeSet *lts, int tapenum, size_t size) /* * Seek to an arbitrary position in a logical tape. * - * *Only* a frozen-for-read tape can be seeked. + * *Only* a frozen-for-read tape can be sought. * * Return value is TRUE if seek successful, FALSE if there isn't that much * data in the tape (in which case there's no state change). It's true that the past tense of "seek" is "sought", but it feels a bit weird to me in this context. This is a comment on a function called "seek", and it's not clear to me that it should conjugate like the normal "seek" verb. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo patch
Thanks :) You make sense. 2015-05-21 1:49 GMT+09:00 Heikki Linnakangas : > On 05/20/2015 07:29 PM, CharSyam wrote: > >> Hi, >> >> I changed typos error. and attached patch for this. >> Thanks you. >> >> I only changed comments only >> > > Thanks, committed. Except for this one: > > --- src/backend/utils/sort/logtape.c > +++ src/backend/utils/sort/logtape.c > @@ -926,7 +926,7 @@ LogicalTapeBackspace(LogicalTapeSet *lts, int tapenum, > size_t size) > /* > * Seek to an arbitrary position in a logical tape. > * > - * *Only* a frozen-for-read tape can be seeked. > + * *Only* a frozen-for-read tape can be sought. > * > * Return value is TRUE if seek successful, FALSE if there isn't that much > * data in the tape (in which case there's no state change). > > It's true that the past tense of "seek" is "sought", but it feels a bit > weird to me in this context. This is a comment on a function called " blah>seek", and it's not clear to me that it should conjugate like the > normal "seek" verb. > > - Heikki > >
Re: [HACKERS] Typo patch
Thomas Hunger writes: > Unless I misunderstood the documentation I think this fixes a typo. Thanks :) Yeah, that should be consistent with the syntax summary. Applied, thanks. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
