fix: Use C-type long long (8 bytes) type format to pack int64 values #2609
Conversation
|
/kind bug |
roelschr
force-pushed
the
fix-int64-serialization
branch
from
fca7b15 to
a552a6c
Compare
|
Converted to draft to work on a backwards compatible solution. |
|
@roelschr friendly ping, are you working on this? If not, I'd be more than happy to invest some time here to introduce this compatibility layer. |
Signed-off-by: Achal Shah <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: roelschr The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Igor Gustavo Hoelscher <[email protected]>
Signed-off-by: Igor Gustavo Hoelscher <[email protected]>
Signed-off-by: Achal Shah <[email protected]>
achals
force-pushed
the
fix-int64-serialization
branch
from
30b7cce to
4d8cdb8
Compare
Codecov Report
@@ Coverage Diff @@
## master #2609 +/- ##
===========================================
- Coverage 80.68% 59.38% -21.31%
===========================================
Files 176 177 +1
Lines 15670 15685 +15
===========================================
- Hits 12643 9314 -3329
- Misses 3027 6371 +3344
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -108,12 +108,15 @@ jobs: | |||
| node-version: '17.x' | |||
| registry-url: 'https://registry.npmjs.org' | |||
| - name: Build and install dependencies | |||
| # There's a `git restore` in here because `make install-go-ci-dependencies` is actually messing up go.mod & go.sum. | |||
| if value_type == "string_val": | ||
| return v.string_val.encode("utf8"), ValueType.STRING | ||
| elif value_type == "bytes_val": | ||
| return v.bytes_val, ValueType.BYTES | ||
| elif value_type == "int32_val": | ||
| return struct.pack("<i", v.int32_val), ValueType.INT32 | ||
| elif value_type == "int64_val": | ||
| return struct.pack("<l", v.int64_val), ValueType.INT64 | ||
| return struct.pack("<q", v.int64_val), ValueType.INT64 |
There was a problem hiding this comment.
not sure I understand the compatibility approach you're taking here - can you explain?
what do the various values of entity_key_serialization_version represent? and there needs to be some fork in the behavior here depending on entity_key_serialization_version right?
I was imagining we should do something like the following:
- add
entity_key_serialization_versionto the protos, as you've done, where 0 will represent the current (broken) serialization method and 1 will represent the new (correct) serialization method - ensure that existing feature view protos do not magically get
entity_key_serialization_versionswitched from 0 (the default value since the field doesn't exist) to 1, which requires special logic in the registry (as I think you've done) - default all new feature views to use serialization method 1
| @@ -75,6 +75,9 @@ message FeatureViewSpec { | |||
|
|
|||
| // Whether these features should be served online or not | |||
| bool online = 8; | |||
|
|
|||
| // Needed for backwards compatible behaviour when fixing | |||
There was a problem hiding this comment.
update comment after when fixing
What this PR does / why we need it:
This PR fixes a bug related to the materialization of INT64 entity keys, by packing int64 values as C-type "long long". It also adds a new test that tests the serialization behavior with an INT64 entity key value bigger than int32 max.
Which issue(s) this PR fixes:
Fixes #2345.