serhiy-bzhezytskyy commented on code in PR #74:
URL: https://github.com/apache/solr-orbit/pull/74#discussion_r3931118901
##########
solrorbit/conversion/query.py:
##########
@@ -196,8 +196,16 @@ def _translate_query_node(node: dict, fq_list: list =
None) -> str:
hi, hi_bracket = "*", "]"
# Convert dates if format is specified (common for date fields)
os_format = bounds.get("format")
- lo = _convert_date_to_solr_format(lo, os_format)
- hi = _convert_date_to_solr_format(hi, os_format)
+ lo, lo_is_date_only = _convert_date_to_solr_format(lo, os_format)
+ hi, hi_is_date_only = _convert_date_to_solr_format(hi, os_format)
+ # A whole-day bound that OpenSearch rounds to the END of the day
+ # covers that day; Solr must be given the next day and told to
+ # exclude it. `gte`/`lt` round to the day's start, which is what
+ # the conversion above already produced.
+ if hi_is_date_only and "lte" in bounds:
+ hi, hi_bracket = _round_date_only_bound(hi), "}"
+ if lo_is_date_only and "gt" in bounds:
Review Comment:
Confirmed, and fixed in 03fd7dcf.
Reproduced before the fix:
```
{"gte": "2015-01-01", "gt": "2015-01-05", "format": "yyyy-MM-dd"}
-> dropoff_datetime:[2015-01-02T00:00:00Z TO *]
```
The lower bound is taken from `gte`, which rounds to the start of the day
and must not move, so the result was a day short.
Each branch now records which key it used, and the rounding is gated on that
rather than on a key being present.
`test_only_the_bound_that_is_used_is_rounded` covers it, and fails if the
condition goes back to `"gt" in bounds`.
##########
solrorbit/conversion/query.py:
##########
@@ -538,50 +561,62 @@ def _convert_date_to_solr_format(date_str,
os_format=None) -> str:
os_format: Optional OpenSearch date format pattern (e.g., "dd/MM/yyyy")
Returns:
- ISO 8601 date string for Solr (e.g., "2015-01-01T00:00:00Z")
+ (value, is_date_only) — the ISO 8601 date string for Solr
+ (e.g., "2015-01-01T00:00:00Z"), and whether the source named a whole
+ day rather than an instant.
If the date is already in ISO format or conversion fails, returns the
original string unchanged.
"""
if not isinstance(date_str, str) or date_str in ("*", "now"):
- return date_str
-
- # Map OpenSearch date format patterns to Python strptime format
- OS_TO_PYTHON_FORMAT = {
- "dd/MM/yyyy": "%d/%m/%Y",
- "MM/dd/yyyy": "%m/%d/%Y",
- "yyyy-MM-dd": "%Y-%m-%d",
- "yyyy/MM/dd": "%Y/%m/%d",
- "dd-MM-yyyy": "%d-%m-%Y",
- "MM-dd-yyyy": "%m-%d-%Y",
- # Add more as needed
- }
+ return date_str, False
# If format is provided, use it to parse the date
if os_format:
- python_fmt = OS_TO_PYTHON_FORMAT.get(os_format)
- if python_fmt:
+ pattern = OS_TO_PYTHON_FORMAT.get(os_format)
+ if pattern:
+ python_fmt, has_time = pattern
try:
dt = datetime.strptime(date_str, python_fmt)
- return dt.strftime("%Y-%m-%dT%H:%M:%SZ")
+ return dt.strftime("%Y-%m-%dT%H:%M:%SZ"), not has_time
except ValueError:
logger.warning(f"Failed to parse date '{date_str}' with format
'{os_format}'")
- return date_str
+ return date_str, False
else:
logger.warning(f"Unknown OpenSearch date format: '{os_format}'")
# Try common patterns if no format specified
- for python_fmt in OS_TO_PYTHON_FORMAT.values():
+ for python_fmt, has_time in OS_TO_PYTHON_FORMAT.values():
try:
dt = datetime.strptime(date_str, python_fmt)
- return dt.strftime("%Y-%m-%dT%H:%M:%SZ")
+ return dt.strftime("%Y-%m-%dT%H:%M:%SZ"), not has_time
except ValueError:
continue
# If it's already in ISO-like format, return as-is
# (handles cases like "2015-01-01T00:00:00Z" or partial ISO)
- if "T" in date_str or len(date_str) == 10: # YYYY-MM-DD
- return date_str
+ if "T" in date_str:
+ return date_str, False
+ if len(date_str) == 10: # YYYY-MM-DD, reached only if strptime rejected it
+ return date_str, True
Review Comment:
Confirmed, and the consequence was worse than a wrong flag:
`_round_date_only_bound` then failed to parse the value and returned it
unchanged, but the caller had already switched the bracket to exclusive. So the
bound moved from inclusive to exclusive while the value stayed put.
```
{"gte": "0000000001", "lte": "0000000010"} on a string field
-> serial_no:[0000000001 TO 0000000010}
```
That silently excludes the value asked for, and a zero-padded string id is
not a far-fetched input.
Rather than guard the rounding, I removed the branch in 03fd7dcf. It is
reachable only after every pattern in the map has failed strptime, and the map
covers `yyyy-MM-dd` and the five other ten-character forms - so a well-formed
date-only string never gets there, and the branch could only ever fire on
something that is not a date. `is_date_only` is now set only by a successful
parse. `test_a_bound_that_is_not_a_date_keeps_its_bracket` covers it, and fails
if the branch is put back.
--
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]
