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

Auto-convert unix 'time' field to SQL compatible Timestamp field. #3

Merged
merged 2 commits into from
Sep 25, 2014
Merged

Auto-convert unix 'time' field to SQL compatible Timestamp field. #3

merged 2 commits into from
Sep 25, 2014

Conversation

arohter
Copy link
Contributor

@arohter arohter commented Sep 24, 2014

We're importing mixpanel events into AWS Redshift, which unfortunately does not natively support unix timestamp format. This PR injects a new SQL Timestamp/DateTime format compatible field during export, which can be used directly when importing into a SQL database.

@erik
Copy link
Owner

erik commented Sep 24, 2014

Awesome, thank you! Definitely a good improvement to make.

Tests are breaking because this assumes that the "time" property is always specified. I think that this is a valid assumption to make, but I'm not totally sure after reading the docs... Any insight?

Could be safer to just check for nil before casting.

@erik
Copy link
Owner

erik commented Sep 24, 2014

Ah, found it in the HTTP spec

time: ... If this property is not included in your request, Mixpanel will use the time the event arrives at the server.

So that should be a fine assumption then. Could you add a new test that verifies the time outputs are correct?

@arohter
Copy link
Contributor Author

arohter commented Sep 24, 2014

I'm new to go, but I believe I added a decent nil and type check :)

erik added a commit that referenced this pull request Sep 25, 2014
Auto-convert unix 'time' field to SQL compatible Timestamp field.
@erik erik merged commit d83add5 into erik:master Sep 25, 2014
@erik
Copy link
Owner

erik commented Sep 25, 2014

Great! Thanks again!

@arohter arohter deleted the timestamp branch September 25, 2014 06:32
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.

None yet

2 participants