Skip to content
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 #4): Postgres array filter impl. #191

Merged
merged 36 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e8f06db
Basics of array operator support
suresh-prakash Sep 25, 2023
cd439f9
ArrayFilterExpression support (Part #1)
suresh-prakash Sep 26, 2023
44e6690
Simplify Mongo execution by removing redundant filters
suresh-prakash Sep 26, 2023
a348ca8
Merge remote-tracking branch 'origin' into array_filter_support
suresh-prakash Dec 30, 2023
c6ba6cd
Split the Array Expression into 2 (for leaf and internal)
suresh-prakash Dec 30, 2023
157f26e
Comment fixes and renaming
suresh-prakash Dec 30, 2023
37058ad
Further refactoring
suresh-prakash Dec 30, 2023
0960315
Minor refactoring
suresh-prakash Dec 30, 2023
cc4347c
Refactor to take-in the wrapping LHS parser
suresh-prakash Dec 30, 2023
369daa4
Minor fixes
suresh-prakash Dec 30, 2023
7ac51bd
MongoDB implementation of array filter with integration test
suresh-prakash Dec 30, 2023
0021c9a
Functionality working for MongoDB
suresh-prakash Dec 30, 2023
c074d5c
Revert "Functionality working for MongoDB"
suresh-prakash Dec 30, 2023
ad3c865
Revert "MongoDB implementation of array filter with integration test"
suresh-prakash Dec 30, 2023
3ffa433
MongoDB implementation of array filter with integration test
suresh-prakash Dec 30, 2023
dba24d4
Functionality working for MongoDB
suresh-prakash Dec 30, 2023
6c8a7c5
Refactoring to unify the implementation
suresh-prakash Dec 30, 2023
879c87e
Refactor
suresh-prakash Dec 30, 2023
ca0c7c0
Merge branch 'array_filter_support' into array_filter_mongo_impl
suresh-prakash Dec 30, 2023
73f7125
Refactored cleanly
suresh-prakash Dec 30, 2023
7d273f5
Postgres relational filter refactoring
suresh-prakash Dec 31, 2023
d157442
Refactor
suresh-prakash Dec 31, 2023
79c44e2
Merge branch 'array_filter_mongo_impl' into postgres_filter_parser_re…
suresh-prakash Dec 31, 2023
4fabba5
Merge remote-tracking branch 'origin' into array_filter_mongo_impl
suresh-prakash Jan 7, 2024
6defee8
Merge remote-tracking branch 'origin' into postgres_filter_parser_ref…
suresh-prakash Jan 7, 2024
57a10f8
Merge branch 'array_filter_mongo_impl' into postgres_filter_parser_re…
suresh-prakash Jan 7, 2024
c374531
Partial changes
suresh-prakash Jan 7, 2024
912e0c9
Postgres Array Filter implementation
suresh-prakash Jan 7, 2024
c0e20d0
Fixed for additional test
suresh-prakash Jan 7, 2024
bfde2f4
Merge branch 'main' into postgres_filter_parser_refactoring
suresh-prakash Jan 12, 2024
19cbb1a
Addressed review comments
suresh-prakash Jan 16, 2024
9bbabbb
Merge branch 'postgres_filter_parser_refactoring' into postgres_array…
suresh-prakash Jan 16, 2024
809c758
Fixed test failures
suresh-prakash Jan 16, 2024
7187d95
Fixed code comment
suresh-prakash Jan 16, 2024
e80c2ab
Merge remote-tracking branch 'origin' into postgres_array_filter_impl
suresh-prakash Jan 16, 2024
e63860b
Update document-store/src/main/java/org/hypertrace/core/documentstore…
suresh-prakash Jan 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
import org.testcontainers.shaded.com.google.common.collect.Maps;
import org.testcontainers.utility.DockerImageName;

class ArrayFiltersQueryTest {
class ArrayFiltersQueryIntegrationTest {
private static final String COLLECTION_NAME = "galaxy";

private static Map<String, Datastore> datastoreMap;
Expand Down Expand Up @@ -123,9 +123,9 @@ private static void createCollectionData() throws IOException {
Utils.buildDocumentsFromResource("query/array_operators/galaxy.json");
datastoreMap.forEach(
(k, v) -> {
v.deleteCollection(ArrayFiltersQueryTest.COLLECTION_NAME);
v.createCollection(ArrayFiltersQueryTest.COLLECTION_NAME, null);
Collection collection = v.getCollection(ArrayFiltersQueryTest.COLLECTION_NAME);
v.deleteCollection(ArrayFiltersQueryIntegrationTest.COLLECTION_NAME);
v.createCollection(ArrayFiltersQueryIntegrationTest.COLLECTION_NAME, null);
Collection collection = v.getCollection(ArrayFiltersQueryIntegrationTest.COLLECTION_NAME);
collection.bulkUpsert(documents);
});
}
Expand All @@ -143,13 +143,15 @@ public Stream<Arguments> provideArguments(final ExtensionContext context) {
}
}

@SuppressWarnings("unused")
private static class MongoProvider implements ArgumentsProvider {
@Override
public Stream<Arguments> provideArguments(final ExtensionContext context) {
return Stream.of(Arguments.of(MONGO_STORE));
}
}

@SuppressWarnings("unused")
private static class PostgresProvider implements ArgumentsProvider {
@Override
public Stream<Arguments> provideArguments(final ExtensionContext context) {
Expand All @@ -158,17 +160,16 @@ public Stream<Arguments> provideArguments(final ExtensionContext context) {
}

@ParameterizedTest
@ArgumentsSource(MongoProvider.class)
@ArgumentsSource(AllProvider.class)
void getAllSolarSystemsWithAtLeastOnePlanetHavingBothWaterAndOxygen(final String dataStoreName)
throws IOException, JSONException {
throws JSONException {
final Collection collection = getCollection(dataStoreName);

final Query query =
Query.builder()
.setFilter(
DocumentArrayFilterExpression.builder()
.operator(ANY)
// # Can pass in some alias to this?
.arraySource(IdentifierExpression.of("additional_info.planets"))
.filter(
and(
Expand All @@ -187,7 +188,11 @@ void getAllSolarSystemsWithAtLeastOnePlanetHavingBothWaterAndOxygen(final String
IdentifierExpression.of("elements"),
RelationalOperator.EQ,
ConstantExpression.of("Water")))
.build()))
.build(),
RelationalExpression.of(
IdentifierExpression.of("meta.num_moons"),
RelationalOperator.GT,
ConstantExpression.of(0))))
.build())
.build();

Expand All @@ -209,7 +214,7 @@ private String readResource(final String fileName) {

private Collection getCollection(final String dataStoreName) {
final Datastore datastore = datastoreMap.get(dataStoreName);
return datastore.getCollection(ArrayFiltersQueryTest.COLLECTION_NAME);
return datastore.getCollection(ArrayFiltersQueryIntegrationTest.COLLECTION_NAME);
}

private String iteratorToJson(final Iterator<Document> iterator) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,21 @@
"Oxygen",
"Water",
"Nitrogen"
]
],
"meta": {
"num_moons": 7
}
},
{
"name": "Planet 2",
"elements": [
"Oxygen",
"Helium",
"Water"
]
],
"meta": {
"num_moons": 1
}
}
]
}
Expand Down Expand Up @@ -46,7 +52,10 @@
"Oxygen",
"Nitrogen",
"Water"
]
],
"meta": {
"num_moons": 1
}
},
{
"name": "Mars",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,21 @@
"Oxygen",
"Water",
"Nitrogen"
]
],
"meta": {
"num_moons": 7
}
},
{
"name": "Planet 2",
"elements": [
"Oxygen",
"Helium",
"Water"
]
],
"meta": {
"num_moons": 1
}
}
]
}
Expand Down Expand Up @@ -61,7 +67,10 @@
"Hydrogen",
"Helium",
"Methane"
]
],
"meta": {
"num_moons": 10
}
},
{
"name": "Planet 8",
Expand Down Expand Up @@ -100,7 +109,10 @@
"Oxygen",
"Nitrogen",
"Water"
]
],
"meta": {
"num_moons": 1
}
},
{
"name": "Mars",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package org.hypertrace.core.documentstore.postgres.query.v1.vistors;

import lombok.AllArgsConstructor;
import org.hypertrace.core.documentstore.expression.type.SelectTypeExpression;
import org.hypertrace.core.documentstore.postgres.query.v1.PostgresQueryParser;

@AllArgsConstructor
public class PostgresArrayRelationalWrappingFilterVisitorProvider
implements PostgresWrappingFilterVisitorProvider {

private PostgresQueryParser postgresQueryParser;
private String arraySource;
private String alias;

@Override
public PostgresSelectTypeExpressionVisitor getForRelational(
final PostgresSelectTypeExpressionVisitor baseVisitor, final SelectTypeExpression rhs) {
// Override the base visitor,
// pick the LHS field name (elements.inner),
// replace it with alias (inner), and
// optionally trim double quotes (TRIM('"' FROM inner::text))
return new PostgresIdentifierTrimmingExpressionVisitor(
new PostgresIdentifierReplacingExpressionVisitor(
new PostgresIdentifierExpressionVisitor(postgresQueryParser), arraySource, alias),
rhs);
}

@Override
public PostgresSelectTypeExpressionVisitor getForNonRelational(
PostgresSelectTypeExpressionVisitor baseVisitor) {
return getForRelational(baseVisitor, null);

Check warning on line 31 in document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresArrayRelationalWrappingFilterVisitorProvider.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresArrayRelationalWrappingFilterVisitorProvider.java#L31

Added line #L31 was not covered by tests
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package org.hypertrace.core.documentstore.postgres.query.v1.vistors;

import lombok.AllArgsConstructor;
import org.hypertrace.core.documentstore.expression.type.SelectTypeExpression;
import org.hypertrace.core.documentstore.postgres.query.v1.PostgresQueryParser;

@AllArgsConstructor
public class PostgresDocumentArrayWrappingFilterVisitorProvider
implements PostgresWrappingFilterVisitorProvider {

private PostgresQueryParser postgresQueryParser;
private String alias;

@Override
public PostgresSelectTypeExpressionVisitor getForRelational(
final PostgresSelectTypeExpressionVisitor baseVisitor, final SelectTypeExpression rhs) {
// Override the existing parser,
// parse the field name (meta.num_moons),
// get data accessor expression prefixed with alias (planets->'meta'->>'num_moons'), and
// cast according to the value type (CAST (planets->'meta'->>'num_moons' AS NUMERIC) > ?)
return new PostgresRelationalIdentifierAccessingExpressionVisitor(
new PostgresIdentifierExpressionVisitor(postgresQueryParser), alias, rhs);
}

@Override
public PostgresSelectTypeExpressionVisitor getForNonRelational(
final PostgresSelectTypeExpressionVisitor baseVisitor) {
// Any LHS field name (elements) is to be prefixed with current alias (inner)
return new PostgresIdentifierAccessingExpressionVisitor(baseVisitor, alias);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
import static org.hypertrace.core.documentstore.expression.operators.RelationalOperator.EQ;
import static org.hypertrace.core.documentstore.expression.operators.RelationalOperator.IN;
import static org.hypertrace.core.documentstore.postgres.PostgresCollection.ID;
import static org.hypertrace.core.documentstore.postgres.utils.PostgresUtils.encodeAliasForNestedField;
import static org.hypertrace.core.documentstore.postgres.utils.PostgresUtils.prepareParsedNonCompositeFilter;

import java.util.Optional;
import java.util.stream.Collector;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
import org.hypertrace.core.documentstore.Key;
import org.hypertrace.core.documentstore.expression.impl.ArrayRelationalFilterExpression;
Expand All @@ -27,9 +29,17 @@

public class PostgresFilterTypeExpressionVisitor implements FilterTypeExpressionVisitor {
protected PostgresQueryParser postgresQueryParser;
@Nullable private final PostgresWrappingFilterVisitorProvider wrappingVisitorProvider;

public PostgresFilterTypeExpressionVisitor(PostgresQueryParser postgresQueryParser) {
this(postgresQueryParser, null);
}

public PostgresFilterTypeExpressionVisitor(
PostgresQueryParser postgresQueryParser,
final PostgresWrappingFilterVisitorProvider wrappingVisitorProvider) {
this.postgresQueryParser = postgresQueryParser;
this.wrappingVisitorProvider = wrappingVisitorProvider;
}

@SuppressWarnings("unchecked")
Expand All @@ -51,7 +61,11 @@
public String visit(final RelationalExpression expression) {
final PostgresSelectExpressionParserBuilder parserBuilder =
new PostgresSelectExpressionParserBuilderImpl(postgresQueryParser);
final PostgresSelectTypeExpressionVisitor lhsVisitor = parserBuilder.build(expression);
final PostgresSelectTypeExpressionVisitor baseVisitor = parserBuilder.build(expression);
final PostgresSelectTypeExpressionVisitor lhsVisitor =
Optional.ofNullable(wrappingVisitorProvider)
.map(visitor -> visitor.getForRelational(baseVisitor, expression.getRhs()))
.orElse(baseVisitor);

final PostgresRelationalFilterContext context =
PostgresRelationalFilterContext.builder()
Expand Down Expand Up @@ -80,16 +94,44 @@
postgresQueryParser.getParamsBuilder());
}

@SuppressWarnings("unchecked")
@SuppressWarnings({"unchecked", "SwitchStatementWithTooFewBranches"})
@Override
public String visit(final ArrayRelationalFilterExpression expression) {
throw new UnsupportedOperationException();
/*
EXISTS
(SELECT 1
FROM jsonb_array_elements(COALESCE(planets->'elements', '[]'::jsonb)) AS elements
WHERE TRIM('"' FROM elements::text) = 'Oxygen'
)
*/
switch (expression.getOperator()) {
case ANY:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

return getFilterStringForAnyOperator(expression);

default:
throw new UnsupportedOperationException(
"Unsupported array operator: " + expression.getOperator());

Check warning on line 113 in document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresFilterTypeExpressionVisitor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresFilterTypeExpressionVisitor.java#L112-L113

Added lines #L112 - L113 were not covered by tests
}
}

@SuppressWarnings("unchecked")
@SuppressWarnings({"unchecked", "SwitchStatementWithTooFewBranches"})
@Override
public String visit(final DocumentArrayFilterExpression expression) {
throw new UnsupportedOperationException();
/*
EXISTS
(SELECT 1
FROM jsonb_array_elements(COALESCE(document->'planets', '[]'::jsonb)) AS planets
WHERE <parsed_containing_filter_with_aliased_field_names>
)
*/
switch (expression.getOperator()) {
case ANY:
return getFilterStringForAnyOperator(expression);

default:
throw new UnsupportedOperationException(
"Unsupported array operator: " + expression.getOperator());

Check warning on line 133 in document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresFilterTypeExpressionVisitor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresFilterTypeExpressionVisitor.java#L132-L133

Added lines #L132 - L133 were not covered by tests
}
}

public static Optional<String> getFilterClause(PostgresQueryParser postgresQueryParser) {
Expand All @@ -113,4 +155,75 @@
throw new UnsupportedOperationException(
String.format("Query operation:%s not supported", operator));
}

private String getFilterStringForAnyOperator(final ArrayRelationalFilterExpression expression) {
// Convert 'elements' to planets->'elements' where planets could be an alias for an upper
// level array filter
// For the first time (if 'elements' was not under any nested array, say a top-level field),
// use the field identifier visitor to make it document->'elements'
final PostgresIdentifierExpressionVisitor identifierVisitor =
new PostgresIdentifierExpressionVisitor(postgresQueryParser);
final PostgresSelectTypeExpressionVisitor arrayPathVisitor =
wrappingVisitorProvider == null
? new PostgresFieldIdentifierExpressionVisitor(identifierVisitor)

Check warning on line 168 in document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresFilterTypeExpressionVisitor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/vistors/PostgresFilterTypeExpressionVisitor.java#L168

Added line #L168 was not covered by tests
: wrappingVisitorProvider.getForNonRelational(identifierVisitor);
final String parsedLhs = expression.getArraySource().accept(arrayPathVisitor);

// Extract the field name
final String identifierName =
expression
.getArraySource()
.accept(new PostgresIdentifierExpressionVisitor(postgresQueryParser));

// If the field name is 'elements.inner', alias becomes 'elements_dot_inner'
final String alias = encodeAliasForNestedField(identifierName);

// Any LHS field name (elements) is to be prefixed with current alias (elements_dot_inner)
final PostgresWrappingFilterVisitorProvider visitorProvider =
new PostgresArrayRelationalWrappingFilterVisitorProvider(
postgresQueryParser, identifierName, alias);
final String parsedFilter =
expression
.getFilter()
.accept(new PostgresFilterTypeExpressionVisitor(postgresQueryParser, visitorProvider));

return String.format(
"EXISTS (SELECT 1 FROM jsonb_array_elements(COALESCE(%s, '[]'::jsonb)) AS \"%s\" WHERE %s)",
parsedLhs, alias, parsedFilter);
}

private String getFilterStringForAnyOperator(final DocumentArrayFilterExpression expression) {
// Convert 'elements' to planets->'elements' where planets could be an alias for an upper
// level array filter
// For the first time (if 'elements' was not under any nested array, say a top-level field),
// use the field identifier visitor to make it document->'elements'
final PostgresIdentifierExpressionVisitor identifierVisitor =
new PostgresIdentifierExpressionVisitor(postgresQueryParser);
final PostgresSelectTypeExpressionVisitor arrayPathVisitor =
wrappingVisitorProvider == null
? new PostgresFieldIdentifierExpressionVisitor(identifierVisitor)
: wrappingVisitorProvider.getForNonRelational(identifierVisitor);
final String parsedLhs = expression.getArraySource().accept(arrayPathVisitor);

// Extract the field name
final String identifierName =
expression
.getArraySource()
.accept(new PostgresIdentifierExpressionVisitor(postgresQueryParser));

// If the field name is 'elements.inner', alias becomes 'elements_dot_inner'
final String alias = encodeAliasForNestedField(identifierName);

// Any LHS field name (elements) is to be prefixed with current alias (elements_dot_inner)
final PostgresWrappingFilterVisitorProvider wrapper =
new PostgresDocumentArrayWrappingFilterVisitorProvider(postgresQueryParser, alias);
final String parsedFilter =
expression
.getFilter()
.accept(new PostgresFilterTypeExpressionVisitor(postgresQueryParser, wrapper));

return String.format(
"EXISTS (SELECT 1 FROM jsonb_array_elements(COALESCE(%s, '[]'::jsonb)) AS \"%s\" WHERE %s)",
parsedLhs, alias, parsedFilter);
}
}
Loading