serhiy-bzhezytskyy opened a new pull request, #77:
URL: https://github.com/apache/solr-orbit/pull/77
### Description
`_calendar_interval_to_solr_gap` received whichever of the three interval
keys was present, lowercased it, and looked it up in a table of calendar names:
```python
interval = (dh_conf.get("calendar_interval")
or dh_conf.get("fixed_interval")
or dh_conf.get("interval", "month"))
...
"1m": "+1MINUTE",
"month": "+1MONTH",
"1m_month": "+1MONTH", # avoid conflict with 1m (minute)
...
return mapping.get(str(interval).lower(), "+1MONTH")
```
Two things follow from that, and the comment in the table is a note that the
second was known:
- A `fixed_interval` is a count and a unit, so it is never in the table.
`fixed_interval: 60d` fell through to the `+1MONTH` default, and so did
`2000d`. No warning: the value looked converted.
- OpenSearch distinguishes the two single-unit abbreviations **by case** —
`1M` is one month, `1m` is one minute. Lowercasing collapses them, so
`calendar_interval: 1M` produced `+1MINUTE`. The `1m_month` key exists to
sidestep that, but no OpenSearch workload can emit `1m_month`, so nothing ever
reached it.
The interval is now read according to the key it arrived under.
`calendar_interval` takes the names case-insensitively and the abbreviations
case-sensitively; `fixed_interval` is parsed as `<count><unit>` over
`ms|s|m|h|d`, the five units OpenSearch accepts there; the deprecated
`interval` accepts either form, so both are tried. An interval that is neither
still falls back to `+1MONTH`, but now logs a warning naming the aggregation,
instead of quietly emitting a wrong bucket width.
### Measured
Every `date_histogram` in `opensearch-benchmark-workloads` — 27 aggregations
across 6 workloads — converted before and after:
```
same calendar_interval 1w +7DAYS -> +7DAYS x12
same calendar_interval month +1MONTH -> +1MONTH x5
same calendar_interval day +1DAY -> +1DAY x1
same fixed_interval 1m +1MINUTE -> +1MINUTE x1
CHANGED calendar_interval 1M +1MINUTE -> +1MONTH x4
CHANGED fixed_interval 60d +1MONTH -> +60DAY x3
CHANGED fixed_interval 2000d +1MONTH -> +2000DAY x1
```
19 unchanged, 8 corrected: `nyc_taxis` `date_histogram_fixed_interval` and
its `_with_tz` / `_with_metrics` variants, `noaa` `date-histo-entire-range`,
and four `noaa_semantic_search` hybrid aggregations where a month was being
requested as a minute.
Solr accepts every gap the new parser can produce — `+2699999MILLI`,
`+30SECOND`, `+90MINUTE`, `+3HOUR`, `+60DAY`, `+2000DAY` all return HTTP 200 on
Solr 10.0.0.
Running the converter over `opensearch-benchmark-workloads/nyc_taxis` now
emits `"gap": "+60DAY"` for `date_histogram_fixed_interval`, matching the
`fixed_interval: 60d` in the source.
### What this does not fix
The gap is the bucket width, not the bucket edges. The generated facet still
carries `"start": "NOW/YEAR-10YEAR"`, so the window drifts with the clock and
today lands outside the corpus entirely — a separate defect, in different
lines, with its own measurement to make. So this PR makes the width faithful;
it does not by itself make the generated `date_histogram` operation return the
same histogram as OpenSearch.
Bucket counts were compared against OpenSearch for `nyc_taxis` only. For
`noaa` and `noaa_semantic_search` the claim here is narrower: the gap now says
what the source workload says, and Solr accepts it.
### Issues Resolved
None to close.
### Related
`+60DAY` is the same value that apache/solr-orbit-workloads#PLACEHOLDER
writes into the shipped `nyc_taxis` operation, verified there against
OpenSearch bucket by bucket. Neither PR needs the other: that one corrects a
file that exists, this one corrects what the converter would generate for it.
They are linked because the shipped workload should stay reproducible by the
converter — a hand-written value the converter cannot produce is a divergence
that the next regeneration silently reverts.
### Testing
- [x] New functionality includes testing
`tests/unit/solr/test_workload_converter.py` gains cases for each key: every
`fixed_interval` unit, `1M` against `1m`, the deprecated `interval` under both
readings, the missing-interval default, and an assertion that an unconvertible
interval logs a warning naming the aggregation. Two of them go through
`_convert_aggregations_to_facets` rather than the helper, so the facet body
itself is asserted.
`test_unknown_defaults_to_month` asserted the old silent `+1MONTH` for
`"fortnight"`; the default is unchanged, so that expectation moved to the test
that also requires the warning.
Full unit suite: 205 passed. `ruff check` clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
