Convert date_histogram fixed_interval instead of defaulting to +1MONTH - #77
Open
serhiy-bzhezytskyy wants to merge 1 commit into
Open
Conversation
The converter collapsed calendar_interval, fixed_interval and the deprecated
interval into one value and looked it up, lowercased, in a table of calendar
names. Two consequences:
* A fixed_interval is a number and a unit, so it was never in the table.
fixed_interval: 60d silently became gap: +1MONTH, as did 2000d.
* Lowercasing conflates the two abbreviations OpenSearch distinguishes by
case: calendar_interval: 1M is one month and 1m is one minute. The table
carried a "1m_month" key with a comment noting the clash; no OpenSearch
workload can produce that key, so 1M read as one minute.
The interval is now read from the key it arrived under: calendar names and
their single-unit abbreviations case-sensitively, or a fixed_interval parsed
as a count plus ms/s/m/h/d, the five units OpenSearch accepts there. Solr
takes every resulting gap (+2699999MILLI, +30SECOND, +90MINUTE, +3HOUR,
+60DAY, +2000DAY all return HTTP 200 on 10.0.0). An interval that is neither
still falls back to +1MONTH, but now says so in a warning rather than
producing quietly wrong bucket widths.
Measured over the 27 date_histogram aggregations in opensearch-benchmark-
workloads: 19 gaps unchanged, 8 corrected — nyc_taxis 60d ×3 and noaa 2000d
from +1MONTH, and noaa_semantic_search 1M ×4 from +1MINUTE.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
_calendar_interval_to_solr_gapreceived whichever of the three interval keys was present, lowercased it, and looked it up in a table of calendar names:Two things follow from that, and the comment in the table is a note that the second was known:
fixed_intervalis a count and a unit, so it is never in the table.fixed_interval: 60dfell through to the+1MONTHdefault, and so did2000d. No warning: the value looked converted.1Mis one month,1mis one minute. Lowercasing collapses them, socalendar_interval: 1Mproduced+1MINUTE. The1m_monthkey exists to sidestep that, but no OpenSearch workload can emit1m_month, so nothing ever reached it.The interval is now read according to the key it arrived under.
calendar_intervaltakes the names case-insensitively and the abbreviations case-sensitively;fixed_intervalis parsed as<count><unit>overms|s|m|h|d, the five units OpenSearch accepts there; the deprecatedintervalaccepts 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_histograminopensearch-benchmark-workloads— 27 aggregations across 6 workloads — converted before and after:19 unchanged, 8 corrected:
nyc_taxisdate_histogram_fixed_intervaland its_with_tz/_with_metricsvariants,noaadate-histo-entire-range, and fournoaa_semantic_searchhybrid aggregations where a month was being requested as a minute.Solr accepts every gap the new parser can produce —
+2699999MILLI,+30SECOND,+90MINUTE,+3HOUR,+60DAY,+2000DAYall return HTTP 200 on Solr 10.0.0.Running the converter over
opensearch-benchmark-workloads/nyc_taxisnow emits"gap": "+60DAY"fordate_histogram_fixed_interval, matching thefixed_interval: 60din 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 generateddate_histogramoperation return the same histogram as OpenSearch.Bucket counts were compared against OpenSearch for
nyc_taxisonly. Fornoaaandnoaa_semantic_searchthe claim here is narrower: the gap now says what the source workload says, and Solr accepts it.Issues Resolved
None to close.
Related
+60DAYis the same value that apache/solr-orbit-workloads#21 writes into the shippednyc_taxisoperation, 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
tests/unit/solr/test_workload_converter.pygains cases for each key: everyfixed_intervalunit,1Magainst1m, the deprecatedintervalunder 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_facetsrather than the helper, so the facet body itself is asserted.test_unknown_defaults_to_monthasserted the old silent+1MONTHfor"fortnight"; the default is unchanged, so that expectation moved to the test that also requires the warning.Full unit suite: 205 passed.
ruff checkclean.