gmantele / vollt

Java libraries implementing the IVOA protocol: ADQL, UWS and TAP
http://cdsportal.u-strasbg.fr/taptuto/
29 stars 28 forks source link

return type of UserDefinedFunction is not defined #130

Open vforchi opened 3 years ago

vforchi commented 3 years ago

I developed some custom functions by implementing UserDefinedFunction. One of these functions returns a Timestamp (a DATE in the signature), but when it is in one of the return columns the DBType is null. I think this is because the ADQL parser only considers instances of DefaultUDF and not of UserDefinedFunction. I can't extend DefaultUDF, because it's final.

I can't easily get rid of the custom classes for each function, because I rely this in other parts of the code, is there any other way around it?

gmantele commented 3 years ago

Hi, sorry for the late answer.

I have some trouble to see what it the exact problem.

If I understand well, you've got a problem in queries like:

SELECT my_UDF_returning_a_timestamp(...)
FROM ....

Is it correct?

Do you have any error message that can help me better understand the issue?

When does this error occur? During the query parsing, while translating or when formatting the query result into VOTable (or any other format)?

vforchi commented 3 years ago

There is no error, it's just that the return type is not what I would expect, and it defaults to String

gmantele commented 3 years ago

So, the problem is in the result file returned by TAP ? The value returned by the database is returned/considered by VOLLT/TAP as a String instead of a Date. Is that your problem?

vforchi commented 3 years ago

Yes, correct

gmantele commented 3 years ago

Ok, then 2 things:

  1. You're right, the DBType of the returned column is NULL because I extract the info only from DefaultUDF. This latter is final and can not be extended. That is now clearly a design flaw. Right now, I am improving some things in the management of UDF in VOLLT/ADQL. What changes for you is that DefaultUDF will disappear and UserDefinedFunction will no longer be abstract. Both will be merged into UserDefinedFunction ; none function defined in there will be final. So for you, it should not change your extensions of UserDefinedFunction (but you will probably be able to make some clean up as most functions will already be implemented). Doing that will also fix this GitHub Issue, as there will no longer be any processing difference in the library between DefaultUDF and UserDefinedFunction. And so, a DBType will be associated with your column.

  2. However, this may not change the fact that, I assume, you get a String from the database. Your JDBC driver probably returns DB's date/timestamp as String. Currently, TAPLib format any java.sql.Date, java.sql.Time and java.util.Date returned by the JDBC driver into a String following ISO8601. In the current state, this processing does not apply for String, even if they are marked as date/timestamp in the TAP/ADQL metadata. I should probably do that anyway, though I will not know the format of date/timestamp returned/used by the DB/JDBC driver....but something by default could be done anyway.

In conclusion, with 1. fixed, there will be a DBType generated by the library and so there will datatype="char" arraysize="*" xtype="adql:TIMESTAMP" in the VOTable output for this column. But because of 2., I can not guarantee the format of the date/timestamp string in there.

Currently, I don't think there is anything simple you can do to fix (even temporarily) this issue, unless you want to change a core class of the library in a fork.

I will commit tomorrow (or beginning of next week) the merge of DefaultUDF and UserDefinedFunction as I am already working on it. So, if you want, I can notify you through this GitHub issue when it is available (and eventually generate a tap.jar for you). However, it will be commited in the branch adql-2.1 which is not guaranteed to be yet stable and so, may imply some changes on your side....but I am really not sure...it depends on what you have/use.

Anyway, did I answer to all your questions?

vforchi commented 3 years ago

Yes, thanks. It was very clear. I will wait for adql 2.1 to be released.

I get a Timestamp from the DB, and I format it properly using STIL. The only thing that is wrong is the metadata.