Array filtering support (Part #4): Postgres array filter impl. #191
Conversation
suresh-prakash
requested review from
kotharironak and
skjindal93
as code owners
suresh-prakash
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
=========================================
Coverage 79.57% 79.58%
- Complexity 956 957 +1
=========================================
Files 182 188 +6
Lines 4539 4623 +84
Branches 373 379 +6
=========================================
+ Hits 3612 3679 +67
- Misses 669 678 +9
- Partials 258 266 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Test Results 38 files ±0 38 suites ±0 32s ⏱️ ±0s Results for commit e63860b. ± Comparison against base commit 88a0440. This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
|
||
| @Override | ||
| public PostgresQueryParser getPostgresQueryParser() { | ||
| return baseVisitor.getPostgresQueryParser(); |
There was a problem hiding this comment.
Curious: In the PostgresIdentifierReplacingExpressionVisitor class, we initially verify the existence of the postgresQueryParser. Afterward, we reference the baseVisitor as follows:
postgresQueryParser != null ? postgresQueryParser : baseVisitor.getPostgresQueryParser()Why aren't we doing the same here and directly accessing it from the baseVisitor?
There was a problem hiding this comment.
Actually, that is also not required as we are not passing in any baseVisitor in the constructor. Updated there to get rid of the unnecessary conditional statement.
|
|
||
| @Override | ||
| public PostgresQueryParser getPostgresQueryParser() { | ||
| return baseVisitor.getPostgresQueryParser(); |
There was a problem hiding this comment.
Same question as above: #191 (comment)
There was a problem hiding this comment.
Actually, that is also not required as we are not passing in any baseVisitor in the constructor. Updated there to get rid of the unnecessary conditional statement.
| // Convert 'elements' to planets->'elements' where planets could be an alias for an upper | ||
| // level array filter | ||
| // Also, for the first time (if this was not under any nesting), use the field identifier | ||
| // visitor to make it document->'elements' |
There was a problem hiding this comment.
is this document->'elements' correct? should it be `document->'planets'?
There was a problem hiding this comment.
document->'elements' is correct. I've updated the code comments to be more clear.
| .getArraySource() | ||
| .accept(new PostgresIdentifierExpressionVisitor(postgresQueryParser)); | ||
|
|
||
| // If the field name is 'elements.inner', just pick the last part as the alias ('inner') |
There was a problem hiding this comment.
Could this be an issue if there are two array expressions, both having the same last part, such as inner in planets.inner and elements.inner?
If so, creating an alias with the full path, like elements_inner, might be a helpful solution.
There was a problem hiding this comment.
Actually, the alias is used within a smaller scope, so it shouldn't be an issue. But, yeah, it still makes sense to update it for clarity. Will do.
| ) | ||
| */ | ||
| switch (expression.getOperator()) { | ||
| case ANY: |
There was a problem hiding this comment.
If there are more operators, we should move the code for each operator to its dedicated function for readability? For instance, moving code of any operator to getFilterExpressionForAnyOperator() here.
...ment-store/src/main/java/org/hypertrace/core/documentstore/postgres/utils/PostgresUtils.java
Outdated
Show resolved
Hide resolved
…/postgres/utils/PostgresUtils.java
So far, we do not have support for filtering by LHS arrays.
The PRs in this series aim to support LHS arrays in filters.
An LHS array can contain either primitives or documents.
In the case of a primitive LHS array, we can apply a relational operator on each element.
In the case of a document LHS array, we can apply a generic filter (including relational expression or logical expression or even another array filter expression)
Sample data:
[ { "key": 3, "planets": [ { "name": "Planet 1", "elements": [ "Oxygen", "Water", "Nitrogen" ] }, { "name": "Planet 2", "elements": [ "Oxygen", "Helium", "Water" ] } ] }, { "key": 1, "planets": [ { "name": "Mercury" }, { "name": "Venus" }, { "name": "Earth", "elements": [ "Oxygen", "Nitrogen" ] }, { "name": "Mars", "elements": [ "Iron" ] }, { "name": "Jupiter", "elements": [] }, { "name": "Saturn", "elements": [] }, { "name": "Uranus", "elements": [ "Hydrogen", "Helium", "Methane" ] }, { "name": "Neptune", "elements": [ "Hydrogen", "Helium", "Methane", "Ammonia" ] } ] }, { "key": 2, "planets": [ { "name": "Mercury", "elements": [ "Silicate", "Aluminum" ] }, { "name": "Venus", "elements": [ "Carbon Dioxide", "Sulfuric Acid" ] }, { "name": "Earth", "elements": [ "Oxygen", "Nitrogen", "Water" ] }, { "name": "Mars", "elements": [ "Iron", "Silicate" ] }, { "name": "Jupiter", "elements": [ "Hydrogen", "Helium" ] }, { "name": "Saturn", "elements": [ "Hydrogen", "Helium", "Methane" ] }, { "name": "Uranus", "elements": [ "Hydrogen", "Helium", "Methane", "Ammonia" ] }, { "name": "Neptune", "elements": [ "Hydrogen", "Helium", "Methane", "Ammonia", "Methane Hydrate" ] } ] } ]Input query:
Get a list of solar systems with at least one planet not having either oxygen or water.The filter API would look like
Sample MongoDB match stage query to be generated
{ "$expr": { "$anyElementTrue": { "$map": { "input": { "$ifNull": [ "$planets", [] ] }, "as": "planet", "in": { "$not": { "$or": [ { "$anyElementTrue": { "$map": { "input": { "$ifNull": [ "$$planet.elements", [] ] }, "as": "element", "in": { "$eq": ["$$element","Oxygen"] } } } }, { "$anyElementTrue": { "$map": { "input": { "$ifNull": [ "$$planet.elements", [] ] }, "as": "element", "in": { "$eq": ["$$element","Water"] } } } } ] } } } } } }Sample Postgres Query:
Notes:
ANYoperator is supported for nowNOTlogical operatorALLcan be achieved by usingNOTtwice to theANYfilter (before and after). E.g.:Fetch solar systems with all planets containing both oxygen and wateris equivalent toFetch solar systems with no planet without both oxygen and waterNONEcan be achieved by usingNOTonce beforeANY, becauseNONE = NOT ANYINinstead ofORand=, static importnotandorlogical operators, etc.)This PR contains
Previous PRs
#173
#188
#189