Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
88f526f to
9805faf
Compare
9805faf to
c1c4113
Compare
Unknown types reflect as `UnresolvedType` rather than `NullType`, which the issue proposed, so the error carries the CrateDB type name.
c1c4113 to
18d8ade
Compare
bgunebakan
left a comment
There was a problem hiding this comment.
Thank you! I've reviewed that with NUMERIC PR because there is some places that touches both implementation. I've added some comments.
| resolved = self._lookup_type(type_) | ||
| if resolved is None: | ||
| # Debug level: reflecting `pg_catalog` alone leaves twenty-odd columns unresolved. | ||
| log.debug("Unable to resolve CrateDB type: %s", type_) |
| "numeric": sqltypes.NUMERIC, | ||
| "float_vector": FloatVector, | ||
| "geo_point": Geopoint, | ||
| "geo_shape": Geoshape, |
There was a problem hiding this comment.
Issue ticket also mentions ip and character, they are also unmapped with geo fields. Could you add them to map for covering all types mentioned on ticket?
| if not type_.endswith(ARRAY_SUFFIX): | ||
| return None | ||
| element_name = type_[: -len(ARRAY_SUFFIX)] | ||
| # SQLAlchemy's `ARRAY` cannot hold an array. |
There was a problem hiding this comment.
The comment says "SQLAlchemy's ARRAY cannot hold an array", but it can. What is the reason of the comment?
| # `pg_catalog` reports `regproc` for function references, which no table can declare. | ||
| UNRESOLVABLE = "regproc" |
There was a problem hiding this comment.
regproc drives five parametrised cases, so if it's ever mapped they quietly stop testing what they say. A name the server can never report would be better
| # `pg_catalog` reports `regproc` for function references, which no table can declare. | |
| UNRESOLVABLE = "regproc" | |
| UNRESOLVABLE = "unresolvable_cratedb_type" |
| for record in caplog.records | ||
| if record.name == "sqlalchemy_cratedb.dialect" | ||
| ] | ||
| assert messages == ["Unable to resolve CrateDB type: {0}".format(data_type)] |
There was a problem hiding this comment.
Any future debug line on this logger would breaks the test.
| assert messages == ["Unable to resolve CrateDB type: {0}".format(data_type)] | |
| assert "Unable to resolve CrateDB type: {0}".format(data_type) in messages |
| serializes them as strings, storing every digit instead of rounding to float | ||
| - Types: Added `numeric` and `numeric_array` to the reflected type map, where | ||
| they previously resolved to an abstract type that could not be compiled | ||
| - Reflection: Fixed columns of a CrateDB type the dialect cannot represent |
There was a problem hiding this comment.
Reflection: is a new label, the file has used Types:, Compiler: and Dialect:. I'd keep same labels.
Reflecting a column whose CrateDB type the dialect doesn't know gave it the abstract
UserDefinedType. Reading worked, but compiling DDL from the reflected table crashed without naming the column or the type:sys.summitsreproduces it:coordinatesis ageo_point, andGeopointwas never registered in the type map.Closes #295.
Stacked on #300 (
flo/292-numeric-type), since both touchTYPES_MAPand the type compiler, and thenumeric_arrayderivation here needs thenumericentry #300 adds.Fix
Unknown types reflect as
UnresolvedType, which carries the CrateDB type name. The column still reads, and compiling it fails with that name:The type is exported, so callers can check
isinstance(column.type, UnresolvedType)and skip the column.NullType, which the issue suggests, would report "did you forget to specify a type" and name nothing.geo_pointandgeo_shapemap to the existingGeopointandGeoshape.Array types are derived from their element (
<element>_array) instead of a fixed list of sixteen, so e.g.timestamp without time zone_arrayresolves too.object_arraykeeps its entry forObjectArray. Arrays of arrays and arrays of unresolved types stay unresolved.Out of scope: reflection still drops type parameters, so a reflected
VARCHAR(10)comes back unbounded.Tests
tests/reflection_test.py, new: the array form of every mapped type, the refusal for unresolvable scalars, elements and nested arrays, and a live reflection ofsys.summits.