-
Notifications
You must be signed in to change notification settings - Fork 0
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
FTX-886-kusto: fix issue where python datetime is not handled correctly #1
base: 2.0.0+sm
Are you sure you want to change the base?
Conversation
return ", ".join(Cursor._escape(element) for element in value) | ||
elif isinstance(value, datetime): | ||
value = value.isoformat() | ||
return "'{}'".format(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like sqlalchemy by default replaces the T but I tested this with raw queries in the following formats and they all work with the server im connecting to right now.
"2020-01-01 01:01:01",
"2020-01-01 01:01",
"2020-01-01 01:01:01.000001",
"2020-01-01T01:01:01.000001",
"2020-01-01T01:01:01.000001",
"2020-01-01T01:01:01.000001+00:00",
"2020-01-01 01:01:01.000001+00:00",
"2020-01-01T01:01:01.000001+00:00Z",
"2020-01-01 01:01:01.000001+00:00Z"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change itself looks recent, my only concern is how we'll maintain this going forward and if we can upstream that in some fashion.
What's the original license for this, btw?
Aside from making a PR to the original repo, I just try to communicate with the original source code authors and mention issues I see. This is a newer repo so I can only🙏 that the author is somewhat active.
It looks like the author put up an apache license: https://github.com/dodopizza/sqlalchemy-kusto/blob/main/LICENSE |
I did not want to go with a hacky fix on FTX because that made things look weirder. |
Python datetime wasn't supported and I can only say the dialect is handling the parameter binds a bit differently. But it would take a lot more time for me to understand the sqlalchemy framework to handle this the proper way.
At the very least, I tested this and also saw how sqlalchemy was doing it so it should be fine.