Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Float features are materialized into online stores as doubles #1904

Closed
Agent007 opened this issue Sep 23, 2021 · 2 comments · Fixed by #1906
Closed

Float features are materialized into online stores as doubles #1904

Agent007 opened this issue Sep 23, 2021 · 2 comments · Fixed by #1906
Assignees
Labels
good first issue Good for newcomers priority/p0 Highest priority

Comments

@Agent007
Copy link
Contributor

Agent007 commented Sep 23, 2021

Expected Behavior

For features defined as floats, online retrieval should NOT return double type.

Current Behavior

Float features are materialized into online stores as double type.

Steps to reproduce

Add the following assertion inside the _assert_online_features() function in test_e2e_local.py:

assert response.field_values[0].fields[f"driver_hourly_stats__conv_rate"].float_val > 0

Specifications

  • Version: 0.13.0
  • Platform: all
  • Subsystem: Python SDK

Possible Solution

In the function python_value_to_proto_value() of type_map.py,
value_type perhaps should be set to feature_type if feature_type is not None:

def python_value_to_proto_value(
    value: Any, feature_type: ValueType = None
) -> ProtoValue:
    value_type = (
        feature_type
        if feature_type is not None
        else python_type_to_feast_value_type("", value)
    )
    return _python_value_to_proto_value(value_type, value)
@achals achals added good first issue Good for newcomers priority/p0 Highest priority labels Sep 23, 2021
@achals
Copy link
Member

achals commented Sep 23, 2021

Thanks for the bug report @Agent007 - I noticed the same thing yesterday when working on feast-dev/feast-java-old#37.

@Agent007
Copy link
Contributor Author

@achals You're very welcome! I created a PR for the fix. Wasn't sure if you hadn't seen it yet or if you've been busy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers priority/p0 Highest priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants