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

ADQL parsing error on: distance(centroid(s_region),point('',187.48, 2.05)) #19

Closed
almicol opened this issue Feb 20, 2017 · 12 comments
Closed

Comments

@almicol
Copy link

almicol commented Feb 20, 2017

Hi Gregory,
A query like:

SELECT top 10 centroid(s_region), distance(centroid(s_region),point('', 187.48, 2.05))
FROM ivoa.ObsCore

returns the error:
Incorrect ADQL query: Encountered "centroid". Was expecting one of: "POINT" """ <REGULAR_IDENTIFIER> "POINT" <REGULAR_IDENTIFIER> """

Though, CENTROID does return a POINT.

Using distance( cast(centroid(s_region) as POINT), point('', 187.48, 2.05) ) does not help.
Hope you could have a look.
Many thanks in advance,
Alberto

@vforchi
Copy link
Contributor

vforchi commented Feb 20, 2017

For the record, this happens on our implementation, that uses a SQLServer backend, but the error comes from the parser, that expects either a Point or a Column as argument of the DISTANCE operator.

@gmantele
Copy link
Owner

Yes, exactly. The parser is accepting only a POINT or a column reference as parameter of DISTANCE. This is actually how it is described by the ADQL's standard (v2.0). I agree that allowing CENTROID in parameter of DISTANCE would make perfectly sense otherwise.

I could easily change this behaviour, but my parser would not follow anymore the standard. In theory it is not a real big deal, but TOPCAT is using it in its ADQL editor.

Maybe it could be possible to make this part of the next version of ADQL (v2.1).

@gmantele gmantele self-assigned this Feb 20, 2017
@gmantele
Copy link
Owner

A way to get round this grammar issue would be with the following ADQL query:

SELECT c, distance(c, point('', 187.48, 2.05))
FROM (SELECT TOP 10 centroid(s_region) AS c FROM ivoa.ObsCore) AS trick

Ugly, but it should do what is expected.

@vforchi
Copy link
Contributor

vforchi commented Feb 20, 2017

ADQL 2.1 already specifies that DISTANCE works between two geometries:

The specification defines two versions of the DISTANCE function, one that accepts two geometries, and one that accepts four separate numeric values.

@gmantele
Copy link
Owner

Not according to the following:

The DISTANCE function computes the arc length along a great circle
between two points and returns a numeric value expression in degrees.

And also to the grammar definition:

<distance_function> ::=
DISTANCE <left_paren>
<coord_value> <comma>
<coord_value>
<right_paren>
| DISTANCE <left_paren>
<numeric_value_expression> <comma>
<numeric_value_expression> <comma>
<numeric_value_expression> <comma>
<numeric_value_expression>
<right_paren>

<coord_value> ::= <point> | <column_reference>

@vforchi
Copy link
Contributor

vforchi commented Feb 20, 2017

I would say that, at the very least, the standard is not clear on this point (pun intended):

This function computes the centroid of a given geometry and returns a POINT (See 2.4.11).

By the way, the reference does not exist.

@almicol
Copy link
Author

almicol commented Feb 20, 2017

Your proposed get-around (ugly, but interesting!) query returns an error:
Incorrect ADQL query: 1 unresolved identifiers!

  • Type mismatch! A geometry was expected instead of "c".

You might want to have a look at that too.

@gmantele
Copy link
Owner

A first comment to both of you: the ADQL library is not yet adapted for ADQL 2.1. So the function CAST, for instance, can not yet work.

Then, @almicol, I admit to be a bit puzzled. This query passes the ADQL query validation on the validator of the website (http://cdsportal.u-strasbg.fr/adqltuto/validator.html) which is using the last stable version, and passes also the validation on the new on-development version on my computer.

  • Are you using a special version? If yes, which one?
  • Have you changed a bit the parser or any related class?

@vforchi
Copy link
Contributor

vforchi commented Feb 20, 2017

Agreed, it's not in 2.0, I was quoting 2.1 only because you said it could be included in 2.1, but the current draft is not very clear.

We are using my fork, where I added the support for the geometry in SQLServer.
I didn't change the parser, but I modified a little bit the translator. I'll check if I introduced the error.

For the record, I forked from the trunk, not from the stable version, so it may be something there.

@vforchi
Copy link
Contributor

vforchi commented Feb 20, 2017

It looks like you changed this method in ADQLColumn

@Override
public boolean isGeometry(){
   return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isGeometry() || (dbLink.getDatatype().isUnknown() && !dbLink.getDatatype().isNumeric()));
}

from:

@Override
public boolean isGeometry(){
   return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isGeometry());
}

@gmantele
Copy link
Owner

Indeed, but it is actually not the problem. It turns out that I declared the wrong return type for CENTROID in CentroidFunction ; it was declared as a numeric instead of a geometry. After having fixed that, the error disappeared (but another occurred: bad translation of CENTROID into PgSphere ; but that should not bother you). So, that should fix the problem on your side too. Otherwise, tell me.

To get this correction, either you update your fork with the last version of my repository, or you apply the correction by yourself after having read my last commit. I recommend you the first solution because it will avoid you merge issues later.


By the way, I could also import your SQLServer translation corrections (atan2 and TOP/DISTINCT) in my repository ; you can send a PullRequest if you want. Otherwise, I will probably apply the same corrections later on my side.

@gmantele
Copy link
Owner

The grammar issue about the function DISTANCE is reported on ivoa/lyonetia#6 and may be fixed in the next version of the ADQL standard (v2.1).

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

No branches or pull requests

3 participants