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.

Sign up for GitHub

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

Jump to bottom

[FSTORE-1181] Helper columns should return all columns if they have different names across feature groups #1203

Merged
merged 11 commits into from
Feb 16, 2024
Merged

[FSTORE-1181] Helper columns should return all columns if they have different names across feature groups #1203

merged 11 commits into from
Feb 16, 2024

Conversation

davitbzh
Copy link
Contributor

This PR adds/fixes/changes...

  • please summarize your changes to the code
  • and make sure to include all changes to user-facing APIs

JIRA Issue: -

Priority for Review: -

Related PRs: -

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Tests on VM

Checklist For The Assigned Reviewer:

- [ ] Checked if merge conflicts with master exist
- [ ] Checked if stylechecks for Java and Python pass
- [ ] Checked if all docstrings were added and/or updated appropriately
- [ ] Ran spellcheck on docstring
- [ ] Checked if guides & concepts need to be updated
- [ ] Checked if naming conventions for parameters and variables were followed
- [ ] Checked if private methods are properly declared and used
- [ ] Checked if hard-to-understand areas of code are commented
- [ ] Checked if tests are effective
- [ ] Built and deployed changes on dev VM and tested manually
- [x] (Checked if all type annotations were added and/or updated appropriately)

@davitbzh davitbzh requested a review from kennethmhc January 23, 2024 17:04
kennethmhc
kennethmhc reviewed Jan 25, 2024
View reviewed changes
python/hsfs/core/feature_view_engine.py Outdated
if _join.query._left_feature_group.event_time:
fv_events.update(
[
_join.query._left_feature_group.event_time
Copy link
Contributor

Choose a reason for hiding this comment

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

should throw exception here if there are ambiguous event time column? and not wait until the query get executed.
same for primary key.

@davitbzh davitbzh requested a review from kennethmhc January 29, 2024 18:18
kennethmhc
kennethmhc reviewed Jan 30, 2024
View reviewed changes
bigquery.json Outdated
@@ -0,0 +1 @@
{"type": "service_account", "project_id": "test"}
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant?

kennethmhc
kennethmhc reviewed Jan 30, 2024
View reviewed changes
python/hsfs/core/feature_view_engine.py Outdated

def _check_if_exists(self, f_name, f_set):
if f_name in f_set:
raise ValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

FeatureStoreException instead?

@davitbzh davitbzh requested a review from kennethmhc January 30, 2024 19:57
kennethmhc
kennethmhc approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@kennethmhc kennethmhc left a comment

Choose a reason for hiding this comment

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

LGTM！

@davitbzh davitbzh merged commit 3283198 into logicalclocks:master Feb 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennethmhc kennethmhc kennethmhc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davitbzh @kennethmhc