GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: Use C-type long long (8 bytes) type format to pack int64 values by roelschr · Pull Request #2609 · feast-dev/feast

Conversation

@roelschr

@roelschr roelschr commented Apr 25, 2022

Copy link
Copy Markdown

What this PR does / why we need it:
This PR fixes a bug related to the materialization of INT64 entity keys, by packing int64 values as C-type "long long". It also adds a new test that tests the serialization behavior with an INT64 entity key value bigger than int32 max.

Which issue(s) this PR fixes:
Fixes #2345.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@feast-ci-bot feast-ci-bot added the size/M label Apr 25, 2022
@roelschr

roelschr commented Apr 25, 2022

Copy link
Copy Markdown
Author

/kind bug
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@feast-ci-bot feast-ci-bot added the kind/bug label Apr 25, 2022
@roelschr
roelschr force-pushed the fix-int64-serialization branch from fca7b15 to a552a6c Compare April 25, 2022 08:28
@kevjumba kevjumba added the ok-to-test label Apr 25, 2022
@roelschr
roelschr marked this pull request as draft April 26, 2022 10:12
@feast-ci-bot feast-ci-bot added the do-not-merge/work-in-progress label Apr 26, 2022
@roelschr

roelschr commented Apr 26, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

Converted to draft to work on a backwards compatible solution.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@achals achals self-assigned this Apr 26, 2022
@achals
achals self-requested a review April 26, 2022 17:18
@achals

achals commented Jun 28, 2022

Copy link
Copy Markdown
Member

@roelschr friendly ping, are you working on this? If not, I'd be more than happy to invest some time here to introduce this compatibility layer.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@achals
ci: Restore specific files during sdist builds
5e5e58a 
Signed-off-by: Achal Shah <[email protected]>
@feast-ci-bot

feast-ci-bot commented Jun 29, 2022

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: roelschr
To complete the pull request process, please assign achals after the PR has been reviewed.
You can assign the PR to them by writing /assign @achals in a comment when ready.

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

Details Needs approval from an approver in each of these files:
  • OWNERS

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 feast-ci-bot added size/L and removed size/M labels Jun 29, 2022
roelschr and others added 3 commits June 29, 2022 11:06
@roelschr @achals
Use long long type format to serialize int64 values
64fd62f 
Signed-off-by: Igor Gustavo Hoelscher <[email protected]>
@roelschr @achals
Update test to avoid type conversion
097da5c 
Signed-off-by: Igor Gustavo Hoelscher <[email protected]>
@achals
version the serialization mechanism and plumb through all the places
4d8cdb8 
Signed-off-by: Achal Shah <[email protected]>
@achals
achals force-pushed the fix-int64-serialization branch from 30b7cce to 4d8cdb8 Compare June 29, 2022 18:23
@codecov-commenter

codecov-commenter commented Jun 29, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Codecov Report

Merging #2609 (4d8cdb8) into master (7bb3e21) will decrease coverage by 21.30%.
The diff coverage is 88.88%.

@@             Coverage Diff             @@
##           master    #2609       +/-   ##
===========================================
- Coverage   80.68%   59.38%   -21.31%     
===========================================
  Files         176      177        +1     
  Lines       15670    15685       +15     
===========================================
- Hits        12643     9314     -3329     
- Misses       3027     6371     +3344
Flag Coverage Δ
integrationtests ?
unittests 59.38% <88.88%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
sdk/python/feast/batch_feature_view.py 100.00% <ø> (ø)
...thon/feast/infra/online_stores/contrib/postgres.py 32.69% <0.00%> (ø)
sdk/python/feast/on_demand_feature_view.py 74.33% <ø> (-6.80%) ⬇️
sdk/python/feast/request_feature_view.py 62.50% <ø> (ø)
sdk/python/feast/infra/online_stores/helpers.py 78.26% <75.00%> (-21.74%) ⬇️
sdk/python/feast/base_feature_view.py 71.11% <100.00%> (-9.58%) ⬇️
sdk/python/feast/feature_view.py 83.76% <100.00%> (-5.18%) ⬇️
sdk/python/feast/infra/key_encoding_utils.py 76.47% <100.00%> (-17.65%) ⬇️
sdk/python/feast/infra/online_stores/sqlite.py 93.70% <100.00%> (-0.79%) ⬇️
sdk/python/feast/registry.py 68.05% <100.00%> (-15.41%) ⬇️
... and 86 more

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 7bb3e21...4d8cdb8. Read the comment docs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
felixwang9817
felixwang9817 reviewed Jun 29, 2022
View reviewed changes
Comment thread .github/workflows/build_wheels.yml
node-version: '17.x'
registry-url: 'https://registry.npmjs.org'
- name: Build and install dependencies
# There's a `git restore` in here because `make install-go-ci-dependencies` is actually messing up go.mod & go.sum.

@felixwang9817 felixwang9817 Jun 29, 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.

remove this stuff

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/feast/infra/key_encoding_utils.py
return struct.pack("<i", v.int32_val), ValueType.INT32
elif value_type == "int64_val":
return struct.pack("<l", v.int64_val), ValueType.INT64
return struct.pack("<q", v.int64_val), ValueType.INT64

@felixwang9817 felixwang9817 Jun 29, 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.

not sure I understand the compatibility approach you're taking here - can you explain?

what do the various values of entity_key_serialization_version represent? and there needs to be some fork in the behavior here depending on entity_key_serialization_version right?

I was imagining we should do something like the following:

  • add entity_key_serialization_version to the protos, as you've done, where 0 will represent the current (broken) serialization method and 1 will represent the new (correct) serialization method
  • ensure that existing feature view protos do not magically get entity_key_serialization_version switched from 0 (the default value since the field doesn't exist) to 1, which requires special logic in the registry (as I think you've done)
  • default all new feature views to use serialization method 1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kevjumba
kevjumba reviewed Jun 30, 2022
View reviewed changes
Comment thread protos/feast/core/FeatureView.proto
// Whether these features should be served online or not
bool online = 8;

// Needed for backwards compatible behaviour when fixing

@kevjumba kevjumba Jun 29, 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.

update comment after when fixing

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@achals achals mentioned this pull request Jun 30, 2022
feat: Add Cassandra/AstraDB online store contribution #2873
Merged
@achals achals mentioned this pull request Jul 15, 2022
fix: Version entity serialization mechanism and fix issue with int64 vals #2944
Merged
@adchia adchia closed this Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevjumba kevjumba kevjumba left review comments
@felixwang9817 felixwang9817 felixwang9817 left review comments
@achals achals Awaiting requested review from achals

Uh oh!

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

Assignees

@achals achals

Labels

do-not-merge/work-in-progress kind/bug ok-to-test size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

INT64 uses "C long" type in struct.pack which is 4 bytes.

Uh oh!

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

7 participants

@roelschr @achals @feast-ci-bot @codecov-commenter @kevjumba @felixwang9817 @adchia
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.