Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grammar fix for CENTROID #5

Open
Zarquan opened this issue Sep 7, 2016 · 0 comments
Open

Grammar fix for CENTROID #5

Zarquan opened this issue Sep 7, 2016 · 0 comments

Comments

@Zarquan
Copy link
Member

Zarquan commented Sep 7, 2016

From an email discussion with Markus

I had to dig into the ADQL grammar in another matter, too: CENTROID.

For one, pgSphere so far doesn't support computing centroids (except
for circles and points, which is lame), so I suspect nobody supports
this properly. I, at least, don't, and now try to give a sensible
error message.

But while digging into this I noticed back then I've quitely
sanitised the grammar, and I think we should fix this "upstream" now.

Currently, we have

<centroid> ::= CENTROID <left_paren> <geometry_value_expression> <right_paren>

<geometry_value_expression> ::=
     <value_expression_primary>
   | <geometry_value_function>

<value_expression_primary> ::=
       <unsigned_value_specification>
     | <column_reference>
     | <set_function_specification>
     | <left_paren> <value_expression> <right_paren>

So, something like CENTROID(47839) or CENTROID(COUNT(*)) is
grammatically ok, and I suspect that was not what people had in mind.
I suspect what they wanted was to allow a column reference. So, I
think what we want is

<geometry_value_expression> ::=
     <column_reference>
   | <geometry_value_function>

The other alternatives in <value_expression_primary> don't really
make sense for any place <geometry_value_expression> turns up in.

On the other hand, what I've been running all these past years was
essentially

geometryExpression = box | point | circle | polygon | region

geometryValue = columnReference.copy()

centroid = (CaselessKeyword("CENTROID")("fName")
    + '(' + Args(geometryValueExpression) + ')')

geometryValueExpression = geometryExpression | geometryValue | centroid

I can't promise I really thought the inclusion of centroid in
geometryValueExpression, but I'd support including it in
<geometry_value_expression>, too. It's probably not deadly
important, but it'd allow stuff like

1=CONTAINS(CENTROID(coverage), CIRCLE('', 10, 2, 3))

or similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant