ivoa-std / ADQL

Astronomical Data Query Language Standard
https://wiki.ivoa.net/twiki/bin/view/IVOA/ADQL
Creative Commons Attribution Share Alike 4.0 International
7 stars 7 forks source link

An attempt to fix issue #17 alternative to PR #24. #25

Closed msdemlei closed 4 years ago

msdemlei commented 4 years ago

Here's what it does:

msdemlei commented 4 years ago

On Tue, Jan 14, 2020 at 05:48:55AM -0800, Grégory Mantelet wrote:

However I don't like limiting the REGION function's argument to a string literal. It is true that currently no database (of TAP services I know) supports parsing directly STC-S (or any alternative). But I think that is more due to the fact that there is not a lot of usage of this REGION function.

Well, I'd claim it's mostly due to it being rather hard. As I said, you'd have to teach your database system STC-S (or, for my Simbad example, access to Simbad's API). I don't think it's likely anyone will do that soon.

What if, in the future, it becomes more frequent? In such case, users may feel immediately limited with a string litteral. I don't have concrete example in mind, but it could be perfectly possible to generate a region dynamically with a simple concatenation or even with a UDF. If someone (user or service provider) has a good use case for that someday, it would be a shame that he has to wait for a new ADQL version.

They won't. Nothing keeps them from being more liberal than what the standard requires. If you can do it, accept column references or whatever in the argument, and nobody will catch you. Advertise this to your users (the examples endpoint is your friend), and everyone is happy.

On the other hand, if in the grammar we say you can use column references in REGION, users have every right to expect that it "generally" works (except perhaps on a few half-broken services). If they then find there is not a single TAP service that can do it, it's a lot worse than if the grammar makes the intent clear from the start.

Also, if I were writing a validator, I'd obviously write a test for parsing STC-S in the database, as that would be something I'd expect to fail quite often. Indeed, all current services would fail -- that's not a good thing.

So, I think it does not hurt to keep allowing that (as in ADQL-2.0). Especially because, as far as I know, nobody really supports it and great efforts would have to be deployed for that...and so, there is a low risk that a user wants to do such a thing, and if he does, the service would immediately complain with an appropriate error message.

Hm -- do you honestly believe it's a good user experience if we require something in a grammar and all services error out when someone tries it?

Again: Nobody keeps you from accepting string expressions, and I think we should understand ADQL evolution such that a future (minor) version may indeed be more liberal, requiring statements to parse that on a previous version would have yielded syntax errors. So, we don't have to require this now just so one day in the future, perhaps when we do have a sane geometry syntax that databases understand, we can support in-DB REGION parsing.

gmantele commented 4 years ago

Ok, then, I agree with your proposal.

I wait for a few more time in case someone else wants to say something, especially @Zarquan and @pdowler . Then, I will approve the PR and merge it.

In the meantime, I close (without merge) the PR #24 that I proposed.