Convert date_histogram fixed_interval instead of defaulting to +1MONTH

solrorbit/conversion/query.py

@@ -26,6 +26,7 @@
 """
 
 import logging
+import re
 from datetime import datetime
 
 from .field import normalize_field_name
@@ -448,12 +449,7 @@ def _convert_single_agg(agg_name: str, agg_def: dict):
         if not field:
             logger.warning("date_histogram agg '%s' has no field — skipping", agg_name)
             return None
-        interval = (
-            dh_conf.get("calendar_interval")
-            or dh_conf.get("fixed_interval")
-            or dh_conf.get("interval", "month")
-        )
-        gap = _calendar_interval_to_solr_gap(interval)
+        gap = _date_histogram_gap(dh_conf, agg_name)
         facet_def = {
             "type": "range",
             "field": field,
@@ -508,25 +504,82 @@ def _convert_single_agg(agg_name: str, agg_def: dict):
     return None
 
 
-def _calendar_interval_to_solr_gap(interval: str) -> str:
-    """Convert an OpenSearch calendar_interval or fixed_interval to a Solr range gap string."""
-    mapping = {
-        "minute": "+1MINUTE",
-        "1m": "+1MINUTE",
-        "hour": "+1HOUR",
-        "1h": "+1HOUR",
-        "day": "+1DAY",
-        "1d": "+1DAY",
-        "week": "+7DAYS",
-        "1w": "+7DAYS",
-        "month": "+1MONTH",
-        "1m_month": "+1MONTH",  # avoid conflict with 1m (minute)
-        "quarter": "+3MONTHS",
-        "1q": "+3MONTHS",
-        "year": "+1YEAR",
-        "1y": "+1YEAR",
-    }
-    return mapping.get(str(interval).lower(), "+1MONTH")
+CALENDAR_INTERVAL_NAMES = {
+    "minute": "+1MINUTE",
+    "hour": "+1HOUR",
+    "day": "+1DAY",
+    "week": "+7DAYS",
+    "month": "+1MONTH",
+    "quarter": "+3MONTHS",
+    "year": "+1YEAR",
+}
+
+CALENDAR_INTERVAL_ABBREVIATIONS = {
+    "1m": "+1MINUTE",
+    "1h": "+1HOUR",
+    "1d": "+1DAY",
+    "1w": "+7DAYS",
+    "1M": "+1MONTH",
+    "1q": "+3MONTHS",
+    "1y": "+1YEAR",
+}
+
+FIXED_INTERVAL_UNITS = {
+    "ms": "MILLI",
+    "s": "SECOND",
+    "m": "MINUTE",
+    "h": "HOUR",
+    "d": "DAY",
+}
+
+FIXED_INTERVAL_PATTERN = re.compile(r"^(\d+)(ms|s|m|h|d)$")
+
+DEFAULT_GAP = "+1MONTH"
+
+
+def _date_histogram_gap(dh_conf: dict, agg_name: str | None = None) -> str:
+    """
+    Solr range gap for an OpenSearch date_histogram.
+
+    The key the interval arrives under decides how it reads: ``calendar_interval: 1m``
+    is one minute and ``calendar_interval: 1M`` is one month, while a fixed_interval is
+    a multiple of a fixed unit and takes no calendar names at all. The deprecated
+    ``interval`` key accepts either form, so both are tried.
+
+    An interval that is neither is logged and falls back to DEFAULT_GAP.
+    """
+    if "calendar_interval" in dh_conf:
+        interval = dh_conf["calendar_interval"]
+        gap = _calendar_interval_to_solr_gap(interval)
+    elif "fixed_interval" in dh_conf:
+        interval = dh_conf["fixed_interval"]
+        gap = _fixed_interval_to_solr_gap(interval)
+    else:
+        interval = dh_conf.get("interval", "month")
+        gap = _calendar_interval_to_solr_gap(interval) or _fixed_interval_to_solr_gap(interval)
+    if gap:
+        return gap
+    logger.warning(
+        "date_histogram agg '%s' has interval '%s', which is neither an OpenSearch "
+        "calendar_interval nor a fixed_interval — using %s, so the buckets will not "
+        "have the width the workload asks for.",
+        agg_name, interval, DEFAULT_GAP,
+    )
+    return DEFAULT_GAP
+
+
+def _calendar_interval_to_solr_gap(interval) -> str | None:
+    """Solr range gap for an OpenSearch calendar_interval, or None if it is not one."""
+    return (CALENDAR_INTERVAL_ABBREVIATIONS.get(str(interval))
+            or CALENDAR_INTERVAL_NAMES.get(str(interval).lower()))
+
+
+def _fixed_interval_to_solr_gap(interval) -> str | None:
+    """Solr range gap for an OpenSearch fixed_interval, or None if it is not one."""
+    match = FIXED_INTERVAL_PATTERN.match(str(interval))
+    if not match:
+        return None
+    return "+{}{}".format(match.group(1), FIXED_INTERVAL_UNITS[match.group(2)])
 
 
 def _convert_date_to_solr_format(date_str, os_format=None) -> str:

tests/unit/solr/test_workload_converter.py

@@ -32,6 +32,8 @@
     translate_to_solr_json_dsl,
     _convert_aggregations_to_facets,
     _calendar_interval_to_solr_gap,
+    _date_histogram_gap,
+    _fixed_interval_to_solr_gap,
 )
 
 
@@ -345,13 +347,72 @@ def test_known_intervals(self):
         self.assertEqual("+1MONTH", _calendar_interval_to_solr_gap("month"))
         self.assertEqual("+1YEAR", _calendar_interval_to_solr_gap("year"))
         self.assertEqual("+1HOUR", _calendar_interval_to_solr_gap("hour"))
+        self.assertEqual("+7DAYS", _calendar_interval_to_solr_gap("week"))
+        self.assertEqual("+3MONTHS", _calendar_interval_to_solr_gap("quarter"))
 
-    def test_unknown_defaults_to_month(self):
-        self.assertEqual("+1MONTH", _calendar_interval_to_solr_gap("fortnight"))
-
-    def test_case_insensitive(self):
+    def test_names_are_case_insensitive(self):
         self.assertEqual("+1MONTH", _calendar_interval_to_solr_gap("MONTH"))
 
+    def test_single_unit_abbreviations_are_case_sensitive(self):
+        """OpenSearch reads 1M as a month and 1m as a minute; both occur in real workloads."""
+        self.assertEqual("+1MONTH", _calendar_interval_to_solr_gap("1M"))
+        self.assertEqual("+1MINUTE", _calendar_interval_to_solr_gap("1m"))
+
+    def test_not_a_calendar_interval(self):
+        self.assertIsNone(_calendar_interval_to_solr_gap("fortnight"))
+        self.assertIsNone(_calendar_interval_to_solr_gap("60d"))
+
+
+class TestFixedIntervalToSolrGap(unittest.TestCase):
+    def test_every_unit_opensearch_accepts(self):
+        self.assertEqual("+2699999MILLI", _fixed_interval_to_solr_gap("2699999ms"))
+        self.assertEqual("+30SECOND", _fixed_interval_to_solr_gap("30s"))
+        self.assertEqual("+90MINUTE", _fixed_interval_to_solr_gap("90m"))
+        self.assertEqual("+3HOUR", _fixed_interval_to_solr_gap("3h"))
+        self.assertEqual("+60DAY", _fixed_interval_to_solr_gap("60d"))
+        self.assertEqual("+2000DAY", _fixed_interval_to_solr_gap("2000d"))
+
+    def test_not_a_fixed_interval(self):
+        self.assertIsNone(_fixed_interval_to_solr_gap("month"))
+        self.assertIsNone(_fixed_interval_to_solr_gap("1M"))
+        self.assertIsNone(_fixed_interval_to_solr_gap("60"))
+
+
+class TestDateHistogramGap(unittest.TestCase):
+    def test_the_key_decides_how_the_interval_reads(self):
+        self.assertEqual("+1MONTH", _date_histogram_gap({"calendar_interval": "1M"}))
+        self.assertEqual("+1MINUTE", _date_histogram_gap({"calendar_interval": "1m"}))
+        self.assertEqual("+60DAY", _date_histogram_gap({"fixed_interval": "60d"}))
+
+    def test_deprecated_interval_key_accepts_either_form(self):
+        self.assertEqual("+1MONTH", _date_histogram_gap({"interval": "1M"}))
+        self.assertEqual("+60DAY", _date_histogram_gap({"interval": "60d"}))
+
+    def test_missing_interval_defaults_to_month(self):
+        self.assertEqual("+1MONTH", _date_histogram_gap({"field": "dropoff_datetime"}))
+
+    def test_unconvertible_interval_warns(self):
+        with self.assertLogs("solrorbit.conversion.query", level="WARNING") as log:
+            gap = _date_histogram_gap({"calendar_interval": "fortnight"}, "dropoffs_over_time")
+        self.assertEqual("+1MONTH", gap)
+        self.assertTrue(any("fortnight" in msg for msg in log.output))
+        self.assertTrue(any("dropoffs_over_time" in msg for msg in log.output))
+
+
+class TestDateHistogramFacetGap(unittest.TestCase):
+    def _gap(self, date_histogram):
+        body = {"aggs": {"dropoffs_over_time": {"date_histogram": date_histogram}}}
+        facets = _convert_aggregations_to_facets(body["aggs"])
+        return facets["dropoffs_over_time"]["gap"]
+
+    def test_fixed_interval_keeps_its_width(self):
+        gap = self._gap({"field": "dropoff_datetime", "fixed_interval": "60d"})
+        self.assertEqual("+60DAY", gap)
+
+    def test_calendar_month_abbreviation_is_not_a_minute(self):
+        gap = self._gap({"field": "dropoff_datetime", "calendar_interval": "1M"})
+        self.assertEqual("+1MONTH", gap)
+
 
 if __name__ == "__main__":
     unittest.main()