-
Notifications
You must be signed in to change notification settings - Fork 590
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
feat(exasol): add exasol backend #7303
Conversation
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.
Thanks for the PR! Glad to hear back from you!
Gave this a once over. Overall, this looks pretty good :)
|
To answer some of your questions:
We don't really have a documented bare minimum feature set for new backends. If I had to say what it was, I think it'd be:
Basically, you should be able to run the equivalent of
We don't have specific documentation for this. I can add some. I suspect you need to run |
|
Regarding getting the tests to pass, the least painful strategy is to insert at the top of every When a test that is marked as Indicating that you've successfully passed the test. You can then remove |
|
Hi @cpcloud, thanks for all the input and help, I'll pickup on it tomorrow. best |
|
@cpcloud I assume the final PR will be |
|
@Nicoretti We can clean up the commits once everything is green :) No need to worry about it for now. |
|
Hi @cpcloud Just did another catchup with your
|
|
@Nicoretti That looks right! The only thing I would change is that you should be able to merge steps 3 and 4 into |
|
@cpcloud sry had to force push. I had rebased the changes from the btw doing a "clean" lock, does not seem to work properly (even on "Clean" Lock
|
|
@Nicoretti I am going to rebase this PR and fix up the merge conflicts! |
|
@cpcloud thx for updating |
|
I can help with the |
Side Note(s):
best |
|
Hi @cpcloud, I am back on the PR. Sorry for the extra delay; I had to address a bug and some other issues in other projects I am maintaining. Since my catch-up yesterday, I have experienced two issues. I'm just wondering if you may have any idea regarding them.
I was wondering if this rings a bell or sounds like a problem you've already encountered in the past with other backends. Thanks for being so supportive all along. Best, |
|
Hey @Nicoretti -- @cpcloud is on a well-deserved vacation but will be back next week. You are right that those
Regarding test flakiness, that could be a bunch of things -- some of the failure modes might help diagnose. If I had to guess, some tests are leaving some global state around, and since we use One thing to try initially is to add |
|
Hi @gforsyth, Thanks for the timely update and feedback, appreciate it. The random test execution is a very good point, I didn't really had it on my radar yet. Best, |
|
I am fixing the merge conflicts! |
|
@Nicoretti Is there any chance Python 3.12 support is coming up and the sqlalchemy version that's supported by |
|
Generally speaking it should be fine to set sqlalchemy constraints to allow patch version bumps, they tend not to break things in patch releases, so instead of the |
|
I am running the test suite now and will help grind through the current failures. I'd like to avoid long running PRs if possible 😅 |
@cpcloud Generally, yes. I need to re-validate it against the test suite of that |
|
@Nicoretti It looks like your approach so far is to go through every test file and implement functionality until tests pass. I highly recommend a different approach that will get this merged upstream much faster: Mark everything that is currently failing as This also has the side benefit of user-facing release notes that show when particular functionality was added. |
Sounds good to me! |
| @@ -52,6 +52,8 @@ | |||
| # druid allows double quotes for identifiers, like postgres: | |||
| # https://druid.apache.org/docs/latest/querying/sql#identifiers-and-literals | |||
| "druid": "postgres", | |||
| # closest match see https://github.com/ibis-project/ibis/pull/7303#discussion_r1350223901 | |||
| "exa.websocket": "oracle", | |||
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.
Here you state that oracle is the closest matching sqlglot dialect, but all the sqlglot calls in exasol/__init__.py are using the postgres dialect. I think one of these needs to change -- should this be postgres here? Or should the dialect= kwargs be oracle?
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.
Unfortunately, I can't provide a link to my previous conversation with @cpcloud due to GitHub's limitation in displaying outdated comments during force rebasing.
To summarize our discussion: historically, @cpcloud generously assisted me by providing implementations using postgress for certain code segments. Later, we delved into identifying the closest SQL dialect supported by sqlglot. During this, I consulted our DB developers to determine the most similar dialect.
In short: Oracle appears to be the closest match, as per their insights. However, navigating dialects is complex, especially with Exasol, where support for specific syntax constructs varies across dialects. Hence, while there might not be an official closest match, the consensus from our DB developers leans toward Oracle.
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.
This looks pretty much ready to go, @Nicoretti ! Thanks for all of the work on this!
There's the one question about which sqlglot dialect to use that we should resolve before this gets merged.
I'd prefer if we didn't add so many strict=False tests, but sometimes that is just what needs to happen. Are there any hints as to the cause of the flakiness?
One issues which caused failing tests in serial and parallel execution is fixed and noted in ...
# Exasol implicitly opens the created schema, therefore we need to restore
# the previous context.
action = (
sa.text(f"OPEN SCHEMA {open_schema}")
if open_schema
else sa.text(f"CLOSE SCHEMA {name}")
)
con.exec_driver_sql(action)As of today, when executed serially, the tests should not be flaky. However, concerning parallel execution, there still seem to be one or more issues that need identification and addressing. |
|
Having rebased against, the latest master a backend test ( |
|
Just rebased, |
|
Bringing in the upstream changes from broke the oracle tests. I had to change the type inference related code for oracle in PR due to the change to I'll fix it back up. |
|
It looks like you also reverted https://github.com/ibis-project/ibis/compare/90aee1a05296526ee1a034327699e5bea0a3cb8b..fc7c9a6e46a2bc769df583d0f044c22146daee9b#diff-70e8f790a7cd8392a8b812c47a95be84f3835051e37a873536cefb3cb726d360L97 which is an important change to ensure that we don't break BigQuery due to Exasol's inability to create a table whose name starts with I'll add this back too. |
Thanks for your help! Heisen bugs ... had my fair share of those 😅.
Thanks, my bad. I assumed that I have accidentally overwritten changes from the |
| Install for Exasol: | ||
|
|
||
| ```{.bash} | ||
| conda install -c conda-forge ibis-exasol |
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.
@Nicoretti -- I will try to remember to add this to the conda-forge recipe when we do our next release, but you should feel free to ping me to remind me.
Also, if you can add sqlalchemy-exasol to conda-forge that will greatly improve the turnaround time on getting ibis-exasol added.
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.
@Nicoretti -- I will try to remember to add this to the
conda-forgerecipe when we do our next release, but you should feel free to ping me to remind me.
Removing it from the docs would be a possibility too. I just used another backend documentation as template, wasn't aware about all implications. But if we want to keep it, I can over to create a ticket as reminder.
Also, if you can add
sqlalchemy-exasoltoconda-forgethat will greatly improve the turnaround time on gettingibis-exasoladded.
Previously, sqlalchemy-exasol was available as a conda-forge package, but it was discontinued, possibly due to being outdated and underused. I need to determine if we should reintroduce it. Given that we now have a concrete use case with Ibis, it seems more likely that its reintroduction would be accepted.
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.
I can help maintain the feedstock (what's one more on a big pile 😁?) -- I think it's a good idea to have it available on conda-forge, if only so that exasol isn't the only backend that you can't install with conda/mamba.
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.
recipe PR here: conda-forge/staged-recipes#24777
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.
nevermind! here: https://github.com/conda-forge/sqlalchemy_exasol-feedstock
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.
the underscore threw me when I was looking before. looks like the conda-forge package is available and up-to-date, so that part is taken care of.
|
What's going on with the coverage warnings in |
@gforsyth |
|
@gforsyth This LGTM! Anything else left to do? |
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.
This is great @Nicoretti! Nothing blocking, just a few notes and we can tackle them in a follow-up, too.
| @@ -251,6 +251,7 @@ def tmpcon(alchemy_con): | |||
| ["mssql"], | |||
| reason="mssql supports support temporary tables through naming conventions", | |||
| ) | |||
| @mark.notimpl(["exasol"], reason="Exasol does not support temporary tables") | |||
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.
| @mark.notimpl(["exasol"], reason="Exasol does not support temporary tables") | |
| @mark.notyet(["exasol"], reason="Exasol does not support temporary tables") |
I think this should be a notyet or a never -- notimpl is a signal to us that Ibis hasn't added support for something that is currently possible.
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.
This currently would be a never because it is not supported by Exasol and I don't know if there are any plans to do so in the near future.
| None, | ||
| param( | ||
| [], | ||
| marks=pytest.mark.notimpl(["exasol"], raises=ExaQueryError, strict=False), |
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.
What's happening here? Is this test flaky?
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.
From what I can tell exasol-substet1-all is reliably failing, due to the lack of support of ? within the where clause:
pyexasol.exceptions.ExaQueryError:
(
message => where clause must be a boolean expression (Session: 1785240712777826304)
dsn => localhost:8563
user => sys
schema => EXASOL
session_id => 1785240712777826304
code => 42000
query =>
WITH t0 AS (SELECT t2."id" AS "id",
t2."bool_col" AS "bool_col",
t2."tinyint_col" AS "tinyint_col",
t2."small_int" AS "small_int",
t2."int_col" AS "int_col",
t2."bigint_col" AS "bigint_col",
t2."float_col" AS "float_col",
t2."double_col" AS "double_col",
t2."date_string_col" AS "date_string_col",
t2."string_col" AS "string_col",
t2."timestamp_col" AS "timestamp_col",
t2."year" AS "year",
t2."month" AS "month",
CASE WHEN (t2."int_col" = ?) THEN NULL ELSE t2."float_col" END AS col_1,
CASE WHEN (t2."int_col" = ?) THEN NULL ELSE t2."float_col" END AS col_2,
CASE WHEN (t2."int_col" = ? OR t2."int_col" = ?) THEN NULL ELSE t2."float_col" END AS col_3
FROM functional_alltypes AS t2)
SELECT t1.col_1, t1.col_2, t1.col_3
FROM (SELECT t0.col_1 AS col_1, t0.col_2 AS col_2, t0.col_3 AS col_3
FROM t0) AS t1
WHERE ? )That said, I don't know if this only is a limitation of the web socket based driver/protocol, or rather a general one.
@gforsyth I can look more into the details If you want me too.
Side Note:
SELECT Foo, Bar from T
WHERE Foo = ?;Should work though.
| @@ -2447,6 +2505,7 @@ def test_date_scalar_from_iso(con): | |||
| raises=sa.exc.DatabaseError, | |||
| reason="ORA-22849 type CLOB is not supported", | |||
| ) | |||
| @pytest.mark.notimpl(["exasol"], raises=AssertionError, strict=False) | |||
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.
is this also passing sometimes?
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.
I think you are maybe on to something here. Just removed the marker locally and ran the test multiple times:
- 10x test_temporal suit only
- Always Green
- 5x Full backend test suite
- Always green
Co-authored-by: Gil Forsyth <gforsyth@users.noreply.github.com>
|
Bombs away! Thanks for your work @Nicoretti, look forward to working with you more on this! |
Looking forward to keep working with you too! |
👋 Hi Ibis Team,
I reached out to you a few months ago (here),
but I wasn't able to follow up on this in a more timely manner.
This PR contains an initial draft of adding an
Exasolbackend toIbis.I am aware that the PR is not very polished yet and also lacks a significant number of features.
That said, with the PR, I also wanted to establish a means of communication to get some feedback
and align with you while pushing the feature of an Exasol backend forward.
Status Quo
just up exasoljust test exasol715 failed, 430 passed, 38 skipped, 26312 deselected, 38 xfailed, 33 errorsQuestions
Questions for/to the Ibis Project
nix-shellfailed after introducing new dependencies.Nix😅)?Other Questions
To-Do's
Edited:
It would be great if you could give me some initial feedback on the PR and have a look at
my questions. I appreciate your effort and look forward to hearing from you.
best
Nico