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

fix: Handles null values in data during GO Feature retrieval #4274

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

EXPEbdodla
Copy link
Contributor

What this PR does / why we need it:

Null values are materialized to online store as &types.Value{} instead of nil. Fixing the issue in Go Feature Server code to handle NULL values during retrieval. Without this fix, Go Feature server retrieval fails.

Fixes

  • Go Feature server retrieval for NULL values in online store.
  • Formatting in type_map.py and Log error when NULL values in INT are upcasted to FLOAT64. Logging helps to easily find the issue otherwise it's hard to debug the issue. Dataset has INT values when the TypeError is raised all the values are logged as FLOAT64 in _type_err() function.

We need to fix NULL values in the array during the retrieval process. Below are my observations:

Null values inside arrays are not supported by either Materialization or Feature Retrieval in GO

  • INT32, INT64 columns are upcasted to float64 when Null value exists in array. So it fails with invalid type error during materialization. Logging exception when this happens.
  • String arrays fail materialization when Null value exists
  • Materialization works for float, double and timestamp. It's stored as Nan. On Go server side, json encoding not converting NaN to None. Feature server retrieval (GO) doesn't work for float, double but works for timestamp because of the default value set when NULL values are encountered in timestamp array.

Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>
Bhargav Dodla added 2 commits June 12, 2024 20:34
Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>
Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>
Copy link
Collaborator

@HaoXuAI HaoXuAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@HaoXuAI HaoXuAI merged commit c491e57 into feast-dev:master Jun 13, 2024
21 checks passed
franciscojavierarceo pushed a commit that referenced this pull request Jun 18, 2024
# [0.39.0](v0.38.0...v0.39.0) (2024-06-18)

### Bug Fixes

* Feast UI importlib change ([#4248](#4248)) ([5d486b8](5d486b8))
* Feature server no_feature_log argument error ([#4255](#4255)) ([15524ce](15524ce))
* Feature UI Server image won't start in an OpenShift cluster ([#4250](#4250)) ([4891f76](4891f76))
* Handles null values in data during GO Feature retrieval ([#4274](#4274)) ([c491e57](c491e57))
* Make Java gRPC client use timeouts as expected ([#4237](#4237)) ([f5a37c1](f5a37c1))
* Remove self assignment code line. ([#4238](#4238)) ([e514f66](e514f66))
* Set default values for feature_store.serve() function ([#4225](#4225)) ([fa74438](fa74438))

### Features

* Add online_read_async for dynamodb ([#4244](#4244)) ([b5ef384](b5ef384))
* Add the ability to list objects by `tags` ([#4246](#4246)) ([fbf92da](fbf92da))
* Added deadline to gRPC Java client ([#4217](#4217)) ([ff429c9](ff429c9))
* Adding vector search for sqlite ([#4176](#4176)) ([2478831](2478831))
* Change get_online_features signature, move online retrieval functions to utils ([#4278](#4278)) ([7287662](7287662))
* Feature/adding remote online store ([#4226](#4226)) ([9454d7c](9454d7c))
* List all feature views ([#4256](#4256)) ([36a574d](36a574d))
* Make RegistryServer writable ([#4231](#4231)) ([79e1143](79e1143))
* Remote offline Store  ([#4262](#4262)) ([28a3d24](28a3d24))
* Set optional full-scan for deletion ([#4189](#4189)) ([b9cadd5](b9cadd5))
@EXPEbdodla EXPEbdodla deleted the fix_null_values_in_proto branch June 20, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants