Skip to content

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

Merged
feast-ci-bot merged 4 commits into
feast-dev:masterfrom
achals:achal/timestamp-infer
Jun 2, 2021
Merged

Update type conversion from pandas to timestamp to support various the timestamp types#1603
feast-ci-bot merged 4 commits into
feast-dev:masterfrom
achals:achal/timestamp-infer

Conversation

@achals

@achals achals commented Jun 1, 2021

Copy link
Copy Markdown
Member

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

codecov-commenter commented Jun 1, 2021

Copy link
Copy Markdown

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.

Comment thread sdk/python/feast/type_map.py Outdated

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

Details 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

woop commented Jun 2, 2021

Copy link
Copy Markdown
Member

/lgtm

achals added 4 commits June 1, 2021 23:43
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>
@achals
achals force-pushed the achal/timestamp-infer branch from fdf1708 to 1acd139 Compare June 2, 2021 06:43
@feast-ci-bot

Copy link
Copy Markdown
Collaborator

New changes are detected. LGTM label has been removed.

@feast-ci-bot feast-ci-bot removed the lgtm label Jun 2, 2021
@achals

achals commented Jun 2, 2021

Copy link
Copy Markdown
Member Author

/lgtm

@feast-ci-bot

Copy link
Copy Markdown
Collaborator

@achals: you cannot LGTM your own PR.

Details

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