Changes to support DLT #636
Conversation
pj-spoelders
requested a deployment
to
manual-approval
GitHub Actions
Waiting
| # --- Date/time --- | ||
|
|
||
| # Some SQLAlchemy versions dispatch DateTime() to 'DATETIME' (upper-case) | ||
| def visit_DATETIME(self, type_, **kw): |
There was a problem hiding this comment.
Is it intentional that they aren't built referring to one another? For instance, we could have visit_DATETIME as it currently is defined and then have visit_datetime call self.visit_DATETIME and pass on the values.
There was a problem hiding this comment.
Fully agree that this needs to be cleaned up a bit and we reuse methods if they're available/work in our case.
| # --- Strings / Text --- | ||
|
|
||
| # SA String(length) -> VARCHAR(n); String() -> CLOB (Exasol requires length) | ||
| def visit_string(self, type_, **kw): |
There was a problem hiding this comment.
Similar question. Why not call visit_VARCHAR or visit_CLOB?
There was a problem hiding this comment.
Fully agree that this needs to be cleaned up a bit and we reuse methods if they're available/work in our case.
| return "DECIMAL(18,0)" | ||
|
|
||
| # Some code paths use DECIMAL directly rather than numeric | ||
| def visit_DECIMAL(self, type_, **kw): |
There was a problem hiding this comment.
Just curious as to how these were arrived to: Were the other visit_* functions checked or were these ones that caused difficulties for Exasol with dlt testing?
There was a problem hiding this comment.
Yes this is just enough to get DLT working right now, there's probably some/plenty of other cases that might still not work.
| try: | ||
| return super().do_execute(cursor, statement, parameters, context) | ||
| except Exception as e: | ||
| mapped = _PYEXA_TO_SA.get(e.__class__.__name__) |
There was a problem hiding this comment.
We might be able to simplify the exceptions here, as the Python Exception case allows for more than 1 to be checked, and we could extract that to a function if other usages needed to be covered.
However, it's possible you have something more in mind, as it looks like do_execute maybe doesn't cover all execution points.
There was a problem hiding this comment.
Let's have a look at how you'd do this this afternoon ..
| """EXASolution has no explicit indexes""" | ||
| return [] | ||
|
|
||
| def type_descriptor(self, typeobj): |
There was a problem hiding this comment.
Did you try putting the ExaTimestamp in the colspecs mapping?
https://github.com/exasol/sqlalchemy-exasol/blob/master/sqlalchemy_exasol/websocket.py#L91
Checklist
Note: If any of the items in the checklist are not relevant to your PR, leave the box unchecked.
For any Pull Request
Is the following correct:
When Changes Were Made
Did you:
When Preparing a Release
Have you: