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

Interval columns #16

Merged
merged 7 commits into from
Jan 11, 2018
Merged

Interval columns #16

merged 7 commits into from
Jan 11, 2018

Conversation

kszucs
Copy link
Contributor

@kszucs kszucs commented Jan 9, 2018

Resolves #15

@xzkostyan
Copy link
Member

Thanks! I'll check PR later.

@kszucs
Copy link
Contributor Author

kszucs commented Jan 9, 2018

Sadly I couldn't setup clickhouse-client locally (OSX) but I guess it'll eventually pass on travis.

@coveralls
Copy link

coveralls commented Jan 9, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 95.777% when pulling d01d3a0 on kszucs:interval into 53ad772 on mymarilyn:master.

@kszucs
Copy link
Contributor Author

kszucs commented Jan 9, 2018

@xzkostyan 3.2 build is failing due to missing setuptools-scm, others are passing.

xzkostyan
xzkostyan reviewed Jan 11, 2018
View reviewed changes
tests/columns/test_interval.py Outdated
]
columns = ', '.join(['INTERVAL {} {}'.format(v, k)
for k, v in interval])
query = 'SELECT {} FROM system.numbers LIMIT 1'.format(columns)
Copy link
Member

Choose a reason for hiding this comment

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

You can use 'SELECT {}' as query template. Query SELECT INTERVAL 1 DAY works fine without FROM and LIMIT clauses.

xzkostyan
xzkostyan requested changes Jan 11, 2018
View reviewed changes
Copy link
Member

@xzkostyan xzkostyan left a comment

Choose a reason for hiding this comment

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

PR looks good, but please rebase to the latest master. I've dropped python 3.2 support, so all builds should be green now.

@kszucs
Copy link
Contributor Author

kszucs commented Jan 11, 2018

@xzkostyan rebase done

@coveralls
Copy link

coveralls commented Jan 11, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.04%) to 95.777% when pulling a26add5 on kszucs:interval into f7b99cc on mymarilyn:master.

@coveralls
Copy link

coveralls commented Jan 11, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 95.777% when pulling d01d3a0 on kszucs:interval into 53ad772 on mymarilyn:master.

@xzkostyan xzkostyan merged commit 248b84b into mymarilyn:master Jan 11, 2018
@xzkostyan
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xzkostyan xzkostyan xzkostyan 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
@kszucs @xzkostyan @coveralls