GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix: Assertion condition when value is 0 by kysersozelee · Pull Request #3401 · feast-dev/feast

Conversation

kysersozelee commented Dec 19, 2022

Copy link

Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

feast-ci-bot added the size/XS label

Dec 19, 2022

kysersozelee changed the title ~~assertion condition when value is 0~~ fix: assertion condition when value is 0

Dec 19, 2022

kysersozelee changed the title ~~fix: assertion condition when value is 0~~ Assertion condition when value is 0

Dec 19, 2022

kysersozelee changed the title ~~Assertion condition when value is 0~~ fix: Assertion condition when value is 0

Dec 19, 2022

felixwang9817 approved these changes

Dec 19, 2022

View reviewed changes

Comment thread

sdk/python/feast/type_map.py Outdated

felixwang9817 Dec 19, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kysersozelee Dec 19, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

felixwang9817 Dec 19, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kysersozelee reacted with thumbs up emoji

All reactions

👍 1 reaction

kysersozelee Dec 19, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

felixwang9817 Dec 19, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

thanks @kysersozelee!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

feast-ci-bot added the approved label

Dec 19, 2022

felixwang9817 commented Dec 19, 2022

Copy link

Copy Markdown

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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kysersozelee force-pushed the fix_valid_scala_assertion_condition branch from 68cb2f5 to dcbe4bb Compare

December 19, 2022 12:36

km-zlatan-el added 2 commits

December 19, 2022 21:38


          fix: Add assertion condition when value is 0

619f6c9

Signed-off-by: zlatan.el <[email protected]>


          chore: Add comment about zero value validation

e0c952f

Signed-off-by: zlatan.el <[email protected]>

kysersozelee force-pushed the fix_valid_scala_assertion_condition branch from dcbe4bb to e0c952f Compare

December 19, 2022 12:38

felixwang9817 approved these changes

Dec 19, 2022

View reviewed changes

felixwang9817 left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

/lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

feast-ci-bot assigned felixwang9817

Dec 19, 2022

feast-ci-bot added the lgtm label

Dec 19, 2022

felixwang9817 removed the lgtm label

Dec 19, 2022

feast-ci-bot added size/S and removed size/XS labels

Dec 19, 2022


          chore: Modifiy the comment

033a432

Signed-off-by: zlatan.el <[email protected]>

kysersozelee force-pushed the fix_valid_scala_assertion_condition branch from 15806c0 to 033a432 Compare

December 19, 2022 13:01

feast-ci-bot added size/XS and removed size/S labels

Dec 19, 2022


          chore: Add the comment

4b2fc9d

Signed-off-by: zlatan.el <[email protected]>

felixwang9817 approved these changes

Dec 19, 2022

View reviewed changes

felixwang9817 left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

/lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

feast-ci-bot added the lgtm label

Dec 19, 2022

feast-ci-bot commented Dec 19, 2022

Copy link

Copy Markdown

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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [felixwang9817]

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

feast-ci-bot merged commit 98a24a3 into feast-dev:master

Dec 20, 2022

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


          chore(release): release 0.28.0

81c3483

# [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: Assertion condition when value is 0 (feast-dev#3401)

cd9dc81

* 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


          fix: Updating the batch field so that you can query create and event …

01ab462

…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


          chore(release): release 0.29.0

d3b7c06

# [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))

Reviewers

felixwang9817

felixwang9817 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

felixwang9817

Labels

approved lgtm size/XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

kysersozelee commented Dec 19, 2022

Uh oh!

felixwang9817 Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

kysersozelee Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

felixwang9817 Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

kysersozelee Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixwang9817 Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

felixwang9817 commented Dec 19, 2022

Uh oh!

felixwang9817 left a comment

Choose a reason for hiding this comment

Uh oh!

felixwang9817 left a comment

Choose a reason for hiding this comment

Uh oh!

feast-ci-bot commented Dec 19, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kysersozelee Dec 19, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.