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

FEAT: OmniSciDB - Add support to week extraction #2171

Merged
merged 1 commit into from
May 29, 2020
Merged

FEAT: OmniSciDB - Add support to week extraction #2171

merged 1 commit into from
May 29, 2020

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented Apr 1, 2020 •
edited
Loading

OmniSciDB - Add support to week extraction

ref: #1499

@xmnlab xmnlab changed the title Omniscidb add support to week extraction FEAT: OmniSciDB - Add support to week extraction Apr 1, 2020
@xmnlab xmnlab marked this pull request as ready for review April 1, 2020 22:46
@xmnlab xmnlab requested a review from jreback April 1, 2020 22:46
@xmnlab
Copy link
Contributor Author

xmnlab commented Apr 1, 2020

this PR is ready for review! thanks!

@xmnlab xmnlab force-pushed the omniscidb-add-support-to-week-extraction branch from 59cd0ee to 30ffbb7 Compare April 5, 2020 01:38
@xmnlab xmnlab force-pushed the omniscidb-add-support-to-week-extraction branch 2 times, most recently from adb162a to 7c61f9b Compare May 26, 2020 19:51
@jreback jreback added this to the Next Bugfix Release milestone May 26, 2020
jreback
jreback suggested changes May 26, 2020
View reviewed changes
ibis/omniscidb/operations.py
@@ -286,6 +289,30 @@ def extract_field_formatter(translator, expr, sql_attr=sql_attr):
return extract_field_formatter


def _extract_field_dow_name(sql_attr):
def extract_field_formatter(translator, expr, sql_attr=sql_attr):
Copy link
Contributor

Choose a reason for hiding this comment

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

did you enable tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as it is not unsupported anymore the current tests should check it https://github.com/ibis-project/ibis/blob/master/ibis/tests/all/test_temporal.py#L361-L397

@xmnlab xmnlab force-pushed the omniscidb-add-support-to-week-extraction branch from 7c61f9b to 29fc58a Compare May 28, 2020 15:56
@jreback
Copy link
Contributor

jreback commented May 29, 2020

lgtm.

@datapythonista see if you can merge here.

@datapythonista
Copy link
Contributor

No, same messages as in the screenshot I shared:

Merging is blocked
The base branch restricts merging to authorized users. Learn more about protected branches.

You’re not authorized to merge this pull request.

@jreback
Copy link
Contributor

jreback commented May 29, 2020

try again @datapythonista
confusingly github has a separate list for branch protection (which i think overrides the 'write' team access).

@datapythonista datapythonista merged commit 9fd587f into ibis-project:master May 29, 2020
@datapythonista
Copy link
Contributor

Yep, all good now, thanks @jreback

I was a bit confused, because I'm not in this list: https://github.com/orgs/ibis-project/people

But apparently is not needed.

Thanks @xmnlab for the PR

@jreback
Copy link
Contributor

jreback commented May 29, 2020

yeah i am not sure the teams are setup correctly, will fix at some point.

@xmnlab xmnlab deleted the omniscidb-add-support-to-week-extraction branch June 3, 2020 20:20
@cpcloud cpcloud mentioned this pull request Jun 13, 2020
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback 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.
3 participants
@xmnlab @jreback @datapythonista