Route numeric and date queries through DV branch on pluggable-datafor… by aggarwalmayank · Pull Request #22580 · opensearch-project/OpenSearch

aggarwalmayank · 2026-07-28T05:27:13Z

Description

Pluggable-dataformat-parquet-backed indices write no BKD for numeric or date fields on the Lucene secondary. IndexOrDocValuesQuery's cost-based dispatch picks the empty point side (cost=0) and returns zero hits for range, term, terms, and bitmap queries.

This PR introduces QueryShardContext.isPluggableDataFormatEnabled() and uses it in NumberFieldType and DateFieldType to route query construction through the pure doc-values branch on pluggable-dataformat-parquet-backed indices.
Non-pluggable-dataformat indices retain their existing isSearchable() behaviour — BKD fast path preserved.

Changes

New: QueryShardContext.isPluggableDataFormatEnabled() — null-safe check that combines the feature flag (via IndexSettings.isPluggableDataFormatEnabled()) with per-index settings.
NumberFieldType.rangeQuery/termQuery/termsQuery/bitmapQuery — call effectiveSearchable(context) which returns false when the gate is on, routing through the pure DV branch.
DateFieldType.rangeQuery — same gate applied inline (termQuery delegates through it).
NumberFieldType.bitmapQuery signature extended to take QueryShardContext so it can consult the gate. Single caller in TermsQueryBuilder updated.

Test coverage

NumberFieldTypeTests — verifies point path on default indices, DV path on pluggable-dataformat-parquet-backed indices for range/term/terms.
DateFieldTypeTests — verifies DV path on pluggable-dataformat-parquet-backed indices for date range/term.
QueryShardContextTests — verifies the gate is on only when both the feature flag and index setting are on.

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.

…mat indices Composite (pluggable-dataformat) indices write no BKD for numeric or date fields on the Lucene secondary. IndexOrDocValuesQuery's cost-based dispatch picks the empty point side (cost=0) and returns zero hits for range, term, terms, and bitmap queries. Introduce QueryShardContext.isPluggableDataFormatEnabled() as a null-safe gate. NumberFieldType and DateFieldType consult this gate: on Mustang-backed indices they treat the field as not-searchable so range/term/terms/bitmap queries build the pure doc-values branch that resolves against codec-served DV columns. Non-pluggable-dataformat indices retain isSearchable() behavior unchanged, preserving the BKD fast path. Extends NumberFieldType.bitmapQuery to take a QueryShardContext so it can consult the same gate. Updates the one caller in TermsQueryBuilder and the Mockito stub in TermQueryWithDocIdAndQueryTests. Adds unit tests in NumberFieldTypeTests, DateFieldTypeTests, and QueryShardContextTests covering both gate states. Signed-off-by: Mayank Aggarwal <aggmayan@amazon.com>

github-actions · 2026-07-28T05:28:44Z

PR Reviewer Guide 🔍

(Review updated until commit `a21df7e`)

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 API Breaking Change The signature of `bitmapQuery` was changed from `bitmapQuery(BytesArray)` to `bitmapQuery(BytesArray, QueryShardContext)`. If this method is called by any plugin or external caller outside the updated `TermsQueryBuilder`, those callers will fail to compile. Consider keeping a backward-compatible overload that delegates with a null context, or confirm there are no external callers. public Query bitmapQuery(BytesArray bitmap, QueryShardContext context) { failIfNotIndexedAndNoDocValues(); return type.bitmapQuery(name(), bitmap, effectiveSearchable(context), hasDocValues()); }

github-actions · 2026-07-28T05:29:03Z

PR Code Suggestions ✨

Latest suggestions up to a21df7e

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Null-safe context check in date range The `context` parameter can be null (as demonstrated by `NumberFieldType.rangeQuery` being called with null context in some paths, and analogous DateFieldType callers). Add a null-safety check to avoid a NullPointerException when the range query is built outside a query-shard scope. server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java [546] -boolean effectiveSearchable = isSearchable() && !context.isPluggableDataFormatEnabled(); +boolean effectiveSearchable = isSearchable() && (context == null \|\| !context.isPluggableDataFormatEnabled()); Suggestion importance[1-10]: 8 __ Why: The `context` parameter can be null in `DateFieldType.rangeQuery` (as evidenced by callers passing null), and calling `context.isPluggableDataFormatEnabled()` would throw NPE. The `NumberFieldType` counterpart correctly handles null via `effectiveSearchable`, so this is a legitimate defensive fix.	Medium

Previous suggestions

Suggestions up to commit 9b38118

Category	Suggestion	Impact
Possible issue	Guard null doc-values query on gated path When `effectiveSearchable` is false because of the pluggable-dataformat gate but `hasDocValues()` is also false, `dvQuery` will be `null`, and this branch returns `null` (or wraps `null`), which will cause an NPE downstream. Also, when the gate turns the field into "not searchable", `context` is guaranteed non-null (the gate check requires it), but you should still guard the `dvQuery == null` case to fail fast or fall back appropriately. server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java [546-553] boolean effectiveSearchable = isSearchable() && !(context != null && context.isPluggableDataFormatEnabled()); // Not searchable (either declared, or gated off by the pluggable-dataformat check). Must have doc values. if (!effectiveSearchable) { + if (dvQuery == null) { + throw new IllegalArgumentException( + "Cannot search on field [" + name() + "] since it is not indexed and doesn't have doc_values." + ); + } if (context.indexSortedOnField(name())) { dvQuery = new IndexSortSortedNumericDocValuesRangeQuery(name(), l, u, dvQuery); } return nowUsed[0] ? new DateRangeIncludingNowQuery(dvQuery) : dvQuery; Suggestion importance[1-10]: 4 __ Why: The concern about `dvQuery` being null is partially valid, but `failIfNotIndexedAndNoDocValues()` is typically called earlier to prevent this state. The suggestion adds defensive validation but may be redundant with existing checks.	Low
General	Explicitly combine feature flag with index setting The test `testIsPluggableDataFormatEnabledWhenOnlyIndexSettingIsOn` expects this method to return false when the feature flag is off, even if the index setting is on. Ensure the AND-with-feature-flag logic is actually implemented in `IndexSettings.isPluggableDataFormatEnabled()`; otherwise, this method will return true and the test will fail. Consider gating the feature flag explicitly here for defense in depth. server/src/main/java/org/opensearch/index/query/QueryShardContext.java [701-703] public boolean isPluggableDataFormatEnabled() { - return indexSettings != null && indexSettings.isPluggableDataFormatEnabled(); + return indexSettings != null + && FeatureFlags.isEnabled(FeatureFlags.PLUGGABLE_DATAFORMAT_EXPERIMENTAL_FLAG) + && indexSettings.isPluggableDataFormatEnabled(); } Suggestion importance[1-10]: 3 __ Why: The suggestion is speculative — it assumes `IndexSettings.isPluggableDataFormatEnabled()` may not check the feature flag, but this is not verifiable from the diff. Adding duplicate feature flag checks may be redundant defense in depth.	Low

Suggestions up to commit 367b5f8

Category	Suggestion	Impact
Possible issue	Only downgrade to DV branch when doc values exist When the field is not searchable but has no doc values, forcing the DV branch via `effectiveSearchable=false` will produce a query that cannot execute (no BKD and no DV). Only downgrade to the DV branch when `hasDocValues()` is true; otherwise let the original searchable path (or its failure) be used. server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java [546] -boolean effectiveSearchable = isSearchable() && !(context != null && context.isPluggableDataFormatEnabled()); +boolean effectiveSearchable = isSearchable() + && !(context != null && context.isPluggableDataFormatEnabled() && hasDocValues()); Suggestion importance[1-10]: 6 __ Why: Valid concern: if a field lacks doc values on a pluggable-dataformat index, forcing the DV branch produces a null `dvQuery`. Gating on `hasDocValues()` is a reasonable safety improvement, though such configurations may be rare.	Low
Possible issue	Guard against null context before indexSortedOnField When `effectiveSearchable` is false due to a null `context`, the subsequent `context.indexSortedOnField(name())` call will throw a NullPointerException. Guard the `indexSortedOnField` call against a null context to preserve prior behavior when the context is unavailable. server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java [546-550] boolean effectiveSearchable = isSearchable() && !(context != null && context.isPluggableDataFormatEnabled()); // Not searchable (either declared, or gated off by the pluggable-dataformat check). Must have doc values. if (!effectiveSearchable) { - if (context.indexSortedOnField(name())) { + if (context != null && context.indexSortedOnField(name())) { Suggestion importance[1-10]: 3 __ Why: The original code already dereferences `context` unconditionally (e.g., `context.indexSortedOnField`), so the null-guard is not a regression introduced by this PR. Impact is minor and not directly caused by the change.	Low

github-actions · 2026-07-28T07:30:09Z

❌ Gradle check result for 367b5f8: TIMEOUT

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?

manaslohani · 2026-07-29T12:34:03Z

+     */
+    private static QueryShardContext mockPluggableDataFormatContext() {
+        QueryShardContext ctx = org.mockito.Mockito.mock(QueryShardContext.class);
+        org.mockito.Mockito.when(ctx.isPluggableDataFormatEnabled()).thenReturn(true);


import statements

manaslohani · 2026-07-29T12:39:42Z

+                    boolean effectiveSearchable = isSearchable() && !(context != null && context.isPluggableDataFormatEnabled());
+
+                    // Not searchable (either declared, or gated off by the pluggable-dataformat check). Must have doc values.
+                    if (!effectiveSearchable) {


nit: can we either remove this null check or add one on the if block below

manaslohani · 2026-07-29T13:17:27Z

+         */
+        private boolean effectiveSearchable(QueryShardContext context) {


For a parquet field is say somehow hasDocValues=false here https://github.com/aggarwalmayank/OpenSearch/blob/367b5f81a28cb5c49ea739c34ea814e6f181c4ab/server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java#L332,
we fall through to return LongPoint.newExactQuery(field, v);

Can you confirm if that is alright?

i think there BKD would silently return zero hits if it fall through that, but i don't think index creatoin with hasDocValues=false with pluggable data format index is allowed.

Address PR feedback: replace fully-qualified org.mockito.Mockito.mock/when and org.mockito.ArgumentMatchers.anyString references with static imports in NumberFieldTypeTests and DateFieldTypeTests. Signed-off-by: Mayank Aggarwal <aggmayan@amazon.com>

github-actions · 2026-07-30T08:23:28Z

Persistent review updated to latest commit 9b38118

github-actions · 2026-07-30T09:18:33Z

✅ Gradle check result for 9b38118: SUCCESS

codecov · 2026-07-30T09:18:58Z

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.42%. Comparing base (d19f68e) to head (a21df7e).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...a/org/opensearch/index/mapper/DateFieldMapper.java	50.00%	0 Missing and 1 partial ⚠️
.../org/opensearch/index/query/QueryShardContext.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22580      +/-   ##
============================================
- Coverage     71.43%   71.42%   -0.02%     
+ Complexity    76760    76723      -37     
============================================
  Files          6142     6142              
  Lines        357766   357799      +33     
  Branches      52148    52164      +16     
============================================
- Hits         255581   255563      -18     
- Misses        81861    81883      +22     
- Partials      20324    20353      +29

☔ 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.

Address PR feedback: QueryShardContext is always non-null on the query execution path, so the defensive null check is unnecessary noise. Signed-off-by: Mayank Aggarwal <aggmayan@amazon.com>

github-actions · 2026-07-30T09:26:32Z

Persistent review updated to latest commit a21df7e

github-actions · 2026-07-30T10:25:03Z

✅ Gradle check result for a21df7e: SUCCESS

nssuresh2007 · 2026-07-30T13:21:26Z

+                    boolean effectiveSearchable = isSearchable() && !context.isPluggableDataFormatEnabled();
+
+                    // Not searchable (either declared, or gated off by the pluggable-dataformat check). Must have doc values.
+                    if (!effectiveSearchable) {


We would also need to cover similarly for other data types: IP, ScaledFloat, Range types (int, float double).

I was exploring for alternatives for a single place fix. Another option is changing at ApproximateScoreQuery.canApproximate() which can help to fix for all types, but this creates a fragile assumption that either doc values query or points query is passed always to this. Hence not recommending that suggestion. But if there is any other option to avoid changes at multiple places, we can do that. Or else we can continue with current approach itself.

aggarwalmayank marked this pull request as ready for review July 28, 2026 07:38

aggarwalmayank requested review from a team, Bukhtawar, alchemist51, andrross, ashking94, cwperks, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, msfroh, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners July 28, 2026 07:38

manaslohani reviewed Jul 29, 2026

View reviewed changes

Drop redundant null check in DateFieldMapper effectiveSearchable

a21df7e

Address PR feedback: QueryShardContext is always non-null on the query execution path, so the defensive null check is unnecessary noise. Signed-off-by: Mayank Aggarwal <aggmayan@amazon.com>

nssuresh2007 reviewed Jul 30, 2026

View reviewed changes

		*/
		private boolean effectiveSearchable(QueryShardContext context) {

Uh oh!

Conversation

aggarwalmayank commented Jul 28, 2026

Description

Test coverage

Related Issues

Check List

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit a21df7e)

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

manaslohani Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

aggarwalmayank Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

manaslohani Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

aggarwalmayank Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manaslohani Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aggarwalmayank Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

codecov Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

nssuresh2007 Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jul 28, 2026 •

edited

Loading

(Review updated until commit `a21df7e`)

github-actions Bot commented Jul 28, 2026 •

edited

Loading

aggarwalmayank Jul 30, 2026 •

edited

Loading

manaslohani Jul 29, 2026 •

edited

Loading

aggarwalmayank Jul 30, 2026 •

edited

Loading

codecov Bot commented Jul 30, 2026 •

edited

Loading