fix: Assertion condition when value is 0 #3401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kysersozelee
changed the title
kysersozelee
changed the title
kysersozelee
changed the title
felixwang9817
approved these changes
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: | |||
There was a problem hiding this comment.
nit: would you mind adding a comment indicating why this special case is necessary?
There was a problem hiding this comment.
e0c952f Left the comment. Let me know if it is not inappropriate!
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
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.There was a problem hiding this comment.
thanks @kysersozelee!
|
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
force-pushed
the
fix_valid_scala_assertion_condition
branch
from
68cb2f5
to
dcbe4bb
Compare
Signed-off-by: zlatan.el <[email protected]>
Signed-off-by: zlatan.el <[email protected]>
kysersozelee
force-pushed
the
fix_valid_scala_assertion_condition
branch
from
dcbe4bb
to
e0c952f
Compare
Signed-off-by: zlatan.el <[email protected]>
kysersozelee
force-pushed
the
fix_valid_scala_assertion_condition
branch
from
15806c0
to
033a432
Compare
Signed-off-by: zlatan.el <[email protected]>
|
[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 |
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 <[email protected]> * chore: Add comment about zero value validation Signed-off-by: zlatan.el <[email protected]> * chore: Modifiy the comment Signed-off-by: zlatan.el <[email protected]> * chore: Add the comment Signed-off-by: zlatan.el <[email protected]> Signed-off-by: zlatan.el <[email protected]> Co-authored-by: zlatan.el <[email protected]> Signed-off-by: franciscojavierarceo <[email protected]>
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 <[email protected]> * chore: Add comment about zero value validation Signed-off-by: zlatan.el <[email protected]> * chore: Modifiy the comment Signed-off-by: zlatan.el <[email protected]> * chore: Add the comment Signed-off-by: zlatan.el <[email protected]> Signed-off-by: zlatan.el <[email protected]> Co-authored-by: zlatan.el <[email protected]> Signed-off-by: franciscojavierarceo <[email protected]> * 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 <[email protected]> * linted Signed-off-by: franciscojavierarceo <[email protected]> * adding change to also support querying the event_timestamp Signed-off-by: franciscojavierarceo <[email protected]> Signed-off-by: zlatan.el <[email protected]> Signed-off-by: franciscojavierarceo <[email protected]> Co-authored-by: kysersozelee <[email protected]> Co-authored-by: zlatan.el <[email protected]>
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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