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: Update SparkSource to have proper comparable that inspects SparkOptions by thechopkins · Pull Request #3819 · feast-dev/feast

Conversation

@thechopkins

@thechopkins thechopkins commented Oct 25, 2023

Copy link
Copy Markdown

What this PR does / why we need it:
This updates the SparkSource data type to be properly comparable. The previous form defaults to using the __eq__ implementation from the DataSource class. This misses updates to additional properties on the SparkSource class (i.e. anything that feeds into the SparkOptions class).

This PR resolves this and adds tests to confirm the updated functionality. This means that FeatureStore.plan() and FeatureStore.apply() will properly pick up changes to SparkSources correctly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@thechopkins
thechopkins requested review from kevjumba and niklasvm as code owners October 25, 2023 22:26
@thechopkins
Update SparkSource to have proper comparable
c1e869e 
Signed-off-by: Calvin Hopkins <[email protected]>
@thechopkins
thechopkins force-pushed the master branch from 9852b6d to c1e869e Compare October 25, 2023 22:27
@thechopkins thechopkins changed the title fix: [Update SparkSource to have proper comparable that inspects SparkOptions] fix: pdate SparkSource to have proper comparable that inspects SparkOptions Oct 25, 2023
@thechopkins thechopkins changed the title fix: pdate SparkSource to have proper comparable that inspects SparkOptions fix: Update SparkSource to have proper comparable that inspects SparkOptions Oct 25, 2023
thechopkins added 2 commits October 25, 2023 15:41
@thechopkins
lint fixes
63a22d9 
Signed-off-by: Calvin Hopkins <[email protected]>
@thechopkins
force precommit
61f604a 
Signed-off-by: Calvin Hopkins <[email protected]>
rebstastic
rebstastic reviewed Oct 26, 2023
View reviewed changes
Comment thread sdk/python/feast/infra/offline_stores/contrib/spark_offline_store/spark_source.py
# Note: Python requires redefining hash in child classes that override __eq__
def __hash__(self):

return super().__hash__()

@rebstastic rebstastic Oct 26, 2023

Copy link
Copy Markdown
Contributor

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.

Should this also include self.spark_options?

Sorry, something went wrong.

Uh oh!

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

@thechopkins thechopkins Oct 26, 2023

Copy link
Copy Markdown
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.

It doesn't necessarily need to as the two pieces of logic are used for different things. Take a look at the redshift_source.py example for how it is implemented elsewhere.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@achals achals added ok-to-test lgtm labels Oct 31, 2023
@jeremyary jeremyary added lgtm and removed lgtm labels Mar 4, 2024
@franciscojavierarceo

franciscojavierarceo commented Mar 16, 2024

Copy link
Copy Markdown
Member

looks like linter is failing here. @thechopkins can you update?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tokoko tokoko mentioned this pull request Mar 26, 2024
fix: Add __eq__, __hash__ to SparkSource for correct comparison #4028
Merged
@jeremyary

jeremyary commented Mar 29, 2024

Copy link
Copy Markdown
Collaborator

closing out as this was addressed via the mentioned PR 4028. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jeremyary jeremyary closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@niklasvm niklasvm Awaiting requested review from niklasvm
@kevjumba kevjumba Awaiting requested review from kevjumba
1 more reviewer
@rebstastic rebstastic rebstastic left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

lgtm ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

5 participants

@thechopkins @franciscojavierarceo @jeremyary @rebstastic @achals
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 01402520 — 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.