POC for table projections and using JSONCodec #1314

amorton · 2024-07-25T22:45:08Z

NOTE: this PR builds on the ajm/filter-codec-poc #1313 work, it should be able to be applied to main after that. I can rebase if needed

CI https://github.com/stargate/data-api/actions/runs/10102613949

What this PR does:

Adds a framework for projections with tables, and a JSONCodec for mapping the values between java/json and CQL.

See comments on the 3 commits .

Which issue(s) this PR fixes:
Fixes #

Checklist

[*] Changes manually tested
Automated Tests added/updated
Documentation added/updated
[*] CLA Signed: DataStax CLA

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

tatu-at-datastax · 2024-07-26T20:16:25Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodec.java

+     * @throws ToJSONCodecException Checked exception raised for any error, users of the function
+     *     must catch and convert to the appropriate error for the use case.
+     */
+    JsonNode apply(ObjectMapper objectMapper, DataType fromCQLType, CqlT value)


Since we don't have to use very vague name like "apply()", could use a more meaningful name;
toJson() or whatever.

fixed as toJson

tatu-at-datastax · 2024-07-26T20:23:01Z

.../java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodecRegistry.java

+          DataTypes.BOOLEAN,
+          JSONCodec.ToCQL.unsafeIdentity(),
+          JSONCodec.ToJSON.unsafeNodeFactory(JsonNodeFactory.instance::booleanNode));
+
  // Numeric Codecs
  public static final JSONCodec<BigDecimal, Long> BIGINT =


For performance reasons we could also consider only converting FP values into BigDecimal JsonNode -- but converting CQL integer values into long-valued JsonNode.
I think our internal handling can deal with Integer and Long valued JsonNodes and this avoids some of BigDecimal overhead (avoids conversion overhead, serialization is faster).

tatu-at-datastax · 2024-07-26T20:23:43Z

.../stargate/sgv2/jsonapi/service/operation/filters/table/codecs/MissingJSONCodecException.java

+ */
+public class MissingJSONCodecException extends Exception {
+
+  // TODO: both javTupe and value may be null when going toJSON


typo: javTupe -> javaType

tatu-at-datastax · 2024-07-26T20:26:45Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/InsertTableOperation.java

-    Map<CqlIdentifier, Term> colValues =
-        row.allColumnValues().entrySet().stream()
-            .collect(Collectors.toMap(Map.Entry::getKey, e -> literal(e.getValue())));
+    Preconditions.checkArgument(


Should throw proper exception if and when this can be triggered by an insert operation (if not, it's an assertion and this is fine).

I will leave these checks and not blocking the merge,
since this is only on the table path, we will refactor these when putting details into these poc work

tatu-at-datastax · 2024-07-26T20:27:41Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/InsertTableOperation.java

+              : ongoingInsert.value(entry.getKey(), bindMarker());
+    }
+
+    assert ongoingInsert != null;


I don't think we use (or should use Java assert keyword) -- Objects.requireNonNull() better.

I will leave these checks and not blocking the merge,
since this is only on the table path, we will refactor these when putting details into these poc work

maheshrajamani · 2024-07-29T01:20:45Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodec.java

 * @param <JavaT> The type of the Java object that needs to be transformed into the type CQL expects
 * @param <CqlT> The type Java object the CQL driver expects
 */
 public record JSONCodec<JavaT, CqlT>(
-    GenericType<JavaT> javaType, DataType targetCQLType, FromJava<JavaT, CqlT> fromJava)
-    implements BiPredicate<DataType, Object> {
+    GenericType<JavaT> javaType,


The codec looks fine for primitive type. Needs a revisit when we doing complex types where only few fields will need to be returned. Will we be creating custom Codec based on user requests?

added a TODO

maheshrajamani · 2024-07-29T01:24:28Z

.../java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodecRegistry.java

+      TableMetadata table, CqlIdentifier column, Object value)
+      throws UnknownColumnException, MissingJSONCodecException {
+
+    Preconditions.checkNotNull(table, "table must not be null");


These checks will create runtime exceptions. Need to be handled with error codes.

maheshrajamani · 2024-07-29T01:31:45Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/AllJSONProjection.java

+          try {
+            return objectMapper.readTree(row.getString("[json]"));
+          } catch (Exception e) {
+            throw new NotImplementedException("BANG " + e.getMessage());


Needs a better error message. IMO shouldn't go into main branch

+1, many checks and error msgs need to be changed
refactor the message

Yuqi-Du · 2024-07-29T17:57:13Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/table/codecs/JSONCodec.java

@@ -28,42 +29,71 @@
 *     CQL expects.
 * @param targetCQLType {@link DataType} of the CQL column type the Java object needs to be
 *     transformed into.
- * @param fromJava Function that transforms the Java object into the CQL object
+ * @param toCQL Function that transforms the Java object into the CQL object


Missing another param explanation toJSON

# 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/tables/FindTableOperation.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

amorton added 5 commits July 26, 2024 07:44

Added ValidatableCommandClause POC

7a76d3e

Encapsulates behavior for clauses that can be validated and better supports schema object types.

POC for OperationProjection

1c209f3

POC to show pushing the projection down how we build the select and how we build the document from the row. see OperationProjection

POC for using JSONCodec on inserts

f102417

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

POC of JSONCodec for creating JSON

b12038d

Expands the JSONCode to support mapping toJSON from the objects the driver sent, to be used in the projection

amorton requested a review from a team as a code owner July 25, 2024 22:45

tatu-at-datastax mentioned this pull request Jul 26, 2024

POC for table filters with codecs for findOne #1313

Merged

2 tasks

tatu-at-datastax reviewed Jul 26, 2024

View reviewed changes

tatu-at-datastax approved these changes Jul 26, 2024

View reviewed changes

maheshrajamani reviewed Jul 29, 2024

View reviewed changes

Yuqi-Du reviewed Jul 29, 2024

View reviewed changes

fix comments and leave todos

f8511ba

Base automatically changed from ajm/filter-codec-poc to main July 29, 2024 22:40

Yuqi-Du merged commit d9b9cd5 into main Jul 29, 2024
3 checks passed

Yuqi-Du deleted the ajm/projection-codecs branch July 29, 2024 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC for table projections and using JSONCodec #1314

POC for table projections and using JSONCodec #1314

amorton commented Jul 25, 2024 •

edited

Loading

tatu-at-datastax Jul 26, 2024

Yuqi-Du Jul 29, 2024

tatu-at-datastax Jul 26, 2024

Yuqi-Du Jul 29, 2024

tatu-at-datastax Jul 26, 2024

Yuqi-Du Jul 29, 2024

tatu-at-datastax Jul 26, 2024

Yuqi-Du Jul 29, 2024

tatu-at-datastax Jul 26, 2024

Yuqi-Du Jul 29, 2024

maheshrajamani Jul 29, 2024

Yuqi-Du Jul 29, 2024

maheshrajamani Jul 29, 2024

maheshrajamani Jul 29, 2024

Yuqi-Du Jul 29, 2024

Yuqi-Du Jul 29, 2024

Yuqi-Du Jul 29, 2024

POC for table projections and using JSONCodec #1314

POC for table projections and using JSONCodec #1314

Conversation

amorton commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amorton commented Jul 25, 2024 •

edited

Loading