Re: [PATCH] Revive line type

Hi,

Test results are as follows:

Contents & Purpose

This patch is for finishing the line type and related functions that not
done yet but listed in catalogs and documentation. There are no other new
features added in this patch.

The regression test cases which included in this patch, Documentation are
also updated.

Run

Regression tests are all succeed, but several problems have be found while
ding some simple test. The updated document said that the points used in
the output are not necessarily the points used on input. I understand that
as long as they are marked on the same line. But when the input data
represents a horizontal or vertical line, the output is not exactly the
same line. It is another line parallel to it.

For example:

postgres=# select line('1,3,2,3');

line

-----------------

[(0,-3),(1,-3)]

(1 row)

postgres=# select line('1,3,1,6');

line

-----------------

[(-1,0),(-1,1)]

(1 row)

In addition, when a straight line coincides with coordinate axis, output
appears -0, I do not know whether it is appropriate.

postgres=# select line('0,1,0,5');

line

-----------------

[(-0,0),(-0,1)]

(1 row)

Negative value appeared when use <-> to calculate the distance between two
parallel lines.

postgres=# select line('1,1,2,1') <-> line('-1,-1,-2,-1') ;

?column?

----------

-2

postgres=# select lseg('1,1,2,1') <-> line('-1,-1,-2,-1') ;

?column?

----------

-2

(1 row)

The same situation occurs in distance calculation between point and a
straight line.

postgres=# select point('-1,1') <-> line('-3,0,-4,0') ;

?column?

----------

-1

(1 row)

Should the distance be positive numbers?

Other functions seem work properly.

Performance

==================================

Because these functions is first implemented. So there is no relatively
comparison for the performance.

Conclusion

This patch lets line type worked. But there are some bugs. Additionally,
function "close_sl" not implemented.

With Regards,

Rui

On Sun, 2013-06-23 at 12:24 +0800, rui hua wrote:
> Regression tests are all succeed, but several problems have be found while
> ding some simple test. The updated document said that the points used in
> the output are not necessarily the points used on input. I understand that
> as long as they are marked on the same line. But when the input data
> represents a horizontal or vertical line, the output is not exactly the
> same line. It is another line parallel to it.

> postgres=# select line('1,3,2,3');
> line
> -----------------
> [(0,-3),(1,-3)]
> (1 row)

This was just using the wrong sign. Fixed.

> In addition, when a straight line coincides with coordinate axis, output
> appears -0, I do not know whether it is appropriate.

This is a matter of the floating-point operations. I don't think it's
on scope to worry about that. With the above fix, the cases you pointed
out go away anyway.

> Negative value appeared when use <-> to calculate the distance between two
> parallel lines.

Fixed with fabs().

New patch included. Still wondering whether to use a A,B,C-based output
format per Tom's comment.

On 6/26/13 9:34 PM, Peter Eisentraut wrote:
> Still wondering whether to use a A,B,C-based output
> format per Tom's comment.

Wouldn't it also be helpful to remove "The points used in the output are
not necessarily the points used on input" by making that not true?

There are three obvious ways you might output a line:

-Math class expectations of slope-intercept form: y = mx + b.
Intercept forms don't handle both horizontal and vertical lines though,
so that's easily out.

-Pair of points. A casual observer might get lucky and observe putting
a pair of points in and getting the same pair back again, then
incorrectly assume that's normally the case. Seems better to never make
that possible in the infinite line case. I'm reminded of how row output
usually is in order gives a bad impression about ordering guarantees in SQL.

-Ax+By+C=0

Outputting that third one, when it's also the internal form, handles any
time of line; will show any assumptions about individual points being
preserved are wrong; and avoids rounding errors too. The only downside
seems to be that bounded lines are easier to show with a pair of points.
I think that suggests an alternate, secondary output format would be
useful though, rather than that a different default one is a good idea.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

Here is a new patch for the line type, with a new input/output format
{A,B,C}, as discussed in this thread.

Peter Eisentraut escribió:
> Here is a new patch for the line type, with a new input/output format
> {A,B,C}, as discussed in this thread.

I gave this a quick look. The new input/output format appears to work
well. The input format using two points still works, which is nice.
Regression tests pass. I tried binary input and output using COPY and
it seems to round-trip without problem.

With the two-point input, horizontal and vertical lines can accept NaNs
without producing a NaN in the output if not necessary.

I tried "interpt" and "intersect" functions and they seem to work.
Haven't gotten around to trying other functions or operators.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Sep 16, 2013 at 07:04:32PM -0300, Alvaro Herrera wrote:
> Peter Eisentraut escribió:
> > Here is a new patch for the line type, with a new input/output format
> > {A,B,C}, as discussed in this thread.
>
> I gave this a quick look. The new input/output format appears to work
> well. The input format using two points still works, which is nice.
> Regression tests pass. I tried binary input and output using COPY and
> it seems to round-trip without problem.
>
> With the two-point input, horizontal and vertical lines can accept NaNs
> without producing a NaN in the output if not necessary.
>
> I tried "interpt" and "intersect" functions and they seem to work.
> Haven't gotten around to trying other functions or operators.

Should the things you tried and others be in the regression tests? If
so, should we start with whatever had been in the regression tests
when the line type was dropped?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter escribió:

> Should the things you tried and others be in the regression tests? If
> so, should we start with whatever had been in the regression tests
> when the line type was dropped?

Actually, the patch does include a regression test for the revived type
(and it passes). I don't think more than that is needed.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

I had a look over this patch and here are my review points:

1. Patch applies cleanly.
2. make, make install and make check is good.
3. I did lot of random testing and didn't find any issue.
4. Test coverage is very well. It has all scenarios and all operators are
tested with line. That's really great.

So no issues from my side.

However, do we still need this in close_pl() ?

#ifdef NOT_USED
if (FPeq(line->A, -1.0) && FPzero(line->B))
{ /* vertical */
}
#endif

Also close_sl, close_lb and dist_lb are NOT yet implemented. It will be good
if we have those. But I don't think we should wait for those functions to be
implemented. We can go ahead with this. Please confirm above concern so that
I will mark it as "Ready for Committer".

Thanks

On Tue, Sep 17, 2013 at 7:34 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> David Fetter escribió:
>
> > Should the things you tried and others be in the regression tests? If
> > so, should we start with whatever had been in the regression tests
> > when the line type was dropped?
>
> Actually, the patch does include a regression test for the revived type
> (and it passes). I don't think more than that is needed.
>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation

On Wed, 2013-09-25 at 14:26 +0530, Jeevan Chalke wrote:
> So no issues from my side.
>
> However, do we still need this in close_pl() ?
>
> #ifdef NOT_USED
> if (FPeq(line->A, -1.0) && FPzero(line->B))
> { /* vertical */
> }
> #endif

No, that can be removed.

> Also close_sl, close_lb and dist_lb are NOT yet implemented. It will
> be good if we have those. But I don't think we should wait for those
> functions to be implemented.

Right, those are separate projects.

On Wed, Oct 2, 2013 at 6:12 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On Wed, 2013-09-25 at 14:26 +0530, Jeevan Chalke wrote:
> > So no issues from my side.
> >
> > However, do we still need this in close_pl() ?
> >
> > #ifdef NOT_USED
> > if (FPeq(line->A, -1.0) && FPzero(line->B))
> > { /* vertical */
> > }
> > #endif
>
> No, that can be removed.
>

Will you please attach new patch with above block removed ? Then I will
quickly check that new patch and mark as "Ready For Committer".

>
> > Also close_sl, close_lb and dist_lb are NOT yet implemented. It will
> > be good if we have those. But I don't think we should wait for those
> > functions to be implemented.
>
> Right, those are separate projects.
>

Agree.

Thanks
--
Jeevan B Chalke

On Thu, 2013-10-03 at 17:50 +0530, Jeevan Chalke wrote:
> Will you please attach new patch with above block removed ? Then I
> will quickly check that new patch and mark as "Ready For Committer".
>
Here you go.

On Wed, Oct 9, 2013 at 10:44 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On Thu, 2013-10-03 at 17:50 +0530, Jeevan Chalke wrote:
> > Will you please attach new patch with above block removed ? Then I
> > will quickly check that new patch and mark as "Ready For Committer".
> >
> Here you go.
>
>
Thanks Peter. Patch looks good to me and ready to go in now.
Marking as "Ready For Committer".

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation