Fix: convert-workload produces an unparseable date bound, and loses a day on a whole-day lte - #74
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are two edge-case logic bugs in the new rounding/date-only detection that can incorrectly round or change bracket semantics when dates aren’t actually parsed or when both gt and gte are provided.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes date range bound translation in the OpenSearch→Solr workload converter so that converted range queries both (1) remain parseable by Solr when input datetimes use a space separator and (2) preserve OpenSearch’s whole-day rounding semantics for date-only lte/gt bounds.
Changes:
- Extend date parsing to handle
yyyy-MM-dd HH:mm:ssand track whether a parsed bound is “date-only” vs an instant. - Implement whole-day rounding for date-only
lte(upper) andgt(lower) bounds by advancing to the next day’s start and adjusting bracket exclusivity accordingly. - Add unit tests covering space-separated datetimes and whole-day rounding behavior for
lte,gt,gte, andlt.
File summaries
| File | Description |
|---|---|
solrorbit/conversion/query.py |
Updates date parsing/format mapping and applies whole-day rounding logic during range query translation. |
tests/unit/solr/test_workload_converter.py |
Adds focused unit tests validating the new date conversion and rounding behaviors. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two defects in _convert_date_to_solr_format, both reachable from upstream
nyc_taxis bodies and neither addressed by the bracket fix.
1. A datetime written with a space, `"2016-01-01 00:00:00"`, had no pattern in
the format map. The function logged "Could not parse date ... using as-is"
and returned it unchanged, so the converter emitted
dropoff_datetime:[2015-01-01 00:00:00 TO 2016-01-01 00:00:00}
and Solr answers HTTP 400, "SyntaxError: Cannot parse ... Encountered
<RANGE_GOOP>". The space ends the range term. Two nyc_taxis operations,
date_histogram_calendar_interval and date_histogram_fixed_interval, are
written this way upstream, so neither could run at all after conversion.
2. A bound with no time names a day, and OpenSearch rounds it to that day's
edge: `lte` and `gt` to its last millisecond, `gte` and `lt` to its first.
Solr rounds nothing. So `lte: "21/01/2015"` with format dd/MM/yyyy became
the FIRST instant of the 21st, and the query lost a whole day of documents.
The two bounds that round to the end of the day now name the start of the
following day and exclude it, which says the same thing without depending on
how fine Solr's date precision is.
The function now reports whether its input named a day or an instant, because
rounding cannot be decided from the value alone -- it needs the bound it
belongs to.
Rounding is gated on the bound that was actually used, not on the mere presence
of a key. A range carrying both `gte` and `gt` takes its lower bound from `gte`,
so gating on `"gt" in bounds` rounded a bound that rounds down: `gte:
"01/01/2015"` came out as the 2nd.
A value that could not be parsed is no longer reported as naming a whole day.
That was decided from string length alone, which is reachable only when every
format in the map has already failed, so it could only ever fire on something
that is not a date - and the caller then flipped the bracket to exclusive while
leaving the value untouched. `lte: "0000000010"` on a string field became
`serial_no:[0000000001 TO 0000000010}`, silently excluding the bound asked for.
Verified against the three real upstream nyc_taxis bodies: the output is now
byte-identical to what the reviewed workload ships by hand. Each of the seven
new tests fails on its own when the behaviour it describes is reverted, except
the one guarding `gte`/`lt`, which must NOT move.
fd8d9f5 to
03fd7dc
Compare
|
Hint - we normally dont force push PR branches unless necessary for other reasons (rebase etc), as it removes the commit stack as a history you can review. |
Closes #73.
Two date-bound defects in
_convert_date_to_solr_format, both independent of thebracket fix in #69 and both still present on
mainat f088380.A datetime with a space separator was passed through verbatim.
yyyy-MM-dd HH:mm:sswas missing from the format map, and an unknown format is logged and used as-is. A space
inside a range term ends the term, so Solr answers HTTP 400 and the operation cannot run
at all:
#69 gave that query a correct exclusive bracket, which does not help while the query
still fails to parse. Two nyc_taxis operations are affected.
A whole-day
ltewas not rounded. A bound with no time names a day, and OpenSearchrounds it to that day's edge —
lteandgtto its last millisecond,gteandlttoits first. Solr rounds nothing, so only the two that move to the end of the day need
translating.
dd/MM/yyyywas already in the map, so the value converted cleanly andsilently landed on midnight, dropping the rest of the day.
Note the bracket in that case is inclusive on
mainand stays inclusive here: upstreamwrites
lte, so]was always right. The only thing wrong was the value.Approach
The format map now records, per format, whether the pattern carries a time, so a
whole-day bound is recognisable after parsing rather than guessed at from its length.
Rounding then advances such a bound to the following day's first instant and makes the
bracket exclusive:
...T23:59:59.999Z]would be the more literal reading, but it assumes Solr's dateprecision is exactly a millisecond. Naming the next day's first instant says "the whole
of the 21st" without depending on that.
Verification
300,649 nyc_taxis documents indexed into Solr 10.0.0 and OpenSearch 3.8.0 from one
file, line number as the id on both sides. Provenance checked rather than assumed:
300,649 corpus lines against 300,649 OpenSearch documents, 10 sampled ids byte-identical,
and the Solr schema read back from the live core.
date_histogram_facetmatchesEqual counts can hide compensating differences, so I compared document id sets rather
than totals: before, 840 missing and 0 extra; after, the sets are equal. And the buckets
the operation actually returns:
2015-01-21is absent from the old output entirely, not merely short.Separately, the converter's output for the three real upstream nyc_taxis bodies is now
byte-identical to what the hand-written Solr port ships — two independent routes reaching
the same string.
5 new unit tests;
pytest tests/passes 1115, skips 5.apache/solr-orbit-workloads#19 corrected the same bounds in the checked-in
nyc_taxisfiles. This is the generator, so without it a regeneration reintroduces both.