fix:addresses not_in filter query for array type field - attributes.lables #83

kotharironak · 2021-02-11T11:59:11Z

Description

This PR addresses a gap for the handling of searching entities for not matching labels for array type field attributes.labels
Currently, we have support for searching entities for matching labels, but we need similar capabilities to search entities for not matching labels.

Currently, filtering entities for matching labels attributes.labels IN (<val1> , <val2>) is translated to attributes.labels = <val1> OR attributes.labels = <val2> This PR adds support for attributes.labels NOT_IN ( , )by translating it intoattributes.labels != AND attributes.labels != `

Testing

added unit tests
verified locally running MongoDB with the following data and query

Data:

db.mytest.insertOne(
   { item: "canvas", qty: 100, attributes: {labels: { valueList : {"values":[{"value":{"string":"attr-v1"}},{"value":{"string":"attr-v2"}},{"value":{"string":"attr-v3"}}]}} }}
)

db.mytest.insertOne(
   { item: "canvas", qty: 100, attributes: {labels: { valueList : {"values":[{"value":{"string":"attr-v01"}},{"value":{"string":"attr-v02"}},{"value":{"string":"attr-v03"}}]}} }}
)

db.mytest.insertOne(
   { item: "canvas", qty: 100, attributes: {labels: { valueList : {"values":[{"value":{"string":"attr-v01"}},{"value":{"string":"attr-v2"}},{"value":{"string":"attr-v4"}}]}} }}
)

db.mytest.insertOne(
   { item: "canvas", qty: 100, attributes: {labels: { valueList : {"values":[{"value":{"string":"attr-b1"}},{"value":{"string":"attr-b2"}},{"value":{"string":"attr-b4"}}]}} }}
)

Query:

db.mytest.find( { $and: [ {'attributes.labels.valueList.values': { $ne: {"value":{"string":"attr-b1"}}}}, {'attributes.labels.valueList.values': { $ne: {"value":{"string":"attr-v4"}}}}] } )

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

Documentation

None

Note

We have a ticket for adding NOT_IN filter support in docstore interface - hypertrace/document-store#32

…ables

codecov · 2021-02-11T12:00:37Z

Codecov Report

Merging #83 (3fc02c2) into main (a6b8de8) will increase coverage by 0.41%.
The diff coverage is 96.66%.

@@             Coverage Diff              @@
##               main      #83      +/-   ##
============================================
+ Coverage     57.38%   57.80%   +0.41%     
- Complexity      240      249       +9     
============================================
  Files            36       36              
  Lines          2619     2645      +26     
  Branches        352      354       +2     
============================================
+ Hits           1503     1529      +26     
  Misses          954      954              
  Partials        162      162

Flag	Coverage Δ	Complexity Δ
integration	`57.80% <96.66%> (+0.41%)`	`0.00 <4.00> (ø)`
unit	`37.96% <96.66%> (+0.63%)`	`0.00 <4.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ertrace/entity/service/util/DocStoreConverter.java	`86.86% <96.66%> (+1.98%)`	`62.00 <4.00> (+9.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6b8de8...3fc02c2. Read the comment docs.

avinashkolluru

Looks fine. But is there no integration test for this?

avinashkolluru · 2021-02-11T12:10:04Z

@kotharironak
Does this return documents that dont even have the attributes.labels field?
Thats why I feel an integration test will be helpful.

kotharironak · 2021-02-11T12:11:35Z

Looks fine. But is there no integration test for this?

Yes. there is no integration test, but we can add. Let me do that.

entity-service-impl/src/main/java/org/hypertrace/entity/service/util/DocStoreConverter.java

skjindal93 · 2021-02-11T17:46:16Z

entity-service-impl/src/main/java/org/hypertrace/entity/service/util/DocStoreConverter.java

+
+    Filter exists = new Filter();
+    exists.setFieldName(fieldName);
+    exists.setOp(Op.EXISTS);


I am not exactly sure whether we need Exists or Not Exists here. Won't we want to return all the entities which do not have this field?

Here we are doing labels NOT IN [A, B]

Do we only want to return entities which has labels but does not have A or B

Or, do we also want to return entities where labels does not exist as well?

I would think #2.

@kotharironak In that case, we need to do "NOT_EXISTS OR the other filter"

@skjindal93 Can you check this integration test - testNotInFilterForMatchingLabelsQuery? It has 5 documents. One doesn't have attributes.labels fields. Is it expected in the result?

Can you check this integration test - testNotInFilterForMatchingLabelsQuery? It has 5 documents. One doesn't have attributes.labels fields. Is it expected in the result?

Yeah, createdEntity5 is expected in the result

@skjindal93 I have added some more integration tests for NOT_IN, IN, and EQ - testDifferentFilterForMatchingLabelsQuery

Yes @kotharironak That entity is expected in the result

Made the changes. @avinashkolluru @skjindal93, now it's good to go. Have a look.

kotharironak · 2021-02-12T07:23:48Z

@kotharironak
Does this return documents that dont even have the attributes.labels field?
Thats why I feel an integration test will be helpful.

@avinashkolluru What is expected behavior here? The current implementation doesn't return. Check the test case testDifferentFilterForMatchingLabelsQuery, and it has one extra entity w/o having attributes.lables fields. Let me know the expected behavior, and accordingly, we can change.

…d in results

skjindal93 · 2021-02-12T08:34:15Z

...ce/src/integrationTest/java/org/hypertrace/entity/service/service/EntityDataServiceTest.java

+        .build();
+    entitiesList = entityDataServiceClient.query(TENANT_ID, query);
+    assertTrue(entitiesList.size() == 2);
+    assertTrue(entitiesList.contains(createdEntity2) && entitiesList.contains(createdEntity2));


I think there is a typo here

entitiesList.contains(createdEntity2) && entitiesList.contains(createdEntity2)

I think it should be

entitiesList.contains(createdEntity2) && entitiesList.contains(createdEntity5)

Done. Good catch, while debugging, I had checked createdEntity5, but had gone back & forth changing this condition.

github-actions · 2021-02-12T10:34:33Z

Unit Test Results

  27 files ±0   27 suites ±0 24s ⏱️ -2s
118 tests +3 118 ✔️ +3 0 💤 ±0 0 ❌ ±0

Results for commit c5eeedb. ± Comparison against base commit a6b8de8.

* Make the Query DTO equatable/hashable

fix:addresses not_in filter query for array type field - attributes.l…

aec7797

…ables

kotharironak requested review from avinashkolluru, buchi-busireddy and tim-mwangi as code owners February 11, 2021 11:59

This comment has been minimized.

Sign in to view

avinashkolluru previously approved these changes Feb 11, 2021

View reviewed changes

avinashkolluru requested a review from skjindal93 February 11, 2021 12:11

skjindal93 requested changes Feb 11, 2021

View reviewed changes

entity-service-impl/src/main/java/org/hypertrace/entity/service/util/DocStoreConverter.java Show resolved Hide resolved

skjindal93 reviewed Feb 11, 2021

View reviewed changes

entity-service-impl/src/main/java/org/hypertrace/entity/service/util/DocStoreConverter.java Show resolved Hide resolved

skjindal93 reviewed Feb 11, 2021

View reviewed changes

entity-service-impl/src/main/java/org/hypertrace/entity/service/util/DocStoreConverter.java Show resolved Hide resolved

kotharironak added 2 commits February 11, 2021 21:41

adds integration tests for newly added checks

d209ee0

updated doc-store version to get neq filter

ed15403

kotharironak dismissed avinashkolluru’s stale review via ed15403 February 11, 2021 17:40

kotharironak requested review from avinashkolluru and skjindal93 February 11, 2021 17:42

This comment has been minimized.

Sign in to view

skjindal93 reviewed Feb 11, 2021

View reviewed changes

Adds tests for EQ, IN and NOT_IN filter for attributes.labels

ea6bdb7

This comment has been minimized.

Sign in to view

added supports for NEQ for attributes.labels

583fb9e

This comment has been minimized.

Sign in to view

Removes exists filter as entites w/o having attributes.labels expecte…

4a0d161

…d in results

This comment has been minimized.

Sign in to view

skjindal93 reviewed Feb 12, 2021

View reviewed changes

Addressed review comments

3fc02c2

kotharironak requested a review from anandtiwary February 12, 2021 09:06

kotharironak requested a review from skjindal93 February 12, 2021 09:07

This comment has been minimized.

Sign in to view

skjindal93 approved these changes Feb 12, 2021

View reviewed changes

avinashkolluru approved these changes Feb 12, 2021

View reviewed changes

kotharironak merged commit c5eeedb into main Feb 12, 2021

kotharironak deleted the handle-not-in-clause branch February 12, 2021 10:32

kotharironak added enhancement New feature or request fix labels Feb 12, 2021

suddendust pushed a commit to suddendust/entity-service that referenced this pull request Sep 5, 2025

Make the Query DTO equatable/hashable (hypertrace#83)

e1195c4

* Make the Query DTO equatable/hashable

fix:addresses not_in filter query for array type field - attributes.lables #83

fix:addresses not_in filter query for array type field - attributes.lables #83

Uh oh!

Conversation

kotharironak commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Documentation

Note

Uh oh!

codecov bot commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

avinashkolluru left a comment

Choose a reason for hiding this comment

Uh oh!

avinashkolluru commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotharironak commented Feb 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

kotharironak commented Feb 12, 2021

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

github-actions bot commented Feb 12, 2021

Unit Test Results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kotharironak commented Feb 11, 2021 •

edited

Loading

codecov bot commented Feb 11, 2021 •

edited

Loading

avinashkolluru commented Feb 11, 2021 •

edited

Loading