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.

Sign up for GitHub

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

Fix findOne/find wrt _id column (actually all filterables) #1359

Merged
merged 5 commits into from
Aug 27, 2024
Merged

Fix findOne/find wrt _id column (actually all filterables) #1359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
633ed29
re-enable failing test as baseline
tatu-at-datastax Aug 23, 2024
b99412d
Merge branch 'main' into tatu/1357-filter-by-_id
tatu-at-datastax Aug 26, 2024
a9b0a40
mvn fmt:format
tatu-at-datastax Aug 26, 2024
5cd72cd
Fix the IT by adding resolution for `_id` filter.
tatu-at-datastax Aug 26, 2024
bf87571
Merge branch 'main' into tatu/1357-filter-by-_id
tatu-at-datastax Aug 27, 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
45 changes: 42 additions & 3 deletions src/main/java/io/stargate/sgv2/jsonapi/service/resolver/matcher/TableFilterResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import io.stargate.sgv2.jsonapi.service.operation.filters.table.NumberTableFilter;
import io.stargate.sgv2.jsonapi.service.operation.filters.table.TextTableFilter;
import io.stargate.sgv2.jsonapi.service.operation.query.DBFilterBase;
import io.stargate.sgv2.jsonapi.service.shredding.collections.DocumentId;
import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.EnumSet;
Expand All @@ -24,6 +25,7 @@
public class TableFilterResolver<CmdT extends Command & Filterable>
extends FilterResolver<CmdT, TableSchemaObject> {

private static final Object DYNAMIC_DOCID_GROUP = new Object();
private static final Object DYNAMIC_TEXT_GROUP = new Object();
private static final Object DYNAMIC_NUMBER_GROUP = new Object();

Expand Down Expand Up @@ -61,7 +63,20 @@ protected FilterMatchRules<CmdT> buildMatchRules() {
ValueComparisonOperator.GTE,
ValueComparisonOperator.LT,
ValueComparisonOperator.LTE),
JsonType.NUMBER);
JsonType.NUMBER)
// Although Tables does not have special handling for _id, our FilterClauseDeserializer
// does, so we need to capture it here.
.capture(DYNAMIC_DOCID_GROUP)
.compareValues(
"_id",
EnumSet.of(
ValueComparisonOperator.EQ,
// ValueComparisonOperator.NE, // TODO: not sure this is supported
ValueComparisonOperator.GT,
ValueComparisonOperator.GTE,
ValueComparisonOperator.LT,
ValueComparisonOperator.LTE),
JsonType.DOCUMENT_ID);

return matchRules;
}
Expand All @@ -75,20 +90,44 @@ private static List<DBFilterBase> findDynamic(CaptureExpression captureExpressio

// TODO: How do we know what the CmdT of the JsonLiteral<CmdT> from .value() is ?
for (FilterOperation<?> filterOperation : captureExpression.filterOperations()) {
final Object rhsValue = filterOperation.operand().value();
if (captureExpression.marker() == DYNAMIC_TEXT_GROUP) {
filters.add(
new TextTableFilter(
captureExpression.path(),
NativeTypeTableFilter.Operator.from(
(ValueComparisonOperator) filterOperation.operator()),
(String) filterOperation.operand().value()));
(String) rhsValue));
} else if (captureExpression.marker() == DYNAMIC_NUMBER_GROUP) {
filters.add(
new NumberTableFilter(
captureExpression.path(),
NativeTypeTableFilter.Operator.from(
(ValueComparisonOperator) filterOperation.operator()),
(BigDecimal) filterOperation.operand().value()));
(BigDecimal) rhsValue));
} else if (captureExpression.marker() == DYNAMIC_DOCID_GROUP) {
Object actualValue = ((DocumentId) rhsValue).value();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably be refactored to avoid duplicating code to create Text/NumberTableFilter.

if (actualValue instanceof String) {
filters.add(
new TextTableFilter(
captureExpression.path(),
NativeTypeTableFilter.Operator.from(
(ValueComparisonOperator) filterOperation.operator()),
(String) actualValue));
} else if (actualValue instanceof BigDecimal) {
filters.add(
new NumberTableFilter(
captureExpression.path(),
NativeTypeTableFilter.Operator.from(
(ValueComparisonOperator) filterOperation.operator()),
(BigDecimal) actualValue));
} else {
throw new UnsupportedOperationException(
"Unsupported DocumentId type: " + rhsValue.getClass().getName());
}
} else {
throw new UnsupportedOperationException(
"Unsupported dynamic filter type: " + filterOperation);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just following the pattern. We can change these to Data API exception later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry I missed this one before merging.

}
}

Expand Down
4 changes: 0 additions & 4 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/tables/FindOneTableIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import java.util.Map;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.ClassOrderer;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -186,9 +185,6 @@ public void findOneDocUpperCaseKey() {
.body("data.document", jsonEquals(DOC_B_JSON));
}

// 22-Aug-2024, tatu: Disabled until we can figure out why it fails
// (added in https://github.com/stargate/data-api/pull/1355)
@Disabled
@Test
@Order(3)
public void findOneDocIdKey() {
Expand Down