Handle case where_LIST type is empty

[judahrand]

What this PR does / why we need it:
This PR deals with multiple issues around empty list features regarding type inference.

It is my opinion that these fixes should be somewhat of a stop gap and that really the way that types are dealt with should be overhauled in Feast.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Fix issue which prevented empty lists being materialized into online stores or retrieved from historical stores.

[feast-ci-bot]

Hi @judahrand. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[judahrand]

Fixes an issue mentioned in #1640 but not the whole issue as this does not deal with int lists.

[woop]

Thanks @judahrand, should we perhaps add a test to ensure that the change you've introduced works?

[codecov-commenter]

Codecov Report

Merging #1703 (69e43ac) into master (21f1ef7) will decrease coverage by 0.20%.
The diff coverage is 40.86%.

@@            Coverage Diff             @@
##           master    #1703      +/-   ##
==========================================
- Coverage   62.29%   62.09%   -0.21%     
==========================================
  Files          96       96              
  Lines        7363     7424      +61     
==========================================
+ Hits         4587     4610      +23     
- Misses       2776     2814      +38     

Flag	Coverage Δ
unittests	62.09% <40.86%> (-0.21%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/integration/registration/test_universal_types.py	35.41% <18.60%> (-5.82%)	⬇️
sdk/python/tests/data/data_creator.py	34.78% <33.33%> (-3.32%)	⬇️
sdk/python/feast/feature.py	74.24% <50.00%> (-0.76%)	⬇️
sdk/python/feast/type_map.py	51.53% <54.16%> (+3.26%)	⬆️
sdk/python/feast/online_response.py	84.90% <71.42%> (-7.60%)	⬇️
sdk/python/tests/conftest.py	70.31% <0.00%> (-1.57%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21f1ef7...69e43ac. Read the comment docs.

[judahrand]

Hey @woop. Took me a while to get to this but it is easier now that the tests have been reworked a tad. Added the test you asked for. Worth revisiting?

[judahrand]

Wanted to go back and get the integration tests to run with the code from master + my test to make sure this is actually still broken. Also to see if those tests that failed a second ago were just flakey.

Looks like I need approval to run them again... @achals

[judahrand]

Wanted to go back and get the integration tests to run with the code from master + my test to make sure this is actually still broken. Also to see if those tests that failed a second ago were just flakey.

Looks like I need approval to run them again... @achals

I think you might have to unlabel and re-label ok-to-test? I would run them locally but they seem to fail even with no changes on my system (possibly macOS related 🤷‍♂️ )

[judahrand]

Hmmm... Maybe I'm wrong... Any idea why the integration tests are skipping?

[adchia]

Re python_type_to_feast_value_type:

What are the downsides to relaxing the null constraint on inference in this method? It seems relatively normal to have some feature values be null / empty e.g. a user history feature for a new user.

[judahrand]

Re python_type_to_feast_value_type:

What are the downsides to relaxing the null constraint on inference in this method? It seems relatively normal to have some feature values be null / empty e.g. a user history feature for a new user.

What would you suggest it should return? ValueType.UNKNOWN? I'm not sure what the downstream consequences of that might be?

[adchia]

I think ValueType.UNKNOWN would probably throw an error since we need some type, and presumably it got here in many cases because there was no supplied type (e.g. "inference" code)

[120, null, 1, 3, 4], I'd expect that to still infer to an int list that we'd want to support.

There's likely downstream effects of asserting that though, though I think the only real store we "support" right now is BQ and materialization crashes there (#1839)

[judahrand]

I think ValueType.UNKNOWN would probably throw an error since we need some type, and presumably it got here in many cases because there was no supplied type (e.g. "inference" code)

Agreed.

[120, null, 1, 3, 4], I'd expect that to still infer to an int list that we'd want to support.

There's likely downstream effects of asserting that though, though I think the only real store we "support" right now is BQ and materialization crashes there (#1839)

Yup, that's the approach I'm trying here: infer type from all the data provided rather than infer a type for each piece of data alone. This'll still fail in the case where all the data is empty lists (or all null) which isn't great in my opinion given that there is an underlying schema from the data source (at least for the DB sources).

Should we not be inferring types from the schema rather than the data really?

[judahrand]

Tests pass!

Actually, for realz ready to be reviewed now @adchia

[judahrand]

Flakey test.... Seen it flake before.

[adchia]

/lgtm

[feast-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, judahrand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [adchia]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment