Add range query support to DSL query executor with field-type-aware value handling

[ask-kamal-nayan]

Description

Adds RangeQueryTranslator to the dsl-query-executor sandbox plugin, converting DSL range queries into Calcite comparison expressions (>=, >, <=, <) for execution on the multi-engine analytics path.

This PR supersedes #21331: it carries the original implementation commits (authorship preserved), rebased onto current main, and adds behavioral fixes aligning the translator with legacy _search semantics, verified against the corresponding server code paths.

Supported after this PR

Category	Examples
Numeric ranges	{"range": {"price": {"gte": 100, "lte": 500}}}, numeric strings coerced ("gte": "100"), doubles, decimal bounds on integer fields ("gt": 10.5 matches values from 11)
Keyword ranges	Lexicographic: {"range": {"brand": {"gte": "apple", "lte": "dell"}}}
Date ranges	Default format, custom format (e.g. dd/MM/yyyy), epoch_millis, date math (now-7d, 2022-06-15||/M), rounding operators, time_zone
relation	INTERSECTS/CONTAINS/WITHIN accepted and ignored on scalar fields (matching SimpleMappedFieldType); DISJOINT rejected
No bounds	Converts to an exists predicate (IS NOT NULL), matching RangeQueryBuilder.doToQuery
Unmapped field	Converts to match-none, matching the legacy DISJOINT rewrite

Unsupported parameters fail fast with ConversionException (boost, _name) rather than silently changing result semantics.

Fixes on top of the original implementation

1. Field-type-gated value parsing — string values are interpreted by the target field's type: numeric fields coerce to numbers, keyword fields compare lexicographically, only date/timestamp fields go through DateMathParser. Previously every string was date-parsed, breaking numeric-string and keyword ranges.
2. Date rounding keyed on inclusivity — lower bound parses with roundUp=!includeLower, upper with roundUp=includeUpper, matching DateFieldMapper.dateRangeQuery. Previously gt/lt with day-granularity strings rounded the wrong direction and returned wrong results (e.g. "gt": "2022-01-01" included most of Jan 1). Also removes a false-positive where / date-format separators (e.g. "31/12/2022" with dd/MM/yyyy) suppressed end-of-day rounding.
3. epoch_millis is absolute — time_zone no longer shifts epoch values (was silently off by the zone offset when combined).
4. Literal typing by field type — TIMESTAMP(3) literals are produced only for date-typed fields. Previously any Long bound became a timestamp, even on numeric fields.
5. Decimal bounds on integer-family fields — legacy NumberFieldMapper truncate-and-adjust semantics ("gt": 10.5 → matches from 11; "lt": -10.5 → matches through -11), applied identically to string-origin decimals, with overflow guards at each type's MIN/MAX returning match-none instead of wrapping around.
6. No-bounds range is legal — converts to IS_NOT_NULL, matching the legacy exists-query rewrite (previously an error).
7. Unmapped field returns no matches — converts to a match-none predicate, matching legacy behavior (previously an error).
8. relation parity on scalar fields — CONTAINS/WITHIN are accepted and ignored exactly as SimpleMappedFieldType.rangeQuery does; only DISJOINT is rejected. Previously all non-INTERSECTS values were rejected, breaking queries that work on the legacy path.
9. _name (queryName) rejected — consistent with the other translators' unsupported-parameter handling; previously silently ignored (matched_queries cannot be populated on this path).

Testing

• RangeQueryTranslatorTests: 62 tests — original shape assertions plus value-level assertions (exact epoch millis for format/timezone/rounding paths, literal types, coercion, overflow boundaries), written test-first: 18 tests documented the bugs above before the fixes
• Full dsl-query-executor module: 202 tests, 0 failures
• ./gradlew -Dsandbox.enabled=true :sandbox:plugins:dsl-query-executor:test
• DslRangeQueryIT migrated to current test-framework APIs (indexDoc removal, TotalHits.value()) and annotated @AwaitsFix with the same condition as the plugin's other integration tests (analytics E2E pipeline not yet wired); it compiles and will activate with them

Notes for reviewers

• Calcite canonically types exact-numeric literals as DECIMAL (RexLiteral rejects INTEGER in strict mode); tests assert value correctness + not-TIMESTAMP rather than a specific numeric SqlTypeName
• Known deliberate divergences from legacy, documented rather than silently approximated: boost (no scoring on this path yet), _name (no per-hit matched_queries side-channel), ranges over analyzed text fields (legacy compares analyzed terms; this path compares stored values), allowExpensiveQueries gate (cluster-setting concern, different execution model)
• Follow-ups, out of scope: injectable clock for deterministic now-based date-math tests; TIME-typed fields currently receive date treatment via isDateType (no such fields exist on this path in practice)

Related Issues

Relates to #20914
Supersedes #21331

Check List

• Functionality includes testing
• New functionality has javadoc added
• Commits are signed per the DCO using --signoff

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 84652b1)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incorrect narrowing for SMALLINT/TINYINT narrowToFieldType casts the long to int for INTEGER, SMALLINT, and TINYINT alike, returning an Integer for SMALLINT/TINYINT fields. Callers of getMaxValueForType/getMinValueForType correctly use Short.MAX_VALUE/Byte.MAX_VALUE, but the produced literal is an Integer that may exceed the field's declared range (e.g., a truncated value of 200 on a TINYINT field will be emitted as Integer 200). Consider returning Short/Byte (or otherwise narrowing correctly) for these types. * queries). DISJOINT is rejected, though RangeQueryBuilder itself rejects it at builder * level before our translator is invoked. * <p> * Field-type gating for string values (when no format/timeZone/date-math indicators): * - TIMESTAMP/DATE fields: parsed through DateMathParser * - Numeric fields: coerced to number using the field's type Signum treats zero decimals like negatives For a lower bound whose value is exactly 0.5 with signum > 0 the code increments correctly, but for value like -0.5, signum is -1 and the code returns truncated 0 unchanged (adjustedInclusive=true), producing >= 0. That matches legacy for lower bounds. However for value 0.5 as upper bound: signum > 0, upper branch keeps truncated 0 → <= 0. Legacy NumberFieldMapper.INTEGER.rangeQuery for upper 0.5 with hasDecimal & signum>0 keeps as-is, matching. Please double-check the signum == 0 case (value like 0.0 is filtered by hasDecimalPart==false, so unreachable) and confirm the fractional-only negatives such as -0.5 upper bound (signum<0, decrement) result in <= -1, which is what the current code produces — appears correct, but the logic is subtle and worth an explicit test for |value| < 1 decimals. if (RangeBoundMath.isIntegerType(fieldTypeName) && RangeBoundMath.hasDecimalPart(value)) { long truncated = RangeBoundMath.toLongValue(value); if (isLower) { // Positive decimal lower bound -> increment if (RangeBoundMath.signum(value) > 0) { if (truncated >= RangeBoundMath.getMaxValueForType(fieldTypeName)) { return ctx.getRexBuilder().makeLiteral(false); } adjusted = RangeBoundMath.narrowToFieldType(truncated + 1, fieldTypeName); } else { adjusted = RangeBoundMath.narrowToFieldType(truncated, fieldTypeName); } } else { // Negative decimal upper bound -> decrement if (RangeBoundMath.signum(value) < 0) { if (truncated <= RangeBoundMath.getMinValueForType(fieldTypeName)) { return ctx.getRexBuilder().makeLiteral(false); } adjusted = RangeBoundMath.narrowToFieldType(truncated - 1, fieldTypeName); } else { adjusted = RangeBoundMath.narrowToFieldType(truncated, fieldTypeName); } } adjustedInclusive = true; // decimal adjustment makes bound inclusive } else if (RangeBoundMath.isIntegerType(fieldTypeName) && !RangeBoundMath.hasDecimalPart(value) && value instanceof Number) { // Whole numeric value on integer field: narrow to field-appropriate type for Calcite adjusted = RangeBoundMath.narrowToFieldType(((Number) value).longValue(), fieldTypeName); } Timezone ignored on epoch_millis nanos path In parseDateValueNanos, the epoch_millis branch computes maxMillis using integer division MAX_NANOSECOND_INSTANT.getNano() / 1_000_000, which drops sub-millisecond nanos from the ceiling. Values between this truncated maxMillis and the true max representable millisecond boundary will be clamped to MAX_NANOSECOND_INSTANT rather than converted via millis * 1_000_000, causing a subtle off-by-nanos discrepancy versus the non-epoch_millis path. Also, unlike the millis path, no explicit note is given that epoch_millis skips timezone (consistent, but worth verifying it matches legacy DateFieldMapper nanos behavior). static Long parseDateValueNanos(String strValue, String format, String timeZone, boolean roundUp) throws ConversionException { try { if ("epoch_millis".equals(format)) { try { long millis = Long.parseLong(strValue); // Legacy DateUtils.toNanoSeconds: rejects negative, rejects past max if (millis < 0) { throw new ConversionException( "Failed to parse epoch_millis value '" + strValue + "': value before epoch not representable in nanos" ); } long maxMillis = MAX_NANOSECOND_INSTANT.getEpochSecond() * 1000 + MAX_NANOSECOND_INSTANT.getNano() / 1_000_000; if (millis > maxMillis) { return instantToNanos(MAX_NANOSECOND_INSTANT); } return millis * 1_000_000L; } catch (NumberFormatException e) { throw new ConversionException("Failed to parse epoch_millis value '" + strValue + "': not a valid number"); } } DateFormatter formatter = format != null ? DateFormatter.forPattern(format) : DateFormatter.forPattern("strict_date_optional_time"); ZoneId zoneId = timeZone != null ? ZoneId.of(timeZone) : ZoneId.of("UTC"); Instant instant = formatter.toDateMathParser().parse(strValue, System::currentTimeMillis, roundUp, zoneId); // Clamp per legacy DateUtils.clampToNanosRange instant = clampToNanosRange(instant); return instantToNanos(instant); } catch (ConversionException e) { throw e; } catch (Exception e) { throw new ConversionException("Failed to parse date value '" + strValue + "': " + e.getMessage()); } } Boost comparison via != on float rangeQuery.boost() != 1.0f will reject any boost that isn't exactly 1.0f, which is the intended semantics, but users constructing a builder that explicitly sets boost(1.0f) are fine. However, legacy AbstractQueryBuilder defines the default as DEFAULT_BOOST = 1.0f; if any code path sets a non-default sentinel or NaN, direct float equality can misbehave. Consider using Float.compare(rangeQuery.boost(), AbstractQueryBuilder.DEFAULT_BOOST) != 0 for clarity and to correctly handle NaN. if (rangeQuery.boost() != 1.0f) { throw new ConversionException("Range query 'boost' parameter is not supported"); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 84652b1

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard integer-field bounds against out-of-range values A whole Number value that exceeds the field's range (e.g., gte(300) on a TINYINT field, or gte(Long.MAX_VALUE) on INTEGER) is silently narrowed via (int) longValue, producing an incorrect literal and matching wrong rows. Add an overflow guard that returns match-none (literal false) for lower bounds above max / upper bounds below min, mirroring NumberFieldMapper semantics. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [242-245] } else if (RangeBoundMath.isIntegerType(fieldTypeName) && !RangeBoundMath.hasDecimalPart(value) && value instanceof Number) { - // Whole numeric value on integer field: narrow to field-appropriate type for Calcite - adjusted = RangeBoundMath.narrowToFieldType(((Number) value).longValue(), fieldTypeName); + long lv = ((Number) value).longValue(); + long max = RangeBoundMath.getMaxValueForType(fieldTypeName); + long min = RangeBoundMath.getMinValueForType(fieldTypeName); + if (isLower && lv > max) return ctx.getRexBuilder().makeLiteral(false); + if (!isLower && lv < min) return ctx.getRexBuilder().makeLiteral(false); + if (lv > max) lv = max; + if (lv < min) lv = min; + adjusted = RangeBoundMath.narrowToFieldType(lv, fieldTypeName); } Suggestion importance[1-10]: 7 __ Why: This is a legitimate correctness concern: whole numeric bounds exceeding the field's integer range are silently truncated and could match wrong rows. Adding overflow guards mirrors legacy NumberFieldMapper semantics for out-of-range bounds.	Medium
	Prevent silent overflow when narrowing to int Narrowing a long to (int) silently truncates values outside int range for SMALLINT/TINYINT/INTEGER, which can produce incorrect literals when overflow guards did not fire (e.g., when the value came from a non-decimal path). Consider using Math.toIntExact or clamping explicitly so unexpected overflow surfaces as an error rather than silent wrap-around. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeBoundMath.java [82-87] static Number narrowToFieldType(long value, SqlTypeName typeName) { if (typeName == SqlTypeName.BIGINT) { return value; } - return (int) value; + return Math.toIntExact(value); } Suggestion importance[1-10]: 6 __ Why: Silent truncation via (int) value can lead to incorrect query results for whole-number bounds outside the field's range. Using Math.toIntExact would surface such errors, though the caller in RangeQueryTranslator should handle bounds explicitly.	Low
	Prevent overflow in epoch_millis to nanos conversion millis * 1_000_000L can overflow long for very large millisecond values that pass the maxMillis check only by a narrow margin, because maxMillis truncates the sub-millisecond nanos of MAX_NANOSECOND_INSTANT. Any value in (maxMillis, maxMillisExact] reaches the multiplication and can silently overflow. Compute an exact maxMillis (or use Math.multiplyExact) so all out-of-range inputs are clamped. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [98-111] if ("epoch_millis".equals(format)) { try { long millis = Long.parseLong(strValue); - // Legacy DateUtils.toNanoSeconds: rejects negative, rejects past max if (millis < 0) { throw new ConversionException( "Failed to parse epoch_millis value '" + strValue + "': value before epoch not representable in nanos" ); } - long maxMillis = MAX_NANOSECOND_INSTANT.getEpochSecond() * 1000 + MAX_NANOSECOND_INSTANT.getNano() / 1_000_000; + // Exact largest millis that fits within nanos-representable range + long maxMillis = Math.floorDiv(instantToNanos(MAX_NANOSECOND_INSTANT), 1_000_000L); if (millis > maxMillis) { return instantToNanos(MAX_NANOSECOND_INSTANT); } - return millis * 1_000_000L; + return Math.multiplyExact(millis, 1_000_000L); Suggestion importance[1-10]: 5 __ Why: Valid edge case: maxMillis truncates sub-millisecond nanos so values in a narrow range slip past the guard and could theoretically overflow during multiplication. Impact is limited to a very narrow input band near the max nanosecond instant.	Low
General	Guard nanos conversion against pre-epoch inputs For instants before epoch, getNano() returns a non-negative nanosecond-of-second while getEpochSecond() is negative, so epochSec * 1e9 + nano produces an incorrect value. Although clampToNanosRange guards this in the main flow, the helper is unsafe if reused. Use Math.addExact or explicitly document that the input must be >= Instant.EPOCH. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [151-153] static long instantToNanos(Instant instant) { + if (instant.isBefore(Instant.EPOCH)) { + throw new IllegalArgumentException("instantToNanos requires instant >= EPOCH; got " + instant); + } return instant.getEpochSecond() * 1_000_000_000L + instant.getNano(); } Suggestion importance[1-10]: 3 __ Why: The instantToNanos helper is currently only called after clampToNanosRange which ensures the instant is >= EPOCH, so this is a defensive hardening rather than a real bug. Minor maintainability improvement.	Low

---

Previous suggestions

Suggestions up to commit 4c9c0cb

Category	Suggestion	Impact
General	Tighten date-math detection for "now" prefix value.startsWith("now") incorrectly classifies any string beginning with "now" (e.g., "november", "nowhere") as a date-math expression, which will then be date-parsed even on non-date fields and likely throw a confusing error. Tighten the check to require that "now" be followed by end-of-string or a date-math operator (+, -, /). sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [42-44] static boolean isDateMathExpression(String value) { - return value.startsWith("now") || value.contains("||"); + if (value.contains("||")) { + return true; + } + if (value.startsWith("now")) { + if (value.length() == 3) return true; + char c = value.charAt(3); + return c == '+' || c == '-' || c == '/'; + } + return false; } Suggestion importance[1-10]: 7 __ Why: Legitimate correctness issue: strings like "november" would be incorrectly classified as date-math and cause parsing errors on non-date fields. The tightened check aligns better with actual date-math syntax.	Medium
	Narrow CoercedNumber values on integer fields When the incoming value is a CoercedNumber wrapping a whole number (e.g., "100" on INTEGER), the value instanceof Number check is false, so no narrowing happens and the value stays wrapped in CoercedNumber. This is inconsistent with raw whole numeric values on integer fields, which do get narrowed. Handle the CoercedNumber case explicitly to ensure consistent literal typing between raw and string-coerced numeric inputs. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [242-245] -} else if (RangeBoundMath.isIntegerType(fieldTypeName) && !RangeBoundMath.hasDecimalPart(value) && value instanceof Number) { - // Whole numeric value on integer field: narrow to field-appropriate type for Calcite - adjusted = RangeBoundMath.narrowToFieldType(((Number) value).longValue(), fieldTypeName); +} else if (RangeBoundMath.isIntegerType(fieldTypeName) && !RangeBoundMath.hasDecimalPart(value)) { + if (value instanceof Number) { + adjusted = RangeBoundMath.narrowToFieldType(((Number) value).longValue(), fieldTypeName); + } else if (value instanceof CoercedNumber) { + adjusted = RangeBoundMath.narrowToFieldType(((CoercedNumber) value).value.longValue(), fieldTypeName); + } } Suggestion importance[1-10]: 6 __ Why: Valid observation that CoercedNumber wrapping a whole number doesn't get narrowed like raw numbers, leading to inconsistent literal typing between raw and string-coerced numeric inputs on integer fields.	Low
	Avoid manual millis-to-nanos overflow arithmetic The multiplication millis * 1_000_000L can overflow silently for large millis values that pass the maxMillis check boundary but are close to it — actually the check bounds this correctly, but any millis slightly below maxMillis still multiplies safely. However, the maxMillis computation itself uses integer division which truncates the fractional millisecond, so a value equal to maxMillis still multiplies to less than the true max nanos. Consider clamping via Instant conversion instead: Instant.ofEpochMilli(millis) then instantToNanos(clampToNanosRange(...)) to avoid the overflow-adjacent arithmetic and stay consistent with the non-epoch path. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [107-111] -long maxMillis = MAX_NANOSECOND_INSTANT.getEpochSecond() * 1000 + MAX_NANOSECOND_INSTANT.getNano() / 1_000_000; -if (millis > maxMillis) { - return instantToNanos(MAX_NANOSECOND_INSTANT); -} -return millis * 1_000_000L; +Instant instant = clampToNanosRange(Instant.ofEpochMilli(millis)); +return instantToNanos(instant); Suggestion importance[1-10]: 4 __ Why: The suggestion proposes a cleaner implementation using existing helpers, but the current code already guards against overflow via the maxMillis check. The improvement is stylistic rather than fixing a real bug.	Low
Possible issue	Fix silent truncation when narrowing to SMALLINT/TINYINT Narrowing a long to int via a plain cast for SMALLINT/TINYINT silently truncates high bits and can flip sign, producing incorrect bounds when the truncated value exceeds Short/Byte range. Since overflow guards use getMaxValueForType/getMinValueForType per type, the narrowing should also respect those limits (or at minimum only narrow to int for INTEGER and keep long otherwise), so callers get a value consistent with the guard checks. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeBoundMath.java [82-87] static Number narrowToFieldType(long value, SqlTypeName typeName) { if (typeName == SqlTypeName.BIGINT) { return value; } + if (typeName == SqlTypeName.SMALLINT) { + return (int) (short) value; + } + if (typeName == SqlTypeName.TINYINT) { + return (int) (byte) value; + } return (int) value; } Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a real edge case where narrowing to SMALLINT/TINYINT via a plain int cast doesn't reflect the actual field width, though in practice overflow guards limit the affected range. The improvement is a minor correctness enhancement for consistency.	Low

Suggestions up to commit 3369ce7

Category	Suggestion	Impact
Possible issue	Prevent silent truncation on narrowing Narrowing a long to int via cast silently discards high bits when the value exceeds Integer.MAX_VALUE (or is below Integer.MIN_VALUE), producing wrong bounds for SMALLINT/TINYINT/INTEGER without any error. Since the overflow guard in the caller only checks against the field-type max/min before +1/-1, a raw whole-number value larger than the type's range would silently truncate. Add a range check here or use Math.toIntExact to fail fast. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeBoundMath.java [82-87] static Number narrowToFieldType(long value, SqlTypeName typeName) { if (typeName == SqlTypeName.BIGINT) { return value; } + long max = getMaxValueForType(typeName); + long min = getMinValueForType(typeName); + if (value > max || value < min) { + throw new ArithmeticException("Value " + value + " out of range for " + typeName); + } return (int) value; } Suggestion importance[1-10]: 5 __ Why: Silent truncation from long to int could produce incorrect bounds for values outside INTEGER range that aren't caught by the caller's overflow guard (which only applies to decimal-adjusted paths). Adding a range check improves correctness for edge cases.	Low
General	Guard against overflow in nano conversion The instantToNanos computation can silently overflow for negative epoch seconds since getNano() is always non-negative (0-999,999,999), causing incorrect nanos for pre-epoch instants. Although clampToNanosRange prevents this at call sites, this public helper is unsafe if reused. Add a guard or comment explicitly requiring the instant is post-epoch, and consider using Math.addExact/multiplyExact to fail loudly on overflow. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [151-153] static long instantToNanos(Instant instant) { - return instant.getEpochSecond() * 1_000_000_000L + instant.getNano(); + if (instant.isBefore(Instant.EPOCH)) { + throw new IllegalArgumentException("instantToNanos requires post-epoch instant; got " + instant); + } + return Math.addExact(Math.multiplyExact(instant.getEpochSecond(), 1_000_000_000L), instant.getNano()); } Suggestion importance[1-10]: 4 __ Why: The helper is only called after clampToNanosRange, so the overflow is not actually reachable in current code. The suggestion adds defensive robustness for future reuse but has limited immediate impact.	Low
	Validate IP bound value types explicitly Using String.valueOf(rangeQuery.from()) will coerce non-string bound values (e.g. InetAddress or numeric) into potentially unexpected string forms; if a caller passes a non-string IP object, this may yield an unparseable string. Explicitly validate that the bound is a String or InetAddress and reject others with a clear ConversionException instead of relying on toString. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [271-276] if (rangeQuery.from() != null) { - byte[] fromBytes = encodeIpAsIpv6(String.valueOf(rangeQuery.from())); + Object from = rangeQuery.from(); + String fromStr = (from instanceof String) ? (String) from + : (from instanceof InetAddress) ? InetAddresses.toAddrString((InetAddress) from) + : null; + if (fromStr == null) { + throw new ConversionException("IP range bound must be a String or InetAddress, got: " + from.getClass().getName()); + } + byte[] fromBytes = encodeIpAsIpv6(fromStr); RexNode literal = ctx.getRexBuilder().makeLiteral(new ByteString(fromBytes), varbinaryType, false); SqlOperator op = rangeQuery.includeLower() ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL : SqlStdOperatorTable.GREATER_THAN; conditions.add(ctx.getRexBuilder().makeCall(op, fieldRef, literal)); } Suggestion importance[1-10]: 4 __ Why: Using String.valueOf on non-string IP bounds could produce unparseable strings, but encodeIpAsIpv6 will throw a ConversionException for invalid strings anyway, so the impact is limited to error message clarity.	Low
	Use exact arithmetic for bound adjustment The overflow guard compares truncated >= max but the increment truncated + 1 can also overflow when truncated == Long.MAX_VALUE for BIGINT since getMaxValueForType(BIGINT) returns Long.MAX_VALUE — this is correctly guarded. However, for values above Long.MAX_VALUE (e.g., 1e20 doubles), toLongValue returns Long.MAX_VALUE due to Java narrowing, and the guard triggers correctly. Still, consider using Math.addExact defensively for the truncated + 1 and truncated - 1 operations to catch any missed edge cases. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [222-229] if (RangeBoundMath.signum(value) > 0) { if (truncated >= RangeBoundMath.getMaxValueForType(fieldTypeName)) { return ctx.getRexBuilder().makeLiteral(false); } - adjusted = RangeBoundMath.narrowToFieldType(truncated + 1, fieldTypeName); + adjusted = RangeBoundMath.narrowToFieldType(Math.addExact(truncated, 1), fieldTypeName); } else { adjusted = RangeBoundMath.narrowToFieldType(truncated, fieldTypeName); } Suggestion importance[1-10]: 3 __ Why: The overflow guard already correctly prevents truncated + 1 from overflowing since it checks truncated >= max. The suggested Math.addExact is defensive but redundant given the existing guard.	Low

Suggestions up to commit 36c559f

Category	Suggestion	Impact
General	Narrow correctly per integer sub-type Narrowing all non-BIGINT integer types (INTEGER, SMALLINT, TINYINT) with (int) value silently truncates values that overflow the smaller type's range for SMALLINT and TINYINT. Since getMaxValueForType/getMinValueForType already model narrower ranges, narrowing should use the actual type's Java representation (e.g., (short) for SMALLINT, (byte) for TINYINT) to avoid producing incorrect literals that fall outside the field's domain. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeBoundMath.java [82-87] static Number narrowToFieldType(long value, SqlTypeName typeName) { - if (typeName == SqlTypeName.BIGINT) { - return value; + switch (typeName) { + case BIGINT: + return value; + case SMALLINT: + return (short) value; + case TINYINT: + return (byte) value; + case INTEGER: + default: + return (int) value; } - return (int) value; } Suggestion importance[1-10]: 5 __ Why: Valid point that (int) truncation for SMALLINT/TINYINT doesn't match the narrower type ranges, though in practice the overflow guards using getMaxValueForType/getMinValueForType limit exposure. It's a correctness improvement for consistency.	Low
	Reject irrelevant params on IP fields convertIpRange does not validate the relation parameter or boost/queryName (those are checked earlier, good), but it also doesn't reject format or timeZone on IP fields, which are meaningless for IPs. Silently ignoring these can mask user errors. Consider rejecting format/timeZone with a clear ConversionException when the field is IP-typed. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [132-134] if (field.getType() instanceof IpType) { + if (rangeQuery.format() != null || rangeQuery.timeZone() != null) { + throw new ConversionException("Range query 'format'/'time_zone' parameters are not supported for IP fields"); + } return convertIpRange(rangeQuery, field, ctx); } Suggestion importance[1-10]: 5 __ Why: Reasonable suggestion to fail loudly on meaningless parameters for IP fields to avoid silent misconfiguration, though this is a minor UX/correctness enhancement.	Low
	Guard nanos conversion against overflow This can overflow for extreme (but valid Instant) inputs before clamping is applied. Since callers rely on this to produce a bounded nanos value, either assert the input is already clamped or use Math.addExact/Math.multiplyExact to fail loudly on any unclamped path, preventing silent wrap-around producing incorrect epoch nanos. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [151-153] static long instantToNanos(Instant instant) { - return instant.getEpochSecond() * 1_000_000_000L + instant.getNano(); + return Math.addExact(Math.multiplyExact(instant.getEpochSecond(), 1_000_000_000L), instant.getNano()); } Suggestion importance[1-10]: 3 __ Why: Callers clamp before calling instantToNanos, so overflow is already prevented. The defensive change adds minor robustness but has low practical impact.	Low
Possible issue	Prevent silent overflow in nanos conversion Multiplying millis * 1_000_000L can overflow silently for values near Long.MAX_VALUE / 1_000_000, even after the maxMillis check computed with integer arithmetic. Use Math.multiplyExact or compare against a constant like MAX_NANOSECOND_INSTANT-derived millisecond ceiling using a safe long constant to guarantee no overflow occurs before clamping. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [107-111] -long maxMillis = MAX_NANOSECOND_INSTANT.getEpochSecond() * 1000 + MAX_NANOSECOND_INSTANT.getNano() / 1_000_000; +long maxMillis = MAX_NANOSECOND_INSTANT.getEpochSecond() * 1000L + MAX_NANOSECOND_INSTANT.getNano() / 1_000_000L; if (millis > maxMillis) { return instantToNanos(MAX_NANOSECOND_INSTANT); } -return millis * 1_000_000L; +return Math.multiplyExact(millis, 1_000_000L); Suggestion importance[1-10]: 4 __ Why: The maxMillis check does bound the value below overflow threshold since MAX_NANOSECOND_INSTANT corresponds to ~year 2262, so millis * 1_000_000L shouldn't overflow after the check. The suggestion offers defensive coding but limited practical impact.	Low

Suggestions up to commit 7b4abfd

Category	Suggestion	Impact
Possible issue	Guard integer overflow on whole-number bounds For whole numeric values on integer fields, values outside the field's representable range are silently narrowed (e.g., 100000 on SMALLINT wraps to a valid-looking int). Add an overflow check that returns literal false (match-none) or clamps, mirroring the legacy NumberFieldMapper behavior for out-of-range bounds. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [242-245] } else if (RangeBoundMath.isIntegerType(fieldTypeName) && !RangeBoundMath.hasDecimalPart(value) && value instanceof Number) { - // Whole numeric value on integer field: narrow to field-appropriate type for Calcite - adjusted = RangeBoundMath.narrowToFieldType(((Number) value).longValue(), fieldTypeName); + long longVal = ((Number) value).longValue(); + long max = RangeBoundMath.getMaxValueForType(fieldTypeName); + long min = RangeBoundMath.getMinValueForType(fieldTypeName); + if (longVal > max) { + return isLower ? ctx.getRexBuilder().makeLiteral(false) : createLiteral(RangeBoundMath.narrowToFieldType(max, fieldTypeName), field, ctx, fieldTypeName); + } + if (longVal < min) { + return isLower ? createLiteral(RangeBoundMath.narrowToFieldType(min, fieldTypeName), field, ctx, fieldTypeName) : ctx.getRexBuilder().makeLiteral(false); + } + adjusted = RangeBoundMath.narrowToFieldType(longVal, fieldTypeName); } Suggestion importance[1-10]: 6 __ Why: Legitimate concern: whole-number bounds outside the field's integer range are silently narrowed rather than producing match-none/clamp semantics matching legacy NumberFieldMapper behavior.	Low
	Narrow integer values to correct width The narrowing cast (int) value for SMALLINT/TINYINT can silently truncate values that fit in INTEGER but not the target type, defeating the overflow guard. Narrow to the correct width so callers get the right type and out-of-range values are handled consistently with getMax/MinValueForType. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeBoundMath.java [82-87] static Number narrowToFieldType(long value, SqlTypeName typeName) { if (typeName == SqlTypeName.BIGINT) { return value; } + if (typeName == SqlTypeName.SMALLINT) { + return (int) (short) value; + } + if (typeName == SqlTypeName.TINYINT) { + return (int) (byte) value; + } return (int) value; } Suggestion importance[1-10]: 5 __ Why: Valid concern about SMALLINT/TINYINT narrowing, though callers use getMax/MinValueForType overflow checks before calling this, so impact is limited. The improvement adds correctness for edge cases.	Low
General	Prevent silent overflow in nanos conversion The multiplication millis * 1_000_000L can overflow silently when millis is close to Long.MAX_VALUE / 1_000_000 but still below maxMillis. Also, since epoch_millis is already accepted up to maxMillis, but valid input like millis == maxMillis should not lose the sub-millisecond nano portion — however that is fine; the real bug is that Math.multiplyExact or explicit bound check is missing. Use Math.multiplyExact (or clamp beforehand) to fail fast rather than silently wrap. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeDateParsing.java [107-111] long maxMillis = MAX_NANOSECOND_INSTANT.getEpochSecond() * 1000 + MAX_NANOSECOND_INSTANT.getNano() / 1_000_000; if (millis > maxMillis) { return instantToNanos(MAX_NANOSECOND_INSTANT); } -return millis * 1_000_000L; +try { + return Math.multiplyExact(millis, 1_000_000L); +} catch (ArithmeticException e) { + return instantToNanos(MAX_NANOSECOND_INSTANT); +} Suggestion importance[1-10]: 3 __ Why: The suggestion is largely defensive; millis is already bounded by maxMillis check above, so overflow is not actually possible in the current code path. Minor robustness improvement.	Low

Suggestions up to commit 45bcca2

Category	Suggestion	Impact
General	Narrow CoercedNumber whole values on integer fields When from is a CoercedNumber wrapping a whole number (e.g., string "100" on an INTEGER field), from instanceof Number is false, so this branch is skipped and the wrapped value is passed to createLiteral still as a CoercedNumber. That is handled, but it means SMALLINT/TINYINT literals coming from string coercion won't be narrowed to Integer here. Handle the CoercedNumber case explicitly or unwrap it before this check to ensure consistent narrowing. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [157-160] -} else if (isIntegerType(fieldTypeName) && !hasDecimalPart(from) && from instanceof Number) { - // Whole numeric value on integer field: narrow to field-appropriate type for Calcite - adjustedFrom = narrowToFieldType(((Number) from).longValue(), fieldTypeName); +} else if (isIntegerType(fieldTypeName) && !hasDecimalPart(from)) { + if (from instanceof Number) { + adjustedFrom = narrowToFieldType(((Number) from).longValue(), fieldTypeName); + } else if (from instanceof CoercedNumber) { + adjustedFrom = narrowToFieldType(((CoercedNumber) from).value.longValue(), fieldTypeName); + } } Suggestion importance[1-10]: 6 __ Why: Valid observation: CoercedNumber does not extend Number, so the whole-number narrowing branch is skipped for string-coerced values on SMALLINT/TINYINT fields, potentially producing incorrectly typed literals. This is a real correctness gap.	Low
	Detect ip/binary types more precisely Gating on SqlTypeName.VARBINARY alone conflates ip fields and generic binary fields, and also rejects any other custom VARBINARY-mapped types that might legitimately support ranges. Prefer detecting the specific field type (e.g., via a marker type like IpType) rather than the generic SQL type, so future VARBINARY-backed types are not incorrectly rejected. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [119-121] -if (field.getType().getSqlTypeName() == SqlTypeName.VARBINARY) { +SqlTypeName sqlType = field.getType().getSqlTypeName(); +if (sqlType == SqlTypeName.VARBINARY || field.getType() instanceof org.opensearch.analytics.schema.IpType) { throw new ConversionException("Range queries on ip and binary fields are not supported by the DSL conversion path"); } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about type-based gating being too coarse, but based on TestUtils the IpType maps to VARBINARY so the current check does catch it. The improvement is minor and speculative about future custom types.	Low
	Improve error messages for format/timezone DateFormatter.forPattern and ZoneId.of may throw IllegalArgumentException / DateTimeException for invalid patterns/zones, which are caught by the generic catch (Exception e) below and wrapped as ConversionException. However, this means malformed format strings become confusing "Failed to parse date value" messages. Consider validating/format-erroring separately from value-parsing errors to make user-facing errors clearer. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [315-320] -DateFormatter formatter = format != null - ? DateFormatter.forPattern(format) - : DateFormatter.forPattern("strict_date_optional_time"); -ZoneId zoneId = timeZone != null ? ZoneId.of(timeZone) : ZoneId.of("UTC"); - +DateFormatter formatter; +try { + formatter = format != null ? DateFormatter.forPattern(format) : DateFormatter.forPattern("strict_date_optional_time"); +} catch (IllegalArgumentException e) { + throw new ConversionException("Invalid date format '" + format + "': " + e.getMessage()); +} +ZoneId zoneId; +try { + zoneId = timeZone != null ? ZoneId.of(timeZone) : ZoneId.of("UTC"); +} catch (Exception e) { + throw new ConversionException("Invalid timezone '" + timeZone + "': " + e.getMessage()); +} return formatter.toDateMathParser().parse(strValue, System::currentTimeMillis, roundUp, zoneId).toEpochMilli(); Suggestion importance[1-10]: 3 __ Why: The suggestion is a valid error-message clarity improvement but is a minor usability enhancement, not addressing correctness or a significant bug.	Low
	Handle overflow guard consistently across bounds When both from and to are provided and one bound produces the overflow-driven match-none literal, the method returns false immediately, discarding the other bound. However, per the current control flow this is fine, but for the to overflow branch you also return early after processing from conditions already added. Ensure returning false is truly equivalent to no-match when combined with the other bound; using AND with a false literal is semantically identical, so this is safe—but consider adding the literal to conditions instead of returning early to keep the code path uniform and avoid subtle behavior changes if additional side effects are ever added. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java [145-156] if (isIntegerType(fieldTypeName) && hasDecimalPart(from)) { long truncated = toLongValue(from); if (signum(from) > 0) { - // Overflow guard: if truncated == type MAX, ++l would overflow -> match-none if (truncated >= getMaxValueForType(fieldTypeName)) { return ctx.getRexBuilder().makeLiteral(false); } adjustedFrom = narrowToFieldType(truncated + 1, fieldTypeName); } else { adjustedFrom = narrowToFieldType(truncated, fieldTypeName); } - fromInclusive = true; // decimal adjustment makes bound inclusive + fromInclusive = true; } Suggestion importance[1-10]: 2 __ Why: The suggestion acknowledges the current behavior is safe and semantically identical, and the improved_code is essentially the same as existing_code with only a comment removed. This offers minimal value.	Low

[github-actions]

❌ Gradle check result for 9268cfc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit fd9f479

[github-actions]

Persistent review updated to latest commit cd677aa

[github-actions]

❌ Gradle check result for cd677aa: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 8ed6f07

[github-actions]

Persistent review updated to latest commit a667826

[github-actions]

❌ Gradle check result for a667826: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit f909d76

[github-actions]

✅ Gradle check result for f909d76: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.50%. Comparing base (0213338) to head (9b6c870).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22511      +/-   ##
============================================
+ Coverage     73.48%   73.50%   +0.02%     
- Complexity    76460    76510      +50     
============================================
  Files          6104     6104              
  Lines        346582   346582              
  Branches      49879    49879              
============================================
+ Hits         254675   254746      +71     
+ Misses        71664    71596      -68     
+ Partials      20243    20240       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 564b67c

[github-actions]

❌ Gradle check result for 564b67c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 4d8e3ae

[github-actions]

✅ Gradle check result for 4d8e3ae: SUCCESS

[nssuresh2007]

Instead of adding separate tests, would it be possible to utilize the extended test framework like this: https://github.com/nssuresh2007/OpenSearch/blob/main/sandbox/plugins/dsl-query-executor/src/test/resources/golden/terms_with_avg_aggregation.json to define input DSL, RelNode and the output in a single file?

[github-actions]

Persistent review updated to latest commit 9b6c870

[github-actions]

✅ Gradle check result for 9b6c870: SUCCESS

[nssuresh2007]

date_nanos field uses TIMESTAMP(9) precision. Using 3 would lower that precision value right? Is there a way to avoid it?

[ask-kamal-nayan]

Correct — precision 3 here would silently truncate date_nanos values. So for now added check to reject nanosecond-precision date fields with a ConversionException so results stay faithful. Full support is blocked by Calcite's default RelDataTypeSystem clamping timestamp literals to precision 3.

[nssuresh2007]

Can you check if this would work for scaled_float and unsigned_long as well, to confirm if those would not be truncated?

[ask-kamal-nayan]

Good catch, checked, and both would hit this truncation path: the schema maps them to plain BIGINT, so the translator can't distinguish them from long (scaled_float's scaling_factor never reaches the translator; unsigned_long values above Long.MAX_VALUE wrap under the signed narrowing). Fixing this needs schema-level type markers.

[github-actions]

Persistent review updated to latest commit 45bcca2

[github-actions]

Persistent review updated to latest commit 7b4abfd

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit fab62d8.

Path	Line	Severity	Description
sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/RangeQueryTranslator.java	360	medium	encodeIpAsIpv6 calls InetAddress.getByName(value) with a user-controlled string from the range query bound. InetAddress.getByName performs DNS resolution for hostnames, so a crafted query like {"range":{"ip_field":{"gte":"attacker.com"}}} triggers an outbound DNS lookup from the OpenSearch node. This enables DNS-based SSRF and covert channel exfiltration (e.g., DNS query to an attacker-controlled resolver reveals query activity). Not consistent with the stated purpose of IP address encoding; a strict IP-only parser (e.g., InetAddresses.forString or checking addr length before resolution) should be used instead.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 36c559f

[github-actions]

❌ Gradle check result for 36c559f: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 3369ce7

[github-actions]

❌ Gradle check result for 3369ce7: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 4c9c0cb

[github-actions]

Persistent review updated to latest commit 84652b1

[github-actions]

✅ Gradle check result for 84652b1: SUCCESS