bitflicker64 opened a new pull request, #3672: URL: https://github.com/apache/tinkerpop/pull/3672
https://issues.apache.org/jira/browse/TINKERPOP-2363 ## Problem `IntIO` inherits `dictify` from `LongIO`. The guard there validates against the Java `long` range, but the pack it then calls is `IntIO`'s `int32_pack`: ```python class LongIO(_GraphBinaryTypeIO): byte_format_pack = int64_pack @classmethod def dictify(cls, obj, writer, to_extend, as_value=False, nullable=True): if obj < -9223372036854775808 or obj > 9223372036854775807: raise Exception("Value too big, please use bigint Gremlin type") else: cls.prefix_bytes(cls.graphbinary_type, as_value, nullable, to_extend) to_extend.extend(cls.byte_format_pack(obj)) # int32_pack when cls is IntIO return to_extend class IntIO(LongIO): python_type = IntType # plain python int byte_format_pack = int32_pack ``` Python 3 has a single arbitrary precision `int`, so `python_type = IntType` matches every plain `int`. Any value between the Java `int` and `long` bounds clears the guard and then dies in the packer: ``` struct.error: 'i' format requires -2147483648 <= number <= 2147483647 ``` The traceback stops inside `struct` and names neither the value, nor the binding it came from, nor `statics.long`. GraphSON coerces the same values (`Int64` since TINKERPOP-2360), so a script that works on GraphSON fails on GraphBinary, which has been the Python default since 3.6.1 (TINKERPOP-2723). ## Why it bites TINKERPOP-2943 is the same bug reported against `g.V(2**31 + 1)` on a graph with more than 2^31 vertices. It was closed as a duplicate of this ticket, with the promotion fix suggested in the comments. Any Python client that round trips element ids into bindings is exposed, and only once the graph is large enough for ids to cross 2^31. Below that boundary the identical code is fine, so it presents as flakiness rather than a type problem. It was found here while loading a 34.7M edge dataset against JanusGraph 1.1.0: one load completed, the next identical one aborted partway through, depending on where id allocation happened to land. ## Fix `IntIO` gets its own `dictify`: - inside the Java `int` range, write `DataType.int`, byte for byte as before - outside it but inside the Java `long` range, write `DataType.long` - beyond that, the existing `Value too big, please use bigint Gremlin type` error `statics.long` and `statics.bigint` keep working as explicit overrides. Promotion is skipped when `as_value` is true, since a value-only write has no type code in front of it to change. The one such call site is `Duration`'s nanosecond field, whose value is `timedelta.microseconds * 1000` and cannot overflow `int32`. The structural `int32_pack` writes (collection lengths, bytecode instruction counts, `BigDecimal.scale`, string byte lengths) call the packer directly rather than going through `writer.to_dict`, so nothing that has to stay four bytes is touched. ## Tests Added to `tests/unit/io/test_graphbinaryV1.py`: - round trip of `2**31`, `-2**31 - 1`, `3000000000`, and the `int64` bounds - exact wire bytes at the boundary: `0x01` for `2**31 - 1` and `-2**31`, `0x02` one step outside, so a value that already fits is never widened - `2**63` and `-2**63 - 1` still raise - promotion through nested lists and dict values, the shape the original report failed on - `bool` still resolves to the Boolean serializer, since `bool` subclasses `int` `pytest tests/unit` passes, 114 tests. The reproduction was confirmed against this branch before and after the change; it has not been run against a live Gremlin Server. ## Open questions - TINKERPOP-2363 also covers `BigInteger`. That half now looks handled by `BigIntIO` (TINKERPOP-3275), so this PR only takes the `int`/`long` part. Say so if you would rather close the ticket in one pass. - GraphSON goes one step further and promotes past the Java `long` range to `BigInteger`. This keeps the existing explicit error instead. Happy to match GraphSON if you prefer. - `ShortIO` inherits the same mismatched guard, but `short` is an explicit opt in through `statics`, so overflowing it is a caller's choice rather than a silent default. Left out to keep this focused. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
