refactor(formats): turn TypeParser into a TypeMapper implementation for sqlglot
#6876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kszucs
commented
Aug 16, 2023
| from collections.abc import Mapping | ||
|
|
||
|
|
||
| class PostgresTypeParser(TypeParser): |
There was a problem hiding this comment.
Moved to base/sql/glot/datatypes.py for centralization, probably I should move it back.
1 task
kszucs
commented
Aug 17, 2023
cpcloud
requested changes
Aug 17, 2023
|
This PR is not complete yet, I'd like to add more roundtrip tests for certain backends using hypothesis. |
kszucs
force-pushed
the
sqlglot-type-mapper
branch
3 times, most recently
from
ee66638 to
f74e8fd
Compare
kszucs
force-pushed
the
sqlglot-type-mapper
branch
10 times, most recently
from
81e2f6e to
28349ff
Compare
|
@cpcloud could you please give it a preliminary review? I still need to update the cloud backends and somewhat improve the testing. |
cpcloud
reviewed
Aug 24, 2023
kszucs
force-pushed
the
sqlglot-type-mapper
branch
from
4874821 to
a712d34
Compare
|
@cpcloud the cloud backends are passing too, please take a look! Feel free to update the PR as needed, I won't be touching it for a couple of days. |
cpcloud
reviewed
Aug 25, 2023
cpcloud
reviewed
Aug 25, 2023
cpcloud
reviewed
Aug 25, 2023
| @@ -278,6 +277,7 @@ def test_rename_table(con, temp_table, temp_table_orig, new_schema): | |||
| @mark.notyet( | |||
| ["trino"], reason="trino doesn't support NOT NULL in its in-memory catalog" | |||
| ) | |||
| @mark.broken(["snowflake"], reason="snowflake shows not nullable column as nullable") | |||
There was a problem hiding this comment.
Is this broken upstream or is there a bug in the implementation of type parsing for snowflake?
There was a problem hiding this comment.
This is due to a weird behaviour of snowflake
>create temporary table _ibis_test_nullability (a number not null);
+----------------------------------------------------+
| status |
|----------------------------------------------------|
| Table _IBIS_TEST_NULLABILITY successfully created. |
+----------------------------------------------------+
1 Row(s) produced. Time Elapsed: 0.470s
>desc _ibis_test_nullability;
+------+--------------+--------+-------+---------+-------------+------------+-------+------------+---------+-------------+
| name | type | kind | null? | default | primary key | unique key | check | expression | comment | policy name |
|------+--------------+--------+-------+---------+-------------+------------+-------+------------+---------+-------------|
| A | NUMBER(38,0) | COLUMN | N | NULL | N | N | NULL | NULL | NULL | NULL |
+------+--------------+--------+-------+---------+-------------+------------+-------+------------+---------+-------------+
1 Row(s) produced. Time Elapsed: 0.212s
>select * from _ibis_test_nullability;
+---+
| A |
|---|
+---+
0 Row(s) produced. Time Elapsed: 0.224s
>DESC RESULT last_query_id();
+------+--------------+--------+-------+---------+-------------+------------+-------+------------+---------+-------------+
| name | type | kind | null? | default | primary key | unique key | check | expression | comment | policy name |
|------+--------------+--------+-------+---------+-------------+------------+-------+------------+---------+-------------|
| A | NUMBER(38,0) | COLUMN | Y | NULL | N | N | NULL | NULL | NULL | NULL |
+------+--------------+--------+-------+---------+-------------+------------+-------+------------+---------+-------------+
1 Row(s) produced. Time Elapsed: 0.222s
cpcloud
reviewed
Aug 25, 2023
| f"CREATE PRIVATE TEMPORARY TABLE {table} AS {query.strip(';')}" | ||
| ) | ||
| result = con.exec_driver_sql(f"DESCRIBE {table}").mappings().all() | ||
| con.exec_driver_sql(f"DROP TABLE {table}") |
There was a problem hiding this comment.
I wonder why there are a bunch of uncovered lines here.
cpcloud
reviewed
Aug 25, 2023
cpcloud
reviewed
Aug 25, 2023
cpcloud
reviewed
Aug 25, 2023
| "ipv6": dt.INET(nullable=default_nullable), | ||
| "object('json')": dt.JSON(nullable=default_nullable), | ||
| "array(null)": dt.Array(dt.null, nullable=default_nullable), | ||
| "array(nothing)": dt.Array(dt.null, nullable=default_nullable), |
There was a problem hiding this comment.
These types are case sensitive, why did you alter their case?
There was a problem hiding this comment.
From_string converts the input string to lowercase before looking up the types unknown to sqlglot
kszucs
force-pushed
the
sqlglot-type-mapper
branch
from
6a81a51 to
98f0f6a
Compare
|
@cpcloud could you please take another look? |
kszucs
force-pushed
the
sqlglot-type-mapper
branch
2 times, most recently
from
0dc48e0 to
e17ac89
Compare
kszucs
force-pushed
the
sqlglot-type-mapper
branch
from
4694739 to
76d4a84
Compare
|
Cloud backends look good: |
cpcloud
added
refactor
cpcloud
force-pushed
the
sqlglot-type-mapper
branch
from
76d4a84 to
7abda66
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend the
TypeMapperinterface withto_string()andfrom_string()classmethods, then implement theTypeParserto rather be an implementation ofTypeMapperforsqlglot. This means that the mappers are able to convert between ibis types and sqlglot datatype expressions back and forth. Hopefully this will turn useful once we are going to depend on sqlglot more heavily.Additionally support for parsing typestrings and compiling to typestrings (this was the original functionality of the previous type parser), the latter functionality is offloaded to sqlglot (with a couple of exceptions already handled in the original type parse).