PoC for Projection implementation for API Tables #1315
Conversation
Encapsulates behavior for clauses that can be validated and better supports schema object types.
This PR refactors how we process filters by moving the base class FilterableResolver that was used by command resolvers to be standalone class FilterResolver that has CollectionFilterResolver and TableFilterResolver subclasses. The changes to the resolvers are mostly to handle this change, other than findOne. The next part is findOne updated to process the filter for a table. Using the TableFilterResolver to make filters, which then use the NativeTypeTableFilter and JSONCodec to map from the types used for JSOn into what CQL expects. This is POC work, we then need to expand this to handle all the situations we expect.
POC to show pushing the projection down how we build the select and how we build the document from the row. see OperationProjection
Tis commit changes the InsertTableOperation to use the same JSONCodec created to process data for a filter to transform the data from the incoming document into what the driver wants to write to the CQL column
Expands the JSONCode to support mapping toJSON from the objects the driver sent, to be used in the projection
.../java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodecRegistry.java
Outdated
Show resolved
Hide resolved
| throws UnknownColumnException, MissingJSONCodecException { | ||
|
|
||
| Preconditions.checkNotNull(table, "table must not be null"); | ||
| Preconditions.checkNotNull(column, "column must not be null"); | ||
| Preconditions.checkNotNull(columnId, "column must not be null"); |
There was a problem hiding this comment.
Shouldn't these be handled in the resolvers? These throws unhandled errors.
There was a problem hiding this comment.
Depends on how we view this: if there is no way to trigger it from outside (which I think is the case), these are assertions for internal state and should be ok this way.
But if there was a way to get null here from request it should throw JsonApiException.
src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/TableRowProjection.java
Show resolved
Hide resolved
|
|
||
| List<ColumnMetadata> columns = projectionDefinition.extractSelectedColumns(columnsByName); | ||
|
|
||
| if (columns.isEmpty()) { |
There was a problem hiding this comment.
A table can't be with empty columns. Think a redundant check.
There was a problem hiding this comment.
Left a TODO, just in case it breaks anything
There was a problem hiding this comment.
We are not checking whether table has no columns but whether set of columns specified by projection definition matching actual columns is empty.
But if validity of projection columns was performed, this could indeed be redundant (problem caught elsewhere).
…-poc # Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/NativeTypeTableFilter.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/TableFilter.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodec.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodecRegistry.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/MissingJSONCodecException.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/AllJSONProjection.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/FindTableOperation.java # src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/OperationProjection.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/FindCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/FindOneAndUpdateCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/FindOneCommandResolver.java # src/main/java/io/stargate/sgv2/jsonapi/service/resolver/matcher/FilterResolver.java
|
Approved. |
Yes, unfortunately we did not have scaffolding yet for tests on API Tables and timing was such that I wanted to complete implementation fro demo. So I ran out of time to add proper testing. I agree we definitely need tests ASAP. |
| @@ -72,7 +67,7 @@ public Uni<Supplier<CommandResult>> execute( | |||
|
|
|||
| select = select.limit(params.limit()); | |||
|
|
|||
| // Building a statement using the positional values added by the TableFilter | |||
| // Building a statment using the positional values added by the TableFilter | |||
There was a problem hiding this comment.
Yes. Needs to be fixed in a follow-up.
What this PR does:
Adds support for Projection for API Tables
Which issue(s) this PR fixes:
Checklist