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

FTX-886-kusto: fix issue where python datetime is not handled correctly #1

Open
wants to merge 1 commit into
base: 2.0.0+sm
Choose a base branch
from

Conversation

deeTEEcee
Copy link

@deeTEEcee deeTEEcee commented Sep 28, 2022

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.

return ", ".join(Cursor._escape(element) for element in value)
elif isinstance(value, datetime):
value = value.isoformat()
return "'{}'".format(value)
Copy link
Author

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"

Copy link

@clavoie-sightmachine clavoie-sightmachine left a 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?

@deeTEEcee
Copy link
Author

my only concern is how we'll maintain this going forward

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.

What's the original license for this, btw?

It looks like the author put up an apache license: https://github.com/dodopizza/sqlalchemy-kusto/blob/main/LICENSE

@deeTEEcee
Copy link
Author

I did not want to go with a hacky fix on FTX because that made things look weirder.

@deeTEEcee deeTEEcee requested review from clavoie-sightmachine and removed request for bremac February 17, 2023 19:29
@deeTEEcee deeTEEcee removed the request for review from clavoie-sightmachine February 17, 2023 19:30
@deeTEEcee deeTEEcee requested review from ekauzlarich and removed request for ekauzlarich February 17, 2023 19:30
@deeTEEcee deeTEEcee changed the base branch from 2.0.0+sm1 to 2.0.0+sm February 17, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants