opencadc / tap Goto Github PK

Current version is 0.9.5 (with a 0.9.6 snapshot). There have been API changes that will require some work.

The many bug fixes would be beneficial. Also, the current required version (0.6) is not in Maven.

Consider this ADQL:
select top 5 abs(s_ra) from ivoa.obscore where s_ra is not null

In Oracle, this will translate to:
select abs(s_ra) from (select abs(s_ra) from ivoa.obscore where s_ra <> null) where rownum <= 5

The inner select transformation is to allow proper handling of the clauses before the rownum takes effect. The outer abs(s_ra) will not properly be found as the inner abs(s_ra) will simply hand back s_ra. This can be solved by issuing a select all in the outer query:
select * from (select abs(s_ra) from ivoa.obscore where s_ra <> null) where rownum <= 5

SELECT from_column, target_column, key_id
FROM TAP_SCHEMA.key_columns
NATURAL JOIN TAP_SCHEMA.keys
WHERE from_table = 'caom2.ObsCoreEnumField'

With the NATURAL JOIN it should be OK to reference key_id (the natural join key); the current code results is an error: IllegalArgumentException: Column [key_id] is ambiguous.

This is running with Postgres 9.x and version 1.0.2 of the cadc-tap-service.

Consider this ADQL running on the CADC TAP service:

SELECT Plane.*
FROM caom2.Plane AS Plane 
	JOIN caom2.Observation AS Observation ON Plane.obsID = Observation.obsID 
WHERE  ( lower(Observation.observationID) LIKE '%123_%' )

This query will return everything containing the text 123, rather than searching for 123_ as requested. The underscore needs to be escaped as it has special meaning.

Printing an SDO_POINT_TYPE from an Oracle SDO_GEOMETRY object produces a NullPointerException due to the unhandled type.

The OraclePoint class assumes the arguments are Double datatypes, but could be Column references:

SELECT DISTANCE(POINT('ICRS', ra, dec),POINT('ICRS', 266.41683, -29.00781)) AS dist, * 
FROM table 
WHERE 1=CONTAINS(POINT('ICRS', ra, dec),CIRCLE('ICRS', 266.41683, -29.00781, 0.08333333)) 
ORDER BY dist ASC

The OraclePoint class should use the default Expression arguments rather than converting.

Creating an index on a non-existent column results in a null pointer exception (currently on line 235 of TableUpdateRunner.java). A check should be done before to ensure the column exists and, if it doesn't report a 400 Bad Request with a sensible message.

If a radius of 0.0 is supplied to the INTERSECTS function, the result is points that rest on a line. In some implementations that may just result in a no-overlap and return nothing, but Oracle throws an obscure error that is not for end user consumption.

The TAP Oracle library must create an SDO_GEOM object, which means the values must be a valid SDO_ARRAY of points making a circle. Those are calculated using the radius. We could:

• Supply a default radius
• Throw an error and force the caller to create a POINT instead.
• Automagically look for a POINT instead.

some packages in cadc-adql should be moved to cadc-tap-server-pg

provide a better example of how back-end specific code should be done

It's well-known that standard ADQL does not currently contain Boolean-valued expressions. It's also pretty well-known that there have been efforts to extend ADQL with them, most recently to try to allow the use of, e.g., CONTAINS( ... ) as a boolean_factor on its own, rather than as CONTAINS( ... ) = 1. However, for now this is not in the ADQL 2.1 PR and is a problem for another day.

In the mean time, when a user attempts to use a variable as a Boolean, the CADC TAP service (both Argus and in its Rubin RSP variant) produces a largely unhelpful and rather strange error. The following query:

select count(*) from caom2.Observation where obsID

produces the error message:

IllegalArgumentException:ADQL syntax error: Encountered " <S_IDENTIFIER> "obsID "" at line 1, column 47.
Was expecting one of:
    "(" ...
    "(" ...
    "(" ...
    "(" ...
    "(" ...
    "(" ...
    "(" ...
    "(" ...
    "(" ...
    "(" ...
    "(" ...

While the offending column expression is at least mentioned, the "was expecting" text is entirely unhelpful.

In the Rubin context this is a very natural thing for users to attempt, because we have many, many columns that are, in effect, Boolean flags even though they are not actually booleans in the database. We've had a user support request on this already just during the Data Previews era.

I'm hoping this is perhaps an easy thing to fix?

Given this ADQL, the S_REGION column should be replaced in the SQL with the FOOTPRINT column:

SELECT TOP 1 S_REGION FROM ivoa.obscore;

SQL Query is then:

SELECT FOOTPRINT FROM (SELECT FOOTPRINT FROM ivoa.obscore)
WHERE ROWNUM <= 1

However, if the S_REGION is mixed or lowercase, it does not get replaced:

SELECT TOP 1 s_region FROM ivoa.obscore;

SELECT s_region FROM (SELECT s_region FROM ivoa.obscore)
WHERE ROWNUM <= 1

Which will often fail trying to convert the Oracle SDO_GEOMETRY objects back into DALI strings. It's a simple equalsIgnoreCase() fix.

The maxUploadRows field is set but never used to ensure that table uploads don't exceed some arbitrary size. This is helpful to prevent excessive load on databases.

The stack trace is below. The sample query is:

SELECT DISTINCT 
TABLE1.COL1A AS "COL1A",
TABLE1.COL1B AS "COL1B",
TABLE2.COL2A AS "COL2A",
TABLE3.COL3A AS "COL3A"
FROM SCHEMA1.TABLE1
inner join SCHEMA1.TABLE2 on TABLE1.COL1A = TABLE2.COL2A
inner join SCHEMA1.TABLE3 on TABLE2.COL2B = TABLE3.COL3A
WHERE (1=1)
AND TABLE3.COL3A = 'VAL'
GROUP BY TABLE1.COL1A, TABLE1.COL1B, TABLE2.COL2A, TABLE3.COL3A
HAVING TABLE2.COL2A = 
  (SELECT MAX(T3.COL3A) AS MM FROM TABLE3 AS T3
   inner join TABLE1 as t1 on t1.COL1A = T3.COL3A
   inner join TABLE2 as t2 on t2.COL2B = t1.COL1B
   WHERE t1.COL1A = TABLE1.COL1A)
ORDER BY "COL1A" asc;

java.lang.NullPointerException
    at ca.nrc.cadc.tap.parser.navigator.FromItemNavigator.visit(FromItemNavigator.java:120)
    at ca.nrc.cadc.tap.parser.navigator.ExpressionNavigator.visit(ExpressionNavigator.java:446)
    at net.sf.jsqlparser.statement.select.SubSelect.accept(SubSelect.java:51)
    at ca.nrc.cadc.tap.parser.navigator.ExpressionNavigator.visit(ExpressionNavigator.java:340)
    at net.sf.jsqlparser.expression.operators.relational.EqualsTo.accept(EqualsTo.java:30)
    at ca.nrc.cadc.tap.parser.navigator.SelectNavigator.visit(SelectNavigator.java:201)
    at net.sf.jsqlparser.statement.select.PlainSelect.accept(PlainSelect.java:105)
    at ca.nrc.cadc.tap.parser.StatementNavigator.visit(StatementNavigator.java:105)
    at net.sf.jsqlparser.statement.select.Select.accept(Select.java:36)
    at ca.nrc.cadc.tap.parser.ParserUtil.parseStatement(ParserUtil.java:118)
    at ca.nrc.cadc.tap.AbstractTapQuery.navigateStatement(AbstractTapQuery.java:154)
    at ca.nrc.cadc.tap.SqlQuery.doNavigate(SqlQuery.java:164)

In youcat, if one tries to create an index on a column with the incorrect case it results in a null pointer exception.

TableUpdateRunner initially gets the column (description) through the table. This succeeds because TableDescription.getColumn() uses equalsIgnoreCase().

This allows the index creation to proceed, only to fail later (line 235) where a case-sensitive query for the column is executed and returns null.

When the query has completed, having the row count from the query would be useful.

Uplaoded tables with FIELD names that are not valid ADQL identifiers (eg name="My Stuff") fail because the basicUploadManager doesn't quote them properly. Quoted identifiers need to be supported for uploaded tables and in queries that reference such columns.

Motivated by a conversation on the IVOA dal mailing list, I've been looking at the handling of large values of MAXREC.

It appears that the CADC TAP code base rejects MAXREC values greater than the maximum signed 32-bit integer, i.e., 2,147,483,647 or 2^31 - 1, with a reasonable message, e.g.,

<INFO name="QUERY_STATUS" value="ERROR">Invalid MAXREC: 2147483648</INFO>

Values less than or equal to 2,147,483,646 appear to be accepted.

But if exactly 2,147,483,647 is provided, a curious error message appears:

<INFO name="QUERY_STATUS" value="ERROR">ERROR: LIMIT must not be negative</INFO>

Presumably this is because the service is using the well-known trick of supplying MAXREC+1 as a row limit to the underlying database, and then setting the overflow flag based on whether exactly MAXREC+1 rows come back. But in this case since MAXREC+1 can't be expressed in a signed 32-bit integer, it's not surprising that something weird might happen.

It's by no means an error that must be fixed -- feel free to simply close this report -- but perhaps just responding to MAXREC=2147483647 with the same "Invalid MAXREC" message would be a tiny improvement. Or you can leave it as an Easter egg for strange people like me to find.

INTERVAL(tmp.start, tmp.end) complains about types being [Column, Column] when in fact they are doubles in a temporary table:

pyvo.dal.exceptions.DALQueryError: IllegalArgumentException: Interval bounds must be double, found: Column,Column

Replacing the start and end columns in the temporary table with an interval column fixes the problem.

Cross referenced:
http://rt.cadc-ccda.hia-iha.nrc-cnrc.gc.ca/Ticket/Display.html?id=76104

The top level navigator requires three separate components that visit different parts of the query (expressions, references, and from-items)... It might be much simpler to have a single monolithic Visitor that covers all the visit() methods even if that was a large number of methods.

Issues to consider:

• re-use of navigator components is currently sketchy depending on whether they have/accumulate state or not
• would be good to allow sub-select in the from clause, but that means recursion support (especially in the SelectListExtractor which has to percolate column metadata out to the top-level select)

should be skipped

When the cadcproxy cert in the /config directory is expired, the /availability endpoint still reports that the service is up and available. The /tables endpoint reports an error. e.g.

$ curl https://ws-cadc.canfar.net/youcat/availability
<?xml version="1.0" encoding="UTF-8"?>
<vosi:availability xmlns:vosi="http://www.ivoa.net/xml/VOSIAvailability/v1.0">
  <vosi:available>true</vosi:available>
  <vosi:note>service is accepting queries</vosi:note>
  <!--<clientip>x.x.x.x</clientip>-->
</vosi:availability>

$ curl https://ws-cadc.canfar.net/youcat/tables?detail=min
unexpected exception: java.lang.RuntimeException: CONFIG: servops certificate is invalid

Problem noticed with the ALMA tap. The following query instead of returning 10 rows, it limits to sum(count(*)) = 10

curl -L -d "QUERY=SELECT top 10 obs_id, count(*) AS observationID from alma.obscore where t_min>1111 group by obs_id&LANG=ADQL" https://almascience.nrao.edu/tap/sync

It might have to do with inappropriate translation of TOP into Oracle

I get an error when executing the following query:

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

Without the ORDER BY it works smoothly.

Cheers,
Alberto

In a request with FORMAT=foo the error handling fails to create a TableWriter impl that can output the error message correctly. The bug is in DefaultTableWriter.initFormat being used to get the result TableWriter (should throw UnsupportedOperation exception) and to get the error tableWriter (which should always successfully return a writer.

Correct behaviour:

• separate method getErrorTableWriter should be implemented in PluginFactory
• it should look at RESPONSEFORMAT/FORMAT and return the requested impl if possible
• it should return VOTableWriter if RESPONSEFORMAT/FORMAT are invalid and never throw

When expanding "SELECT * ..." into an explicit column list, the converter should use the column_index in the tap_schema to put the columns into a sensible order.

It is feasible that TapSchemaDAO.setOrdered(true) in the QueryRunner could possibly do that right (TableDesc.columns is a List which should preserve order) but it might be safer for AllColumnsConverter to do the sorting itself... in a really large service adding order by to the tap_schema might also not be a great idea.