[API-703] v5 SQL: Add default serializers for the new types #459

mdumandag · 2021-08-25T07:44:09Z

This PR adds support for the new default types, namely

LocalDate -> datetime.date
LocalTime -> datetime.time
OffsetDateTime -> datetime.datetime
LocalDateTime (only deserializing it to datetime.datetime)
BigDecimal -> decimal.Decimal

It also fixes the issues on reading/writing big integer objects.

Also, this PR updates the serialization tests with the new types
(+ sum types that are not added before to tests such as aggregators,
projections, ...)

codecov-commenter · 2021-08-25T09:20:27Z

Codecov Report

Merging #459 (7399e62) into master (d613fc8) will decrease coverage by 0.36%.
The diff coverage is 76.85%.

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   94.47%   94.10%   -0.37%     
==========================================
  Files         345      345              
  Lines       17563    17626      +63     
==========================================
- Hits        16592    16587       -5     
- Misses        971     1039      +68

Impacted Files	Coverage Δ
hazelcast/util.py	`79.35% <21.42%> (-4.55%)`	⬇️
hazelcast/serialization/serializer.py	`97.18% <95.58%> (+1.16%)`	⬆️
hazelcast/serialization/serialization_const.py	`100.00% <100.00%> (ø)`
hazelcast/serialization/service.py	`96.93% <100.00%> (+0.19%)`	⬆️
...otocol/codec/client_authentication_custom_codec.py	`50.00% <0.00%> (-23.81%)`	⬇️
hazelcast/reactor.py	`79.56% <0.00%> (-7.55%)`	⬇️
hazelcast/listener.py	`92.30% <0.00%> (-1.93%)`	⬇️
hazelcast/connection.py	`90.63% <0.00%> (-1.63%)`	⬇️
hazelcast/invocation.py	`92.72% <0.00%> (-0.77%)`	⬇️
hazelcast/proxy/base.py	`98.05% <0.00%> (+0.64%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d613fc8...7399e62. Read the comment docs.

srknzl

Looks good with minors

srknzl · 2021-08-27T08:35:39Z

hazelcast/util.py

+
+ def int_to_bytes(number):
+ # number of bytes to represent the number
+ width = (8 + (number + (number < 0)).bit_length()) // 8


this is hard to understand. I understood the logic but in a quite long time. Maybe add a link to website if you take it from somewhere or else, explain it a bit?

See 9485b4a

srknzl · 2021-08-27T11:07:07Z

tests/unit/util_test.py

+_CONVERSION_TEST_CASES = [
+ (0, [0]),
+ (-1, [255]),
+ (127, [127]),
+ (-128, [128]),
+ (999, [3, 231]),
+ (-123456, [254, 29, 192]),
+ (2147483647, [127, 255, 255, 255]),
+ (-2147483648, [128, 0, 0, 0]),
+ (9223372036854775807, [127, 255, 255, 255, 255, 255, 255, 255]),
+ (-9223372036854775808, [128, 0, 0, 0, 0, 0, 0, 0]),
+ # fmt: off
+ (
+ 23154266667777888899991234566543219999888877776666245132,
+ [0, 241, 189, 232, 38, 131, 251, 137, 60, 34, 23, 53, 163, 116, 208, 85, 14, 65, 40, 174, 120, 10, 56, 12]
+ ),
+ (
+ -23154266667777888899991234566543219999888877776666245132,
+ [255, 14, 66, 23, 217, 124, 4, 118, 195, 221, 232, 202, 92, 139, 47, 170, 241, 190, 215, 81, 135, 245, 199, 244]


Where did you get these(online or via python interpreter etc.)? Just curious..

I tried different values with Java's BigInteger

srknzl · 2021-08-27T11:25:38Z

tests/unit/serialization/binary_compatibility/README.md

@@ -7,6 +7,7 @@ However, we had to edit ``ReferenceObjects`` while generating the file because t
 such serializers for the Python client yet. The list of skipped items from ``ReferenceObjects``
 are shown below.

+* aDate


Maybe we can add a note about this that it was removed due to introducing OffsetDateTime.

This is really an internal thing so I don't think it is necessary. I will put a note about the removal of java.util.Date support in the 5.0 release notes

srknzl · 2021-08-27T12:19:49Z

hazelcast/serialization/serializer.py

+ inp.read_int(),
+ inp.read_byte(),
+ inp.read_byte(),


Are we sure that these will run in this order? (I mean across python versions etc., just to be sure)

If they do that, that would break the public API. So, I don't think it will happen at all.

srknzl · 2021-08-27T12:24:55Z

tests/unit/util_test.py

+ (-2147483648, [128, 0, 0, 0]),
+ (9223372036854775807, [127, 255, 255, 255, 255, 255, 255, 255]),
+ (-9223372036854775808, [128, 0, 0, 0, 0, 0, 0, 0]),
+ # fmt: off


What does this mean?

It is to disable black's autoformatting. It formats this long list one item per line, which does not look good. So, we disable black for this small region

This PR adds support for the new default types, namely - LocalDate -> datetime.date - LocalTime -> datetime.time - OffsetDateTime -> datetime.datetime - LocalDateTime (only deserializing it to datetime.datetime) - BigDecimal -> decimal.Decimal It also fixes the issues on reading/writing big integer objects. Also, this PR updates the serialization tests with the new types (+ sum types that are not added before to tests such as aggregators, projections, ...)

mdumandag added Type: Enhancement Source: Internal labels Aug 25, 2021

mdumandag added this to the 5.0 milestone Aug 25, 2021

mdumandag self-assigned this Aug 25, 2021

mdumandag force-pushed the new-serializers branch 2 times, most recently from f64a29d to 9b4e588 Compare August 25, 2021 08:46

mdumandag changed the title ~~v5 SQL: Add default serializers for the new types~~ [API-703] v5 SQL: Add default serializers for the new types Aug 26, 2021

srknzl self-requested a review August 27, 2021 06:59

srknzl approved these changes Aug 27, 2021

View reviewed changes

mdumandag added 2 commits August 31, 2021 12:46

escape map names

3d221bb

mdumandag force-pushed the new-serializers branch from 3c53cb2 to 3d221bb Compare August 31, 2021 09:47

mdumandag added 3 commits August 31, 2021 13:24

use updated names of the skip functions

ae209af

add comment explaining the width calculation

9485b4a

text improvements

7399e62

mdumandag merged commit 81f202d into hazelcast:master Aug 31, 2021

mdumandag deleted the new-serializers branch August 31, 2021 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API-703] v5 SQL: Add default serializers for the new types #459

[API-703] v5 SQL: Add default serializers for the new types #459

mdumandag commented Aug 25, 2021

codecov-commenter commented Aug 25, 2021 •

edited

Loading

srknzl left a comment

srknzl Aug 27, 2021

mdumandag Aug 31, 2021

srknzl Aug 27, 2021

mdumandag Aug 31, 2021

srknzl Aug 27, 2021

mdumandag Aug 31, 2021

srknzl Aug 27, 2021 •

edited

Loading

mdumandag Aug 31, 2021

srknzl Aug 27, 2021

mdumandag Aug 31, 2021

[API-703] v5 SQL: Add default serializers for the new types #459

[API-703] v5 SQL: Add default serializers for the new types #459

Conversation

mdumandag commented Aug 25, 2021

codecov-commenter commented Aug 25, 2021 • edited Loading

Codecov Report

srknzl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srknzl Aug 27, 2021 • 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

codecov-commenter commented Aug 25, 2021 •

edited

Loading

srknzl Aug 27, 2021 •

edited

Loading