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

refactor(exasol): port to sqlglot #8032

Merged
merged 28 commits into from
Jan 19, 2024

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jan 19, 2024

This PR ports exasol to sqlglot instead of sqlalchemy.

@cpcloud cpcloud added refactor Issues or PRs related to refactoring the codebase exasol Issues related to the exasol backend tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split` tes-required-for-merge Issues that must addressed before merging the-epic-split branch into main labels Jan 19, 2024
@cpcloud cpcloud requested a review from kszucs January 19, 2024 15:35
pyproject.toml Show resolved Hide resolved
@pytest.mark.broken(
["exasol"],
raises=ExaQueryError,
reason="database can't handle UTC timestamps in DataFrames",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be fixable if we get rid of import_from_pandas and do the inserts ourselves.

)
@pytest.mark.notimpl(["exasol"], raises=ExaQueryError)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit and unrelated to this pr is that the exception name ExaQueryError is not consistent with the rest of the error types having full backend name as the prefix, so I think it should be ExasolQueryError

name = _NAMES.get(con.name, "functional_alltypes")
quoted = getattr(getattr(con, "compiler", None), "quoted", True)
dialect = _IBIS_TO_SQLGLOT_DIALECT.get(con.name, con.name)
cols = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required due to backend specific quoting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This effectively makes identifiers case sensitive

@@ -214,7 +213,9 @@ def test_table_to_parquet(tmp_path, backend, awards_players):

df = pd.read_parquet(outparquet)

backend.assert_frame_equal(awards_players.to_pandas(), df)
backend.assert_frame_equal(
awards_players.to_pandas().fillna(pd.NA), df.fillna(pd.NA)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blaah.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phase 2 after the port?? 😂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, we should clean up the output coercions.

Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@kszucs kszucs merged commit 9a9c69d into ibis-project:the-epic-split Jan 19, 2024
47 of 48 checks passed
@cpcloud cpcloud deleted the tes-exasol branch January 29, 2024 11:14
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 4, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 5, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit that referenced this pull request Feb 5, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit that referenced this pull request Feb 6, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit that referenced this pull request Feb 6, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
cpcloud added a commit that referenced this pull request Feb 12, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
cpcloud added a commit that referenced this pull request Feb 12, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
kszucs pushed a commit that referenced this pull request Feb 12, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Feb 21, 2024
This PR ports exasol to sqlglot instead of sqlalchemy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exasol Issues related to the exasol backend refactor Issues or PRs related to refactoring the codebase tes-required-for-merge Issues that must addressed before merging the-epic-split branch into main tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants