Fixes #1365: add integral number codecs, tests #1367

tatu-at-datastax · 2024-08-27T21:49:25Z

What this PR does:

Changes table-shredding to convert JSON values (filter, insert) to use 3 types -- Long, BigInteger, BigDecimal -- instead of just BigDecimal, for less wasteful conversions (BigDecimal use for integral numbers is significant overhead).

Adds more CQL codecs to convert to/from CQL integral types; tests (Unit and Integration).

Which issue(s) this PR fixes:
Fixes #1365

Checklist

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

tatu-at-datastax · 2024-08-29T02:00:32Z

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

 /** Filter to use any JSON number against a table column. */
-public class NumberTableFilter extends NativeTypeTableFilter<BigDecimal> {
+public class NumberTableFilter extends NativeTypeTableFilter<Number> {


Need to change now that we can get values in 3 distinct types (Long, BigInteger, BigDecimal).

tatu-at-datastax · 2024-08-29T16:20:30Z

Getting there:

Codecs added for all 5 integral CQL types, from all 3 JSON number types (Long, BigInteger, BigDecimal
Range checks working EXCEPT for Long case (no ready-made method to use, need to implement)
No dedicated Integration Tests yet, minor adds to "findOne" for tables but not sufficient.

…short/byte)

tatu-at-datastax · 2024-08-29T18:26:27Z

Bounds-/range-check added for smallint/tinyint/int conversion from Long; only missing insertOne integration tests. Ready to start review.

tatu-at-datastax · 2024-08-30T00:14:52Z

src/test/java/io/stargate/sgv2/jsonapi/api/v1/util/DataApiCommandSenderBase.java

+ * Helper class used for constructing and sending commands to the Data API for testing purposes
+ * (usually integration tests).
+ */
+public abstract class DataApiCommandSenderBase<T extends DataApiCommandSenderBase> {


So far 2 implementations (namespace/keyspace commands, table/collection commans); not sure if Table/Collection should be separated, probably not. Self-recursive type to allow type-safe "this" return for chaining.
Share most of configuration.

src/test/java/io/stargate/sgv2/jsonapi/api/v1/util/DataApiCommandSenderBase.java

tatu-at-datastax · 2024-08-30T01:09:06Z

And now with more-compact-than-before abstractions for more efficient (wrt reading, writing) IT writing. Ready for constructive feedback.

amorton · 2024-08-30T18:58:30Z

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

+    }
+
+    static void throwOverflow(DataType targetCQLType, Number value) {
+      throw new ArithmeticException(String.format("Overflow, value too big for %s", targetCQLType));


nit: can we include the value in the message (and for the underflow)

Also nice attention to detail on overflow and underflow - I checked Math.toIntExact() and it says any loss of information is an overflow.

Good point -- I was doing that, but realized that safeNumber() that catches ArithmeticException and re-wraps as ToCQLCodecException adds the actual value.
This is why I chose to avoid adding it in ArithmeticException itself (since message is included as-is and value then duplicated).

On underflow: yeah I must admit I googled for definitions since I recall "overflow" sometimes being used to mean both, too. :)

amorton · 2024-08-30T19:05:28Z

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

+          JSONCodec.ToCQL.safeNumber(JSONCodec.ToCQL::safeLongToInt),
+          JSONCodec.ToJSON.unsafeNodeFactory(JsonNodeFactory.instance::numberNode));
+
+  public static final JSONCodec<BigDecimal, Short> SMALLINT_FROM_BIG_DECIMAL =


is this correct that we have to smallint from BigDecimal - i thought all integral would come from BigInteger ?

This is needed in cases where JSON has, say, 1.0 and I think we want to allow use like that. Jackson bases type of textual representation of JSON Number so if there's decimal point, it's FP, even if fractional part is all zeroes.

I am assuming we want to allow this coercion (existing code, when only using BigDecimal, did allow this): if not, could drop this converter.

amorton · 2024-08-30T19:07:00Z

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

-  // 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).
+  private static final JSONCodec<BigInteger, Long> BIGINT_FROM_BIG_INTEGER =


I get what we are doing with integral values from Jackson - question is what need to support both BigInteger and Long source values ? if it is needed can we add a comment at top to explain strategy .

And that the type mappings here must sync with how we pull values from Jackson.

So, with FPs, cost of processing BigDecimal seems to be roughly similar to Double.
But cost of processing BigInteger is significantly higher than that of boxed primitive integral types. So I figured it is a compromise to try to avoid huge number of converters by slight addition -- I think minimally we could go with BigDecimal and BigInteger but prefer adding Long in between (but nothing else, things do get unwieldy).

I'll add a comment here.

100% on mentioning that handling with RowShredder must be kept in sync.

I added some class Javadoc comments in RowShredder and JSONCodec, hope that suffices.

amorton · 2024-08-30T19:15:47Z

src/main/java/io/stargate/sgv2/jsonapi/service/resolver/matcher/TableFilterResolver.java

@@ -104,7 +103,7 @@ private static List<DBFilterBase> findDynamic(CaptureExpression captureExpressio
                captureExpression.path(),
                NativeTypeTableFilter.Operator.from(
                    (ValueComparisonOperator) filterOperation.operator()),
-                (BigDecimal) rhsValue));
+                (Number) rhsValue));


Just a note - the changes yuqi and I are doing will revert how the resolver functions here are called back to the original approach where it gets called once with everything that matched. Currently it is getting called for every match.

Nothing to change, just a note that when we do that we should look at how to reduce the use of instanceof and casting

Thank you: that sounds like a good improvement.

amorton · 2024-08-30T19:17:26Z

src/main/java/io/stargate/sgv2/jsonapi/service/shredding/tables/RowShredder.java

      case STRING -> value.textValue();
      case BOOLEAN -> value.booleanValue();
      case NULL -> null;
      default -> throw new RuntimeException("Unsupported type");
    };
  }
+


I know this class is not finished / high quality - can you add a quick comment to say the different types for numbers need to match the codecs in JSONCodecRegistry

Added something in class javadoc, but will also add it right here too.

src/test/java/io/stargate/sgv2/jsonapi/api/v1/util/DataApiResponseValidator.java

amorton · 2024-08-30T19:37:13Z

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

@@ -16,7 +17,7 @@

 public class JSONCodecRegistryTest {

-  private final JSONCodecRegistryTestData TEST_DATA = new JSONCodecRegistryTestData();
+  private static final JSONCodecRegistryTestData TEST_DATA = new JSONCodecRegistryTestData();


as a pattern, i had been keeping the "TestData" class as instance but I guess given how tests work static makes sense.

That is unfortunate, but it looks like ParameterizedTest requires source method (I tried and non-static ones do not seem to work? LMK if you know of a way) to be static.
I'd prefer non-static as well so if there's a way, that'd be better.

amorton · 2024-08-30T19:39:29Z

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

-        Arguments.of(DataTypes.INT, BigDecimal.valueOf(100), 100) // second 100 is an int
-        );
+        // Integer types:
+        Arguments.of(DataTypes.BIGINT, -456L, -456L),


nit: consider rename to something like validCodecToCQL.... now you have out of range below.

Excellent point, changing

amorton · 2024-08-30T19:43:34Z

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

+        Arguments.of(DataTypes.VARINT, -39999L, BigInteger.valueOf(-39999)),
+        Arguments.of(DataTypes.VARINT, BigInteger.valueOf(1), BigInteger.valueOf(1)),
+        Arguments.of(
+            DataTypes.VARINT, BigDecimal.valueOf(123456789.0), BigInteger.valueOf(123456789)),


I've not checked that the list here and below is exhaustive, let me know if you want it checked.

Yeah it's not exhaustive in many ways (i.e. does not try different ranges, values), but does try to cover permutations of 3 input "plain" Java types (BigDecimal, BigInteger, Long) x CQL integer types.

So if you can sanity check, that'd be great.

amorton

Generally like where it is going, just small comments on nit picks and making some things clearer.

amorton · 2024-08-30T19:52:08Z

src/test/java/io/stargate/sgv2/jsonapi/api/v1/util/DataApiCommandSenders.java

+    return new DataApiNamespaceCommandSender(namespace);
+  }
+
+  public static DataApiTableCommandSender tableCommand(String namespace, String tableName) {


idea - if we name these assertTableCommand, then do static imports in tests, and make the other name change below it could look like this in a test:

assertTableCommand(namespace, table) .findOne({})

Great idea, will change method name.

Could change class name too, to DataApiCommandAssertions too?

The only downside is that these look odd as set up components (creating table, inserting rows, that are not part of test).
Will change, we can re-think if need be.

amorton

approved

vkarpov15

LGTM

Fixes #1365: add integral number codecs, tests

1119a72

tatu-at-datastax self-assigned this Aug 27, 2024

Start adding more codecs for 3 distinct number types

7be9d68

tatu-at-datastax changed the title ~~Fixes #1365: add integral number codecs, tests~~ (WIP) Fixes #1365: add integral number codecs, tests Aug 27, 2024

tatu-at-datastax added 7 commits August 27, 2024 16:09

Add converters to fix ITs

c1f13a6

Add more unit tests

4719af7

mvn fmt:format

4231072

Merge branch 'main' into tatu/1365-codec-number-int

c0ee476

Merge branch 'main' into tatu/1365-codec-number-int

331ab15

Complete set of codecs for integer types

05457f4

Add tests for range checks

7ea9231

tatu-at-datastax commented Aug 29, 2024

View reviewed changes

tatu-at-datastax added 3 commits August 28, 2024 19:15

Improve error messages, add tests

abe2d87

Minor streamlining of IT

f6d7eed

...

0a91a0b

Add bounds checks for codec conversion for int/smallint/tinyint (int/…

9fb5298

…short/byte)

tatu-at-datastax marked this pull request as ready for review August 29, 2024 18:25

tatu-at-datastax requested a review from a team as a code owner August 29, 2024 18:25

tatu-at-datastax changed the title ~~(WIP) Fixes #1365: add integral number codecs, tests~~ Fixes #1365: add integral number codecs, tests Aug 29, 2024

tatu-at-datastax added 3 commits August 29, 2024 16:51

Add an IT test "mini-framework", fix ErrorCode initialization

f7362e4

mvn fmt:format

59782d3

Merge branch 'main' into tatu/1365-codec-number-int

f9ff396

tatu-at-datastax commented Aug 30, 2024

View reviewed changes

src/test/java/io/stargate/sgv2/jsonapi/api/v1/util/DataApiCommandSenderBase.java Show resolved Hide resolved

tatu-at-datastax added 2 commits August 29, 2024 17:30

More IT refactoring

bd7b3e8

...

97f1a5d

Merge branch 'main' into tatu/1365-codec-number-int

41ac40e

amorton requested changes Aug 30, 2024

View reviewed changes

amorton reviewed Aug 30, 2024

View reviewed changes

tatu-at-datastax added 6 commits August 30, 2024 15:36

Comment improvements as per code review

879c5ef

More changes based on code review

3bbc32d

More refactoring as per code review

1ecba72

Change DataApiResponseValidator immutable

a511df5

Last (?) changes from code review (for first pass)

b4c47c0

Fix a copy-paste error

255dbae

amorton approved these changes Sep 3, 2024

View reviewed changes

tatu-at-datastax merged commit 869d6af into main Sep 3, 2024
3 checks passed

tatu-at-datastax deleted the tatu/1365-codec-number-int branch September 3, 2024 17:58

vkarpov15 reviewed Sep 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1365: add integral number codecs, tests #1367

Fixes #1365: add integral number codecs, tests #1367

tatu-at-datastax commented Aug 27, 2024 •

edited

Loading

tatu-at-datastax Aug 29, 2024

tatu-at-datastax commented Aug 29, 2024

tatu-at-datastax commented Aug 29, 2024

tatu-at-datastax Aug 30, 2024 •

edited

Loading

tatu-at-datastax commented Aug 30, 2024

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024 •

edited

Loading

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024 •

edited

Loading

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024 •

edited

Loading

tatu-at-datastax Aug 30, 2024

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024

amorton left a comment

amorton Aug 30, 2024

tatu-at-datastax Aug 30, 2024

tatu-at-datastax Aug 30, 2024

amorton left a comment

vkarpov15 left a comment

Fixes #1365: add integral number codecs, tests #1367

Fixes #1365: add integral number codecs, tests #1367

Conversation

tatu-at-datastax commented Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

tatu-at-datastax commented Aug 29, 2024

tatu-at-datastax commented Aug 29, 2024

tatu-at-datastax Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

tatu-at-datastax commented Aug 30, 2024

Choose a reason for hiding this comment

tatu-at-datastax Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatu-at-datastax Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatu-at-datastax Aug 30, 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

amorton left a 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 left a comment

Choose a reason for hiding this comment

vkarpov15 left a comment

Choose a reason for hiding this comment

tatu-at-datastax commented Aug 27, 2024 •

edited

Loading

tatu-at-datastax Aug 30, 2024 •

edited

Loading

tatu-at-datastax Aug 30, 2024 •

edited

Loading

tatu-at-datastax Aug 30, 2024 •

edited

Loading

tatu-at-datastax Aug 30, 2024 •

edited

Loading