diff --git a/doc/changes/DM-38084.doc.md b/doc/changes/DM-38084.doc.md new file mode 100644 index 0000000000..7115206462 --- /dev/null +++ b/doc/changes/DM-38084.doc.md @@ -0,0 +1 @@ +Fix docs on referring to timespans in queries, and make related error messages more helpful. diff --git a/doc/lsst.daf.butler/queries.rst b/doc/lsst.daf.butler/queries.rst index 4faadd246a..98802b44a5 100644 --- a/doc/lsst.daf.butler/queries.rst +++ b/doc/lsst.daf.butler/queries.rst @@ -425,11 +425,11 @@ Few examples of valid expressions using some of the constructs: (visit = 100 OR visit = 101) AND exposure % 2 = 1 - visit.datetime_begin > T'2020-03-30 12:20:33' + visit.timespan.begin > T'2020-03-30 12:20:33' - exposure.datetime_begin > T'58938.515' + exposure.timespan.begin > T'58938.515' - visit.datetime_end < T'mjd/58938.515/tai' + visit.timespan.end < T'mjd/58938.515/tai' ingest_date < T'2020-11-06 21:10:00' diff --git a/python/lsst/daf/butler/cli/opt/options.py b/python/lsst/daf/butler/cli/opt/options.py index 68d3ac5836..7c76d5c5ec 100644 --- a/python/lsst/daf/butler/cli/opt/options.py +++ b/python/lsst/daf/butler/cli/opt/options.py @@ -277,9 +277,11 @@ def _config_split(*args: Any) -> dict[str, str]: "--order-by", help=unwrap( """One or more comma-separated names used to order records. Names can be dimension names, - metadata names optionally prefixed by a dimension name and dot, or - timestamp_begin/timestamp_end (with optional dimension name). To reverse ordering for a name - prefix it with a minus sign.""" + metadata field names, or "timespan.begin" / "timespan.end" for temporal dimensions. + In some cases the dimension for a metadata field or timespan bound can be inferred, but usually + qualifying these with "." is necessary. + To reverse ordering for a name, prefix it with a minus sign. + """ ), multiple=True, callback=split_commas, diff --git a/python/lsst/daf/butler/registry/queries/expressions/categorize.py b/python/lsst/daf/butler/registry/queries/expressions/categorize.py index f7a4d695a6..62144d357d 100644 --- a/python/lsst/daf/butler/registry/queries/expressions/categorize.py +++ b/python/lsst/daf/butler/registry/queries/expressions/categorize.py @@ -88,12 +88,17 @@ def categorizeElementId(universe: DimensionUniverse, name: str) -> tuple[Dimensi so at least its message should generally be propagated up to a context where the error can be interpreted by a human. """ - table, sep, column = name.partition(".") + table, _, column = name.partition(".") if column: try: element = universe[table] - except KeyError as err: - raise LookupError(f"No dimension element with name '{table}'.") from err + except KeyError: + if table == "timespan" or table == "datetime" or table == "timestamp": + raise LookupError( + "Dimension element name cannot be inferred in this context; " + f"use .timespan.{column} instead." + ) from None + raise LookupError(f"No dimension element with name {table!r} in {name!r}.") from None if isinstance(element, Dimension) and column == element.primaryKey.name: # Allow e.g. "visit.id = x" instead of just "visit = x"; this # can be clearer. @@ -172,8 +177,9 @@ def categorizeOrderByName(graph: DimensionGraph, name: str) -> tuple[DimensionEl field_name = name elif len(matches) > 1: raise ValueError( - f"Timespan exists in more than one dimesion element: {matches}," - " qualify timespan with specific dimension name." + "Timespan exists in more than one dimension element " + f"({', '.join(element.name for element in matches)}); " + "qualify timespan with specific dimension name." ) else: raise ValueError(f"Cannot find any temporal dimension element for '{name}'.") @@ -197,8 +203,9 @@ def categorizeOrderByName(graph: DimensionGraph, name: str) -> tuple[DimensionEl field_name = name elif len(match_pairs) > 1: raise ValueError( - f"Metadata '{name}' exists in more than one dimension element: " - f"{[element for element, _ in match_pairs]}, qualify metadata name with dimension name." + f"Metadata '{name}' exists in more than one dimension element " + f"({', '.join(element.name for element, _ in match_pairs)}); " + "qualify field name with dimension name." ) else: raise ValueError(f"Metadata '{name}' cannot be found in any dimension.") @@ -206,7 +213,11 @@ def categorizeOrderByName(graph: DimensionGraph, name: str) -> tuple[DimensionEl # qualified name, must be a dimension element and a field elem_name, _, field_name = name.partition(".") if elem_name not in graph.elements.names: - raise ValueError(f"Unknown dimension element name '{elem_name}'") + if field_name == "begin" or field_name == "end": + raise ValueError( + f"Unknown dimension element {elem_name!r}; perhaps you meant 'timespan.{field_name}'?" + ) + raise ValueError(f"Unknown dimension element {elem_name!r}.") element = graph.elements[elem_name] if field_name in ("timespan.begin", "timespan.end"): if not element.temporal: @@ -289,7 +300,11 @@ def categorizeElementOrderByName(element: DimensionElement, name: str) -> str | # qualified name, must be a dimension element and a field elem_name, _, field_name = name.partition(".") if elem_name != element.name: - raise ValueError(f"Element name mismatch: '{elem_name}' instead of '{element}'") + if field_name == "begin" or field_name == "end": + extra = f"; perhaps you meant 'timespan.{field_name}'?" + else: + extra = "." + raise ValueError(f"Element name mismatch: '{elem_name}' instead of '{element}'{extra}") if field_name in ("timespan.begin", "timespan.end"): if not element.temporal: raise ValueError(f"Cannot use '{field_name}' with non-temporal element '{element}'.") diff --git a/python/lsst/daf/butler/registry/tests/_registry.py b/python/lsst/daf/butler/registry/tests/_registry.py index 558ae01f5f..5ba82f76ad 100644 --- a/python/lsst/daf/butler/registry/tests/_registry.py +++ b/python/lsst/daf/butler/registry/tests/_registry.py @@ -2765,6 +2765,18 @@ def testQueryResultSummaries(self): self.assertTrue(messages) self.assertTrue(any("no-purpose" in message for message in messages)) + def testQueryDataIdsExpressionError(self): + """Test error checking of 'where' expressions in queryDataIds.""" + registry = self.makeRegistry() + self.loadData(registry, "base.yaml") + bind = {"time": astropy.time.Time("2020-01-01T01:00:00", format="isot", scale="tai")} + with self.assertRaisesRegex(LookupError, r"No dimension element with name 'foo' in 'foo\.bar'\."): + registry.queryDataIds(["detector"], where="foo.bar = 12") + with self.assertRaisesRegex( + LookupError, "Dimension element name cannot be inferred in this context." + ): + registry.queryDataIds(["detector"], where="timespan.end < time", bind=bind) + def testQueryDataIdsOrderBy(self): """Test order_by and limit on result returned by queryDataIds().""" registry = self.makeRegistry() @@ -2858,7 +2870,7 @@ def do_query(dimensions=("visit", "tract"), datasets=None, collections=None): list(do_query().order_by(order_by)) for order_by in ("undimension.name", "-undimension.name"): - with self.assertRaisesRegex(ValueError, "Unknown dimension element name 'undimension'"): + with self.assertRaisesRegex(ValueError, "Unknown dimension element 'undimension'"): list(do_query().order_by(order_by)) for order_by in ("attract", "-attract"): @@ -2868,7 +2880,11 @@ def do_query(dimensions=("visit", "tract"), datasets=None, collections=None): with self.assertRaisesRegex(ValueError, "Metadata 'exposure_time' exists in more than one dimension"): list(do_query(("exposure", "visit")).order_by("exposure_time")) - with self.assertRaisesRegex(ValueError, "Timespan exists in more than one dimesion"): + with self.assertRaisesRegex( + ValueError, + r"Timespan exists in more than one dimension element \(exposure, visit\); " + r"qualify timespan with specific dimension name\.", + ): list(do_query(("exposure", "visit")).order_by("timespan.begin")) with self.assertRaisesRegex( @@ -2882,6 +2898,11 @@ def do_query(dimensions=("visit", "tract"), datasets=None, collections=None): with self.assertRaisesRegex(ValueError, "Field 'name' does not exist in 'tract'."): list(do_query("tract").order_by("tract.name")) + with self.assertRaisesRegex( + ValueError, r"Unknown dimension element 'timestamp'; perhaps you meant 'timespan.begin'\?" + ): + list(do_query("visit").order_by("timestamp.begin")) + def testQueryDataIdsGovernorExceptions(self): """Test exceptions raised by queryDataIds() for incorrect governors.""" registry = self.makeRegistry() @@ -3001,6 +3022,14 @@ def do_query(element, datasets=None, collections=None): with self.assertRaisesRegex(ValueError, "Field 'attract' does not exist in 'detector'."): list(do_query("detector").order_by(order_by)) + for order_by in ("timestamp.begin", "-timestamp.begin"): + with self.assertRaisesRegex( + ValueError, + r"Element name mismatch: 'timestamp' instead of 'visit'; " + r"perhaps you meant 'timespan.begin'\?", + ): + list(do_query("visit").order_by(order_by)) + def testQueryDimensionRecordsExceptions(self): """Test exceptions raised by queryDimensionRecords().""" registry = self.makeRegistry()