DM-27344: Add butler query-dimension-records subcommand #442
Conversation
| @collections_option(help=collections_option.help + " Only affects results when used with --datasets.") | ||
| @where_option(help=whereHelp) | ||
| @click.option("--no-check", is_flag=True, | ||
| help=unwrap("""Don't check the query before execution. By default the query is checked before it |
There was a problem hiding this comment.
@TallJimbo what do we gain by having this option? When is disabling the check a good idea?
There was a problem hiding this comment.
The check can generate false positives for some valid-but-rare queries. Those all involve wanting multiple instruments and a WHERE constraint on a table derived from instrument (e.g. exposure) on a column in that table whose values are nevertheless instrument-agnostic (e.g. exposure.exposure_time > 30). They then fall into two categories:
- Cases where you do restrict the instruments (or skymaps) you want, but use
INinstead of=andOR:instrument IN ('HSC', 'DECam') AND exposure.exposure_time > 30. The checker just isn't smart enough to rewrite theIN. - Cases where you actually want to query all of the instruments (or skymaps) in the registry: just
exposure.exposure_time > 30. Note that I think we still want to check this case by default, because usually the user is thinking of a specific instrument but we don't know that.
Similar cases may exist for skymaps, but it's harder to think of practically useful cases given the attributes actually on the tract and patch tables.
I'm open to opinions on whether that's sufficiently niche as to make this option not worth its maintenance weight.
| @@ -87,6 +87,7 @@ def readTable(textTable): | |||
| """ | |||
| return AstropyTable.read(textTable, | |||
| format="ascii", | |||
| data_start=2, # skip the header row and the header row underlines. | |||
There was a problem hiding this comment.
I'm a bit surprised that Astropy doesn't have a parser for the default pretty print table output. Part of me is wondering whether we should add an output format option to all the tabular output subcommands so that users can easily parse the output if they want. That would also let you specify csv format in tests for example.
There was a problem hiding this comment.
Formatter options are not a bad idea. Another format might be an option to choose between Table and yaml, if there's ever going to be a case where a user may pipe the output of one command to the input of another? How to do naming/data structure with yaml would probably take a little thought though, or maybe you've got good ideas already.
In this case, AstropyTable.read normally does the Right Thing, but I ran into a situation where it was getting confused. The table had one column and one row, something like
name
----
foo
and it was reading the ---- as a value row. I spent a little time trying to figure out why but didn't really get anywhere. It seemed ok to have the unit test function start reading at idx 2 (3rd row, eh) since we're consistent with table output format (until we decide not to be... then we have to come up with a better or more elaborate fix)
n8pease
force-pushed
the
tickets/DM-27344
branch
from
22426ba
to
01b08f1
Compare
n8pease
force-pushed
the
tickets/DM-27344
branch
from
01b08f1
to
71dcdc2
Compare
No description provided.