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.

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

Update type conversion from pandas to timestamp to support various the timestamp types #1603

Merged
merged 4 commits into from
Jun 2, 2021

Conversation

achals
Copy link
Member

@achals achals commented Jun 1, 2021

What this PR does / why we need it:
Currently, Inferring features fails on columns of type timestamp[ms, tz=UTC] or other tz. This diff attempts to fix using a regex instead of enumerating all the different possibilities.

An option is to use the pandas type instead of the string representation, but this is probably good enough.
https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.Timestamp.html

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Fix Inferring features columns of pandas timestamp types.

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2021

Codecov Report

Merging #1603 (1acd139) into master (87989b0) will decrease coverage by 0.04%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1603      +/-   ##
==========================================
- Coverage   83.61%   83.57%   -0.05%     
==========================================
  Files          65       65              
  Lines        5761     5771      +10     
==========================================
+ Hits         4817     4823       +6     
- Misses        944      948       +4     
Flag Coverage Δ
integrationtests 83.48% <80.00%> (-0.05%) ⬇️
unittests 77.34% <20.00%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
sdk/python/feast/data_source.py 78.77% <ø> (-0.07%) ⬇️
sdk/python/feast/type_map.py 42.65% <80.00%> (+0.93%) ⬆️
sdk/python/feast/telemetry.py 61.53% <0.00%> (-1.09%) ⬇️
sdk/python/feast/repo_operations.py 32.00% <0.00%> (ø)
sdk/python/feast/infra/offline_stores/bigquery.py 91.03% <0.00%> (ø)
sdk/python/feast/__init__.py 88.23% <0.00%> (+1.56%) ⬆️

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 87989b0...1acd139. Read the comment docs.

@@ -438,8 +439,14 @@ def pa_to_value_type(pa_type: object):


def pa_to_feast_value_type(value: Union[pa.lib.ChunkedArray, str]) -> ValueType:
value_type = (
value.type.__str__() if isinstance(value, pa.lib.ChunkedArray) else value
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind explaining this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is moved from line 459 - basically this converts the type of a pandas column into a string representation, if the stringified version has not already been passed in.

@achals achals requested a review from woop June 1, 2021 21:26
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, woop

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop
Copy link
Member

woop commented Jun 2, 2021

/lgtm

Signed-off-by: Achal Shah <achals@gmail.com>
…e specifications

Signed-off-by: Achal Shah <achals@gmail.com>
Signed-off-by: Achal Shah <achals@gmail.com>
Signed-off-by: Achal Shah <achals@gmail.com>
@feast-ci-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@feast-ci-bot feast-ci-bot removed the lgtm label Jun 2, 2021
@achals
Copy link
Member Author

achals commented Jun 2, 2021

/lgtm

@feast-ci-bot
Copy link
Collaborator

@achals: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@achals achals added the lgtm label Jun 2, 2021
@feast-ci-bot feast-ci-bot merged commit f29cb8c into feast-dev:master Jun 2, 2021
@achals achals deleted the achal/timestamp-infer branch June 2, 2021 07:00
woop pushed a commit that referenced this pull request Jun 7, 2021
…e timestamp types (#1603)

* Fix precommit to run black on commit

Signed-off-by: Achal Shah <achals@gmail.com>

* Update type conversion from pandas to timestamp to support various the specifications

Signed-off-by: Achal Shah <achals@gmail.com>

* Undo precommit changs

Signed-off-by: Achal Shah <achals@gmail.com>

* make format

Signed-off-by: Achal Shah <achals@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants