[
https://issues.apache.org/jira/browse/TINKERPOP-2363?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18117322#comment-18117322
]
ASF GitHub Bot commented on TINKERPOP-2363:
-------------------------------------------
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.
> GraphBinary and numerics for Python
> -----------------------------------
>
> Key: TINKERPOP-2363
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2363
> Project: TinkerPop
> Issue Type: Improvement
> Components: python
> Affects Versions: 3.4.6
> Reporter: Stephen Mallette
> Priority: Critical
>
> After TINKERPOP-2360 we've started down the path of coercing python numerics
> to Java type ranges. GraphBinary doesn't do that and therefore switching
> serializers from GraphSON to GraphBinary will have the potential to result in
> error if the python numeric overflows the Java integer space. In addition, we
> don't support {{BigInteger}} serialization apparently so that remains a
> problem. This ticket should just sort out GraphBinary and numbers all at once.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
