Extensible design to add new query and field type support for Star Tree #17100

expani · 2025-01-23T09:02:11Z

Description

This PR introduces constructs to help convert user queries into the equivalent star tree traversal by generating the appropriate DimensionFilter based on the type of query.

DimensionFilterMapper acts as an equivalent mapper for Star Tree supported fields and queries.

StarTreeFilterProvider is responsible for converting the user queries ( represented by QueryBuilder )

I have changed all the older tests to point towards the new classes to ensure that we get coverage for at least numeric term queries.

We have tested term, terms and range queries on numeric and keyword over http_logs for sanity of the code and fixed some tricky bugs on different cases. Thanks to @bharath-techie for help with creating end to end query test with randomness.

I will be adding Unit tests for Range and other new classes that are not covered by existing tests in the coming days.

Most of the TODO left around the code will be picked up in future when supporting Filter Aggregations and Boolean queries. If that's not the case, please comment on the same.

Related Issues

#16537
#16538
#16539

github-actions · 2025-01-23T09:12:46Z

❌ Gradle check result for b85183d: 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?

bharath-techie · 2025-01-24T01:49:03Z

...nsearch/index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNode.java

            low++;
        }

+        if (lastMatchedNode instanceof FixedLengthStarTreeNode) {


should we move this up ?

bharath-techie · 2025-01-24T01:50:04Z

...nsearch/index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNode.java

+                FixedLengthStarTreeNode highStarTreeNode = binarySearchChild(high, false, lowStarTreeNode);
+                if (highStarTreeNode != null) {
+                    for (int lowNodeId = lowStarTreeNode.nodeId(); lowNodeId <= highStarTreeNode.nodeId(); ++lowNodeId) {
+                        collector.collectStarNode(new FixedLengthStarTreeNode(in, lowNodeId));


collectStarNode is confusing since we have special star nodes as well - can it simply be collectNode or collect since collector is starTreeNodeCollector

Oops thats a typo.
I will change StarTreeNodeCollector

bharath-techie · 2025-01-24T01:59:23Z

...nsearch/index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNode.java

+        }
+    }
+
+    private FixedLengthStarTreeNode binarySearchChild(long dimensionValue, boolean matchNextHighest, StarTreeNode lastMatchedNode)


Subjective but can't we rather name matchNextHighest to be match/queryLower or something similar. inverse of MatchNextHighest doesn't tell us that we are looking for match next lowest

Good catch. Incidentally, @msfroh had asked the same.
I will add the inverse statement while adding Java docs or let me know if there are any variable names that captures the sentiment better.

...nsearch/index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNode.java

bharath-techie · 2025-01-24T02:12:43Z

server/src/main/java/org/opensearch/search/startree/StarTreeQueryContext.java

    }

-    public Map<String, Long> getQueryMap() {
-        return queryMap;
+    public FixedBitSet getStarTreeValue(LeafReaderContext ctx) {


nit : can we make the naming consistent :

Technically its getStarTreeValues and getStarTreeValuesArr - since starTreeValues is the name of the variable - we can also rename to be something better like getCachedStarTreeValues/Arr as well

bharath-techie · 2025-01-24T02:34:39Z

server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java

+            FLOAT,
+            new FloatFieldMapperNumeric(),
+            DOUBLE,
+            new DoubleFieldMapperNumeric()


Lets add todo for unsigned long.

Also how does scaled float get handled here ? scaled float is a supported dimension which is present as part of modules

Ack will add a TODO for unsigned long.

Scaled Float along with other mapped field types ( that can be registered from plugins by extending MapperPlugin ) will have this problem since they are not present in OS Core.

So, we will have to expose methods via the existing MappedFieldType which will overridden by the custom mapped types of plugin.

Given this might be a significant change, I think we should pick it up separately rather than accommodating within this PR post 2.19

bharath-techie · 2025-01-24T02:39:03Z

server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java

+        return null;
+    }
+
+    abstract long convertToDocValues(Number parsedValue);


nit : can we rename this to something better - convertToIndexedValue or something similar

bharath-techie · 2025-01-24T02:42:50Z

server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java

+    private Long lowOrdinal;
+    private Long highOrdinal;
+
+    private boolean skipRangeCollection = false;


Can we add comments on what this does?

Also java docs we will need to add for all new classes - which you already might be doing

skipCollectionForOutOfBounds maybe ?

server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java

server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java

...nsearch/index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNode.java

github-actions · 2025-01-24T04:35:42Z

❌ Gradle check result for 973edf5: 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 · 2025-01-24T23:46:00Z

❌ Gradle check result for 5e7a711: 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?

…t#17098) * change version to 2.19 Signed-off-by: Junwei Dai <[email protected]> * bring back the constructor InternalSearchResponse and SearchResponseSections Signed-off-by: Junwei Dai <[email protected]> --------- Signed-off-by: Junwei Dai <[email protected]> Co-authored-by: Junwei Dai <[email protected]> Signed-off-by: expani <[email protected]>

Introduce template query that holds the content of query which can contain placeholders and can be filled by the variables from PipelineProcessingContext produced by search processors. This allows query rewrite by the search processors. --------- Signed-off-by: Mingshi Liu <[email protected]> Co-authored-by: Michael Froh <[email protected]> Signed-off-by: expani <[email protected]>

…tor (opensearch-project#17080) Signed-off-by: Ganesh Ramadurai <[email protected]> Co-authored-by: Ganesh Ramadurai <[email protected]> Signed-off-by: expani <[email protected]>

--------- Signed-off-by: panguixin <[email protected]> Signed-off-by: expani <[email protected]>

…g settings on restore snapshot (opensearch-project#16957) * Add index.knn setting to list of unmodifiable settings when restore snapshot Signed-off-by: AnnTian Shao <[email protected]> * Add index.knn setting to list of unmodifiable settings when restore snapshot Signed-off-by: AnnTian Shao <[email protected]> * Add new Setting property UnmodifiableOnRestore to mark setting as immutable on restore snapshot Signed-off-by: AnnTian Shao <[email protected]> * Add tests for new Setting property UnmodifiableOnRestore Signed-off-by: AnnTian Shao <[email protected]> * fixes and added more tests for new setting property UnmodifiableOnRestore Signed-off-by: AnnTian Shao <[email protected]> * fix test failures Signed-off-by: AnnTian Shao <[email protected]> * Revert "fix test failures" This reverts commit 252100c. Signed-off-by: AnnTian Shao <[email protected]> * fixes by adding back USER_UNMODIFIABLE_SETTINGS for settings without Setting implementation Signed-off-by: AnnTian Shao <[email protected]> * testing CI config for registering plugin settings Signed-off-by: AnnTian Shao <[email protected]> * Revert "testing CI config for registering plugin settings" This reverts commit 9ebab5a. Signed-off-by: AnnTian Shao <[email protected]> * Add UnmodifiableOnRestore only to unmodifiable settings that are already registered, will raise separate PR for settings not yet registered. Add validation check in Setting.java. Add UnmodifiableOnRestore settings cannot be removed on restore Signed-off-by: AnnTian Shao <[email protected]> * fixes and move tests to RestoreSnapshotIT Signed-off-by: AnnTian Shao <[email protected]> * Add back testInvalidRestoreRequestScenarios test method Signed-off-by: AnnTian Shao <[email protected]> --------- Signed-off-by: AnnTian Shao <[email protected]> Signed-off-by: Tommy Shao <[email protected]> Co-authored-by: AnnTian Shao <[email protected]> Signed-off-by: expani <[email protected]>

Signed-off-by: Andrew Ross <[email protected]> Signed-off-by: expani <[email protected]>

Move AcknowledgedResponse, AcknowledgedRequest, AcknowledgedRequestBuilder,and ShardsAcknowledgedResponse to "clustermanager" Java package. This is a purely structural move done via the IDE with no logic changes. Signed-off-by: Andrew Ross <[email protected]> Signed-off-by: expani <[email protected]>

…-project#17005) Prior to this fix, the _msearch API would keep running search requests even after being canceled. With this change, we explicitly check if the task has been canceled before kicking off subsequent requests. --------- Signed-off-by: Michael Froh <[email protected]> Signed-off-by: expani <[email protected]>

Signed-off-by: Prudhvi Godithi <[email protected]> Signed-off-by: expani <[email protected]>

Signed-off-by: expani <[email protected]>

…ore tests Signed-off-by: expani <[email protected]>

Signed-off-by: expani <[email protected]>

github-actions · 2025-01-27T17:32:48Z

❌ Gradle check result for 629866d: 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?

expani · 2025-01-27T17:46:03Z

Closing this as the replacement with clean branch is created
#17137

bharath-techie suggested changes Jan 24, 2025

View reviewed changes

bharath-techie reviewed Jan 24, 2025

View reviewed changes

...nsearch/index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNode.java Outdated Show resolved Hide resolved

opensearch-ci-bot mentioned this pull request Jan 24, 2025

[AUTOCUT] Gradle Check Flaky Test Report for IndexingIT #14302

Open

junweid62 and others added 23 commits January 27, 2025 08:48

Propagate includes and excludes from fetchSourceContext to FieldsVisi…

6360c75

…tor (opensearch-project#17080) Signed-off-by: Ganesh Ramadurai <[email protected]> Co-authored-by: Ganesh Ramadurai <[email protected]> Signed-off-by: expani <[email protected]>

Fix exists query for flat object (opensearch-project#17108)

32f8bc3

--------- Signed-off-by: panguixin <[email protected]> Signed-off-by: expani <[email protected]>

Fix Binlong's name in MAINTAINERS.md (opensearch-project#17113)

f85f69a

Signed-off-by: Andrew Ross <[email protected]> Signed-off-by: expani <[email protected]>

Refactor bootstrap for JPMS support (opensearch-project#17117)

15e622f

Signed-off-by: Prudhvi Godithi <[email protected]> Signed-off-by: expani <[email protected]>

Added support new field types and query types in metric aggregator tests

cff7284

Signed-off-by: expani <[email protected]>

Added test support for decimal fields

2022439

Signed-off-by: expani <[email protected]>

Spotless and removed fixed TODOs

df406da

Signed-off-by: expani <[email protected]>

Trying with reducing iterations to prevent OOM

9f9a3e6

Signed-off-by: expani <[email protected]>

Added support to generate star tree field config by method params

e7df311

Signed-off-by: expani <[email protected]>

Removed excessive instanceOf checks and added initials FLSTN test

87f3988

Signed-off-by: expani <[email protected]>

Added more cases to make binarySearch for term/terms 100% coverage

e2f6dbc

Signed-off-by: expani <[email protected]>

Added test for startreenode range match

6c99381

Signed-off-by: expani <[email protected]>

Added random test for binary exact and range match

a92f0bd

Signed-off-by: expani <[email protected]>

Added Java docs and unit tests for ordinalLookup in Keyword field

aa48350

Signed-off-by: expani <[email protected]>

Formatting

4fcde4b

Signed-off-by: expani <[email protected]>

Using Set instead of List for StarTree and Dimension Filters, Added m…

228cffa

…ore tests Signed-off-by: expani <[email protected]>

Added package info files

ff5c9d6

Signed-off-by: expani <[email protected]>

Removed equals and hashCode from filters and changed back to using List

629866d

Signed-off-by: expani <[email protected]>

expani force-pushed the startree_query_field_support branch from 97a21b1 to 629866d Compare January 27, 2025 16:48

expani requested review from gaobinlong and peternied as code owners January 27, 2025 16:48

expani mentioned this pull request Jan 27, 2025

Extensible design to add new query and field type support for Star Tree #17137

Merged

expani closed this Jan 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible design to add new query and field type support for Star Tree #17100

Extensible design to add new query and field type support for Star Tree #17100

expani commented Jan 23, 2025

github-actions bot commented Jan 23, 2025

bharath-techie Jan 24, 2025

bharath-techie Jan 24, 2025

expani Jan 24, 2025

bharath-techie Jan 24, 2025

expani Jan 24, 2025 •

edited

Loading

bharath-techie Jan 24, 2025

bharath-techie Jan 24, 2025

expani Jan 25, 2025 •

edited

Loading

bharath-techie Jan 24, 2025

bharath-techie Jan 24, 2025

bharath-techie Jan 24, 2025

github-actions bot commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

github-actions bot commented Jan 27, 2025

expani commented Jan 27, 2025

Extensible design to add new query and field type support for Star Tree #17100

Extensible design to add new query and field type support for Star Tree #17100

Conversation

expani commented Jan 23, 2025

Description

Related Issues

github-actions bot commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

expani Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

expani Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

github-actions bot commented Jan 27, 2025

expani commented Jan 27, 2025

expani Jan 24, 2025 •

edited

Loading

expani Jan 25, 2025 •

edited

Loading