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

Add Date in days conversion support #168

Merged
merged 2 commits into from
Mar 6, 2019

Conversation

wamsiv
Copy link
Contributor

@wamsiv wamsiv commented Feb 9, 2019

@andrewseidl Need to tag docker core build with latest commits in it, to enable the commented tests, they are failing on current build: v4.5.0dev-20190104-5aee66d0e5

fixes #167

@@ -131,7 +132,8 @@ def build_input_columnar(df, preserve_index=True,

if mapd_type in {'TIME', 'TIMESTAMP', 'DATE', 'BOOL'}:
# requires a cast to integer
data = thrift_cast(data, mapd_type)
print(col_types[colindex][1])
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need a print() statement here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove it! Thanks!

@randyzwitch
Copy link
Contributor

@wamsiv Since this issue started with OmniSci 4.4, why wouldn't we expect this functionality pass on any build?

@wamsiv wamsiv force-pushed the wam/date_in_days branch 2 times, most recently from d547dbe to 15f921b Compare February 9, 2019 17:49
@wamsiv
Copy link
Contributor Author

wamsiv commented Feb 9, 2019

We started exposing encoding to end points recently through a commit which is not available in this build. Without that there is not no way of knowing days/seconds type.

@randyzwitch
Copy link
Contributor

I guess what I'm asking is if the tests require a certain version of OmniSci, do we need to add some sort of OmniSci version logic into pymapd either in connect() or into the table loading code? It feels like we're breaking backwards compatibility unnecessarily, instead of fixing something that the user shouldn't need to worry about.

@wamsiv
Copy link
Contributor Author

wamsiv commented Feb 22, 2019

Looks like latest build container is failing at the start. And we are not breaking any compatibility, here I have added: https://github.com/omnisci/pymapd/pull/168/files#diff-51b57568ea5bfd539fbcf96d15ad4b18R86 another condition for date encoding in days. The legacy dates will still pass through date_to_seconds(data).

@wamsiv
Copy link
Contributor Author

wamsiv commented Feb 22, 2019

@randyzwitch Ok I think I understand what you are confused about here. So, now the new columns which are getting created have encoding in days. If you try to use load_table here on new dates it would expect the input in days, which my logic does here but problem is that we were not exposing the encoding of new DATE type before. heavyai/heavydb@99adcec now exposes the end point, which is not available in 4.4.2 hence when loading the data through load_table_columnar since we do not receive proper encoding for the type it assumes that encoding is NONE and proceeds to insert value in seconds (legacy way) rather than using new days logic.

@randyzwitch
Copy link
Contributor

Ok, so it's 4.4.2 that unintentionally broke the compatibility by not exposing the endpoint? If so, can't do anything about that now.

This is failing because the newest version for Docker is still 4.4.2. @andrewseidl, if you could ping us when 4.5 is available, then we can merge this PR and get a new version of pymapd out the door

@wamsiv
Copy link
Contributor Author

wamsiv commented Feb 22, 2019

v4.5.0 is available as the latest build and runs fine on local. Looks like this is a system issue, not with our container.

@randyzwitch randyzwitch closed this Mar 1, 2019
@randyzwitch randyzwitch reopened this Mar 1, 2019
@randyzwitch
Copy link
Contributor

LGTM, passes on my local machine. Needed to run docker run --ipc=host -d -p 9091:6274 mapd/core-os-cpu due to port remapping (per #163)

@randyzwitch randyzwitch merged commit b27f78f into heavyai:master Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dates handled improperly on load
2 participants