gmantele / vollt

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

Parser: user-defined functions in SELECT? #101

Open theresadower opened 5 years ago

theresadower commented 5 years ago

(Note: this may be an issue that has been recently resolved; I am working to update a local fork from master and retest.)

User-defined functions in the SELECT portion of a query throw a parser exception. There is some support for user-defined functions in the parser and dbchecker, however this currently doesn't work in SELECT. I don't know if this is a bug, or would be an entirely new feature.

Example query, from a call to MAST CAOM catalog, which we would like to have available via the CAOM TAP service: SELECT dbo.fnAstron_ComputeScale(c.poswcsCD11, c.poswcsCD12)as scale FROM dbo.CaomChunk c WHERE ....

gmantele commented 5 years ago

There should be no problem to declare and use User Defined Function (UDF).

According to me, the problem comes from the prefix you used to qualify your UDF fn_Astron_ComputeScale: dbo.

It is true that the BNF of ADQL-2.0 tells that a prefix is possible and that a UDF name can be qualified but no syntax is ever defined in this BNF grammar for this purpose:

<user_defined_function> ::=
          <user_defined_function_name>
          <left_paren>
                    [ <user_defined_function_param> [ { <comma>
                    <user_defined_function_param> }... ] ]
          <right_paren>

<user_defined_function_name> ::=
          [ <default_function_prefix> ] <regular_identifier>

<default_function_prefix> ::=

Not knowing exactly what was expected by the user, I chose to strictly follow this grammar....hence this simple way to name a UDF.

So, I think that just removing the UDF prefix should resolve your issue. However, if your SQL function really must to be qualified in the corresponding SQL query to run, then, you will have to force this prefix in the SQL translation that you provided in the UDF declaration.

theresadower commented 5 years ago

Gregory,

Thanks so much for your explanation of the UDF support. I have implemented that in our service configuration and it is almost working as is!

Unfortunately, the schema. prefix is required by MSSQL for non-scalar functions. I tried configuring the checker to know: dbo.fnAstron_ComputeScale(cd11 float, cd12 float) -> float rather than: fnAstron_ComputeScale(cd11 float, cd12 float) -> float

Sadly this fails pattern matching, I believe because of the '.' I have made a workaround in our service but the issue will remain for other MSSQL implementations. If I get a chance to test out a new regexp with the schema prefix allowed and it works, I'll make a pull request. Thanks again.

gmantele commented 5 years ago

Sorry, I probably badly explained my suggested correction.

You do not have to "hack" anything. When defining a UDF in the library, you can specify how to translate it in SQL so that it can run as expected in the database. Basically, you have to say to the library that this particular UDF must be renamed in the SQL query into "dbo.fnAstron_ComputeScale".

For that, you have to write an extension of the class UserDefinedFunction and to return the SQL function in its function "translate". Then, you will have to declare this class in your configuration along your UDF definition.

It is quite easy and fast to do. If you want to, I will give you an example tomorrow.

theresadower commented 5 years ago

Aha, that is very similar to what I have done in our code for now, and a little bit cleaner. I'll try to switch to this method when I get a chance soon. Thanks!

theresadower commented 5 years ago

@gmantele Thank to your description, I see how to extend UserDefinedFunction and override the translate function. I don't know what configuration to send so it knows to use the new extension class, however. Can you explain that part further? Thanks again for all your help.