Skip to content
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

BUG: column order should have no effect #456

Open
bsipocz opened this issue Jul 6, 2023 · 1 comment
Open

BUG: column order should have no effect #456

bsipocz opened this issue Jul 6, 2023 · 1 comment
Labels

Comments

@bsipocz
Copy link
Member

bsipocz commented Jul 6, 2023

While I was fixing #445 I noticed that the code picked the first column that matched the criteria (e.g. it did matter where I added the extra info in the test data).
However, the column order should have no effect, e.g. we should parse all and raise an exception if there are multiple columns matching or have a better hierarchy for what to look for.

I'm opening this issue to make sure we double check all service types for this type of hidden regression/assumption.

@bsipocz bsipocz added the bug label Jul 6, 2023
@msdemlei
Copy link
Contributor

msdemlei commented Jul 7, 2023

Interesting point. I'll mention that I don't think any IVOA standard actually explicitly says what a client ought to do in the presence of multiple identified things. Outside of ancient SIA, the thing designated for "unique" (in whatever sense) selection (so far; MIVOT might change that) is the utype, so I've made a quick census of where utype is mentioned in pyvo. The nontrivial occurrences:

  • dal.adhoc: uses utype to build lists and hence is unconcerned by this issue.

  • dal.query: fieldname_with_utype/getbyutype has exactly the greedy behaviour you would like to do away with. getdataurl is a piece of wild heuristics that doesn't count wrt your concerns I'd say -- we cannot forbid multiple matches for these heuristics.

  • dal.sla, dal.ssa: use getbyutype

That's about it.

So... if we're serious about failing for non-unique matches, dal.query.DALResults.field_name_with_utype would be the place to do it.

Me... well, I'm not opposed to a reasonable failure mode (warning?), because it's probably cheap and it helps improving services. However, in general I think this kind of validation is the job of, well, validators, and I'd like pyVO to keep defaulting to best-effort operation (i.e., if we can halfway confidently figure out what the service authors wanted us to do, we should do it).

Meaning: I'll kindly review a PR, but I'll not prepare one myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants