Description

varsenyantitan

opened on Feb 27, 2022

Issue body actions

Expected Behavior

INT64 should be packed as "C long long" to have 64 bits memory capacity.

Current Behavior

INT64 is packed as "C long" which is 32 bits

Steps to reproduce

Materialization of entities with IDs larger than supported range of "C long" raises exception.

Specifications

The bug is in the _serialize_val function in key_encoding_utils.

def _serialize_val(value_type, v: ValueProto) -> Tuple[bytes, int]:
    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("<q", v.int64_val), ValueType.INT64
    else:
        raise ValueError(f"Value type not supported for Firestore: {v}")

• Version:
• feast: 0.18.1
• Platform:
• MacOS 12.0.1
• Subsystem:
• python: 3.8.1

Possible Solution

changing struct.pack("<l", v.int64_val), ValueType.INT64 to struct.pack("<q", v.int64_val), ValueType.INT64 fixes this.

Activity

1. varsenyantitan

   added

   kind/bug

   priority/p2

   on Feb 27, 2022
2. github-actions

   assigned

   tsotnet

   on Feb 27, 2022
3. woop

   added

   priority/p0Highest priorityHighest priority

   and removed

   priority/p2

   on Feb 28, 2022
4. woop commented on Feb 28, 2022

   

   woop

   on Feb 28, 2022

   Member

   More actions

   Hi @varsenyantitan, thanks for raising this issue. We're aware of it already, but let's keep this issue open until it's resolved.

   I think we'll likely need to introduce a new materialization version and support both the old and new modes simultaneously.
5. varsenyantitan commented on Feb 28, 2022

   

   varsenyantitan

   on Feb 28, 2022

   Author

   More actions

   Hi @woop. Thank you for a fast response.
   I am just wandering about the drawbacks of the struct.pack("<q", v.int64_val), ValueType.INT64.
   Also, does the same issue present in Tecton?
6. woop commented on Feb 28, 2022

   

   woop

   on Feb 28, 2022

   Member

   More actions

   There are no drawbacks. We simply need to fix this bug. This issue is not present in Tecton.

   Reacted by Vahan Arsenyan
7. roelschr commented on Apr 23, 2022

   

   roelschr

   on Apr 23, 2022

   More actions

   Hi @woop, I'm unable to see why we'll need a new materialization version, do you have examples?

   I have made this change on a forked repo (and I'm happy to open a PR). Added a new test, while all other tests seem to pass normally. Tested in our current use case for Feast, with both local sqlite and elasticache redis online stores.

   In my mind, this change should not break current materialization use cases.
8. achals commented on Apr 25, 2022

   

   achals

   on Apr 25, 2022

   Member

   More actions

   There's more context on #1908

   Basically, the backwards compatibility issue comes in because entity keys with join keys of type int64 values that have been already written will no longer be readable - since the serialized bytes will be different with this patch, from before.

   So, when users upgrade, they will see missing values for entities that they previously would have been able to read values for. We could just require users to rematerialize, but it seems problematic to do so and I would prefer if we can come up with a backwards compatible way of doing it.

   I haven't given enough thought to it, but we could add a new field in the FeatureView proto called entity_key_encoding_version or something and use that to represent the current (buggy) way of serializing, or the new (correct) way of serializing. And then we could continue to read values for already-materialized feature views, but newly created feature views could use the newer value.

   Does that make sense?
9. roelschr commented on Apr 26, 2022

   

   roelschr

   on Apr 26, 2022

   More actions

   That makes total sense, thank you for this! I'll move my PR back to draft and try to work on this backward compatibility issue.

   Reacted by Achal Shah
10. achals commented on Apr 26, 2022

    

    achals

    on Apr 26, 2022

    Member

    More actions

    Thanks @roelschr! Please let me know if we can help in this change in any way.
11. mjurkus commented on Apr 29, 2022

    

    mjurkus

    on Apr 29, 2022

    More actions

    We ran into the same issue and we had patched on our fork too in the same way. But that's not the only issue - the java server doesn't work with our patch because of this EntityKeySerializerV2.java#L82-L85

    Is it really necessary to add backward compatibility for this?

    Reacted by Vytautas Strimaitis and Igor Gustavo Hoelscher
12. tsotnet

    removed their assignment

    on May 31, 2022
13. achals

    closed this as completedon Jul 22, 2022

Issue actions