-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Array filtering support (Part #3): Postgres relational filter refactoring #189
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
============================================
- Coverage 80.48% 79.57% -0.91%
- Complexity 937 956 +19
============================================
Files 169 182 +13
Lines 4438 4539 +101
Branches 368 373 +5
============================================
+ Hits 3572 3612 +40
- Misses 615 669 +54
- Partials 251 258 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
import org.hypertrace.core.documentstore.postgres.query.v1.vistors.PostgresSelectTypeExpressionVisitor; | ||
|
||
public interface PostgresSelectExpressionParserBuilder { | ||
PostgresSelectTypeExpressionVisitor buildFor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Change
buildFor
tobuild
(Is there a specific reason for choosingbuildFor
)? -
Is it necessary for
PostgresQueryParser
to be included as part of the interface? Could it instead be a member of the implemented builder object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Sure. (No specific reason)
- Yes, can be. Will update.
|
||
@Override | ||
public PostgresRelationalFilterParser parser( | ||
final RelationalExpression expression, final PostgresRelationalFilterContext context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not see the usage of PostgresRelationalFilterContext
. Is it necessary to include it as part of the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required. Will update.
|
||
public interface PostgresRelationalFilterParserFactory { | ||
PostgresRelationalFilterParser parser( | ||
final RelationalExpression expression, final PostgresRelationalFilterContext context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if context
is not required, we can remove it from an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, yeah.
import org.hypertrace.core.documentstore.postgres.query.v1.vistors.PostgresSelectTypeExpressionVisitor; | ||
|
||
public interface PostgresRelationalFilterParser { | ||
String parse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will every parse request have a different context? If not, shall we consider moving it as part of the Parser constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will every parse request have a different context?
Yes. That's the idea in the continuation PR @ #191
value -> { | ||
paramsBuilder.addObjectParam(value).addObjectParam(value); | ||
return String.format( | ||
"((jsonb_typeof(to_jsonb(%s)) = 'array' AND to_jsonb(%s) @> jsonb_build_array(?)) OR (jsonb_build_array(%s) @> jsonb_build_array(?)))", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- What did the previous
IN
request look like? - Won't this result in N OR requests for
N
RHS values? For each of them, it will apply thejson_build_array
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, the definition of IN
is very loose when the LHS is an array.
MongoDB treats, IN
as a non-empty intersection between the LHS array and the RHS array. The Postgres behaviour was inconsistent with this definition breaking the parity. This PR https://github.com/hypertrace/document-store/pull/186/files addressed that by considering non-empty intersection between the LHS array and the RHS array for IN
for Postgres also. So, in this refactoring PR, I didn't change the behavior.
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:
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
Sample Postgres Query:
Notes:
ANY
operator is supported for nowNOT
logical operatorALL
can be achieved by usingNOT
twice to theANY
filter (before and after). E.g.:Fetch solar systems with all planets containing both oxygen and water
is equivalent toFetch solar systems with no planet without both oxygen and water
NONE
can be achieved by usingNOT
once beforeANY
, becauseNONE = NOT ANY
IN
instead ofOR
and=
, static importnot
andor
logical operators, etc.)This PR contains
Previous PRs
#173
#188