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: Assertion condition when value is 0 #3401

Merged

Conversation

kysersozelee
Copy link
Contributor

What this PR does / why we need it:
When materializing a feature defined as a float type, if int is 0, materialization fails.

Which issue(s) this PR fixes:

Fixes #3400

@kysersozelee kysersozelee changed the title assertion condition when value is 0 fix: assertion condition when value is 0 Dec 19, 2022
@kysersozelee kysersozelee changed the title fix: assertion condition when value is 0 Assertion condition when value is 0 Dec 19, 2022
@kysersozelee kysersozelee changed the title Assertion condition when value is 0 fix: Assertion condition when value is 0 Dec 19, 2022
@@ -402,7 +402,10 @@ def _python_value_to_proto_value(
valid_scalar_types,
) = PYTHON_SCALAR_VALUE_TYPE_TO_PROTO_VALUE[feast_value_type]
if valid_scalar_types:
assert type(sample) in valid_scalar_types
if sample == 0 or sample == 0.0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would you mind adding a comment indicating why this special case is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e0c952f Left the comment. Let me know if it is not inappropriate!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kysersozelee hm I think this comment is still not super clear to me - it would be great if you could add why type validation cannot be performed with only one type (if I understand correctly from #3400, the issue is because you might e.g. have an int value in a float column)

Copy link
Contributor Author

@kysersozelee kysersozelee Dec 19, 2022

Choose a reason for hiding this comment

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

@felixwang9817

Left the two comments. Let me know if it is inappropriate!

# Numpy convert 0 to int. However, in the feature view definition, the type of column may be a float.
# So, if value is 0, type validation must pass if scalar_types are either int or float.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks @kysersozelee!

@felixwang9817
Copy link
Collaborator

hey @kysersozelee thanks for the PR! I left a small comment, and you'll have to sign your commit for the DCO check to pass - otherwise lgtm!

@kysersozelee kysersozelee force-pushed the fix_valid_scala_assertion_condition branch from 68cb2f5 to dcbe4bb Compare December 19, 2022 12:36
Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>
Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>
@kysersozelee kysersozelee force-pushed the fix_valid_scala_assertion_condition branch from dcbe4bb to e0c952f Compare December 19, 2022 12:38
Copy link
Collaborator

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

/lgtm

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>
@kysersozelee kysersozelee force-pushed the fix_valid_scala_assertion_condition branch from 15806c0 to 033a432 Compare December 19, 2022 13:01
Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>
Copy link
Collaborator

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: felixwang9817, kysersozelee

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:

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

@feast-ci-bot feast-ci-bot merged commit 98a24a3 into feast-dev:master Dec 20, 2022
@kysersozelee kysersozelee deleted the fix_valid_scala_assertion_condition branch December 20, 2022 21:55
felixwang9817 pushed a commit that referenced this pull request Jan 3, 2023
# [0.28.0](v0.27.0...v0.28.0) (2023-01-03)

### Bug Fixes

* Apply billing project when infer schema ([#3417](#3417)) ([4f9ad7e](4f9ad7e))
* Assertion condition when value is 0 ([#3401](#3401)) ([98a24a3](98a24a3))
* Enable registry caching in SQL Registry ([#3395](#3395)) ([2e57376](2e57376))
* Fix bug where SQL registry was incorrectly writing infra config around online stores ([#3394](#3394)) ([6bcf77c](6bcf77c))
* Get all columns with describe table method from RedshiftData-api ([#3377](#3377)) ([fd97254](fd97254))
* ODFV able to handle boolean pandas type ([#3384](#3384)) ([8f242e6](8f242e6))
* Remove PySpark dependency from Snowflake Offline Store ([#3388](#3388)) ([7b160c7](7b160c7))
* Specifies timeout in exception polling ([#3398](#3398)) ([c0ca7e4](c0ca7e4))
* Update import logic to remove `pyspark` dependency from Snowflake Offline Store ([#3397](#3397)) ([cf073e6](cf073e6))

### Features

* Add template for Github Codespaces ([#3421](#3421)) ([41c0537](41c0537))
* Adds description attribute for features/fields ([#3425](#3425)) ([26f4881](26f4881))
* Snowflake skip materialization if no table change ([#3404](#3404)) ([0ab3942](0ab3942))
franciscojavierarceo pushed a commit to franciscojavierarceo/feast that referenced this pull request Jan 4, 2023
* fix: Add assertion condition when value is 0

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

* chore: Add comment about zero value validation

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

* chore: Modifiy the comment

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

* chore: Add the comment

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>
Co-authored-by: zlatan.el <zlatan.el@kakaomobility.com>
Signed-off-by: franciscojavierarceo <francisco.arceo@affirm.com>
feast-ci-bot pushed a commit that referenced this pull request Jan 5, 2023
…date. (#3411)

* fix: Assertion condition when value is 0 (#3401)

* fix: Add assertion condition when value is 0

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

* chore: Add comment about zero value validation

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

* chore: Modifiy the comment

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

* chore: Add the comment

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>
Co-authored-by: zlatan.el <zlatan.el@kakaomobility.com>
Signed-off-by: franciscojavierarceo <francisco.arceo@affirm.com>

* updating the batch field so that if you want return the created date of a model you can just add it in the get_online_features feature argument

Signed-off-by: franciscojavierarceo <francisco.arceo@affirm.com>

* linted

Signed-off-by: franciscojavierarceo <francisco.arceo@affirm.com>

* adding change to also support querying the event_timestamp

Signed-off-by: franciscojavierarceo <francisco.arceo@affirm.com>

Signed-off-by: zlatan.el <zlatan.el@kakaomobility.com>
Signed-off-by: franciscojavierarceo <francisco.arceo@affirm.com>
Co-authored-by: kysersozelee <kysersoze.lee@gmail.com>
Co-authored-by: zlatan.el <zlatan.el@kakaomobility.com>
kevjumba pushed a commit that referenced this pull request Jan 31, 2023
# [0.29.0](v0.28.0...v0.29.0) (2023-01-31)

### Bug Fixes

* Add check for bool type in addition to sample ([#3452](#3452)) ([1c7c491](1c7c491))
* Buggy SQL for postgres source ([#3424](#3424)) ([1ea100e](1ea100e))
* Ensure no duplicates in `fv.schema` ([#3460](#3460)) ([08ffa8d](08ffa8d))
* Fix delete sfv twice issue ([#3466](#3466)) ([dfd5eae](dfd5eae))
* Stream feature view UI shows transformation issue ([#3464](#3464)) ([1ef5137](1ef5137))
* Update registry.refresh to have a default arg ([#3450](#3450)) ([2f7c4ed](2f7c4ed))
* Updating the batch field so that you can query create and event date. ([#3411](#3411)) ([01ab462](01ab462)), closes [#3401](#3401)

### Features

* Add data source search ([#3449](#3449)) ([fbbb293](fbbb293))
* Adding list_validation_references for default and sql registry ([#3436](#3436)) ([21dd253](21dd253))
* Make UI accessible behind proxy ([#3428](#3428)) ([753d8db](753d8db))
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.

sampled scalar value verification failure for 0 value if float is defined
4 participants