-
Notifications
You must be signed in to change notification settings - Fork 7
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 geometry language feature identifier version inconsistency #71
Comments
Sorry if your email of 2019 (that's indeed a long time ago) did not get any answer. And thank you to revive this in here (though I would have answered also my email, I think). This skipped hyphen is indeed a problem for existing services and validators which use and declare such language feature. It is a shame that I see three solutions (in my personal order of preference):
Considering these identifiers are for machines only and so, are hidden from the users eyese all the time, I guess it is better to go for 2 or 3. None would break anything on the contrary to 1. Mark, anyone else, what do you think? |
If my chronology is right, I don't think that either TAPRegExt (2012) or ADQL 2.0 (2008) can be blamed for this, since at the time of their publication there were no other identifiers of the form History aside, I think that option 1 would be too disruptive, since there are many ADQL-2.0 services out there already using Option 2 is as you say ugly and annoying but not really harmful; I agree that a comment in ADQL-2.1 noting the discrepancy for historical reasons is a very good idea. Option 3 may be OK (ADQL 2.1 is after all still not a REC) depending on how many ADQL-2.1 services and clients there are out there using these identifiers, and also how amenable to updating they are. Does anybody have an opinion or information on that? Maybe @msdemlei demlei does since as I noted above many/most look like DaCHS. But on the whole I'd say my preference is option 2. |
On Fri, Jan 27, 2023 at 10:22:06AM -0800, Grégory Mantelet wrote:
Mark, anyone else, what do you think?
Well, there *is* a fourth option. You see, I originally only stuck
the identifiers into TAPRegExt because back then we said we shouldn't
create identifiers in ADQL from TAPRegExt. In retrospect, I think
none of that was a wise decision (that's particularly easy for me to
say in this case because I said that back then already). We could
rectify that to some extent now by saying:
(a) The legacy identifiers (features-udf, features-adqlgeo) remain in
the TAPRegExt record.
(b) All new feature ids are now in the ADQL record. That would let
us drop the adql fragment in the identifiers (which isn't too pretty
anyway), and the form of the others would be
ivo://ivoa.net/std/adql#features-string
or so.
I *think* most clients would either discard the non-fragment part
(which, for comparison purposes, would actually be smart, as the
ivoid itself needs to be compared case-insensitively by ivoid rules
-- yikes!) or compare the whole strings (which, given the case
crazyness, actually is less right), so that probably wouldn't make
implementations a lot uglier.
[now that I mention it: I think ADQL should say that while the
feature identifiers contain an ivoid and that is case-insensitive,
servers MUST use all-lowercase and clients MAY compare without case
normalisation. Should I raise an issue of that? Or write a PR
immediately?]
So... unless we're worried about ADQL 2.1 implementations already
deployed, that would actually be my favourite outcome: There's a bit
of easily-identified legacy, and there's new-style, consistent
practice.
Now that I think of it, I could even see deprecating the
TAPRegExt#adqlgeo identifier in 2.1 and creating a synonymous
adql#features-(adql?)geo identifier, requiring clients to accept
both. But I think the features-udf key would need to remain in
TAPRegExt (I'd argue it's not *really* specific to ADQL), so perhaps
that's something we can do in 2.2 just as well if we find that might
help.
|
Thank you to both of you for your answers. Although ADQL-2.1 is not yet a REC, there are already services implementing and declaring ADQL-2.1. That's a shame...but that's completely fair as ADQL-2.1 is taking a very long time to reach REC, and some use-cases sometimes require some of the ADQL-2.1 features...people cannot wait forever. I hope we will not have to wait so long for the next ADQL versions 🤞 . Anyway, as the first intent of this Issue is to not break existing ADQL-2.1 implementations and because @msdemlei said (about his suggested 4-th option):
I agree with @mbtaylor : we should go for the 2nd Option (ugly for picky human eyes, but harmless for servers and clients, existing or not):
I am now going to prepare a Pull-Request fixing this IVOID. I will also add into this Pull-Requests 2 other modifications:
|
I raised this on the DAL list in 2019 but didn't get a response, so I'll try it here instead...
In the current ADQL 2.1 draft, geometrical functions are labelled using the following Language Feature identifier:
However in ADQL 2.0, the corresponding functionality was marked by the feature (defined in TAPRegExt 1.0 Sec 2.3):
Note the lack of a second hyphen.
This looks like a typo, so I suggest changing
adql-geo
toadqlgeo
in the current text. If not, there is an inconsistency between ADQL 2.0 and 2.1. I can prepare a PR on request.This change could be problematic if there are already services out there using the
adql-geo
form as written in the current draft. However, I suspect that there are few or none.RegTAP does not record language features, but if I look for all the services claiming to support ADQL 2.1:
there are 24. The couple I tried (both DaCHS) use
adqlgeo
notadql-geo
already (i.e. in accordance with my suggested change, and in violation of the draft as currently written). I suspect the others are mostly DaCHS instances too, so have the same behaviour.Since I spotted this in 2019 (STILTS v3.2 and later) taplint has issued an Error if the
adql-geo
form is used, like this:i.e. taplint currently agrees with my suggested change rather than what's written in the current ADQL 2.1 draft.
So my guess is that this change is not going to cause problems.
This issue has come up now because ESDC are using the
adql-geo
form in their ADQL 2.1 services, as in the current draft, and have hit that taplint error.The text was updated successfully, but these errors were encountered: