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

SQLAlchemy: Use JSON type adapter for implementing CrateDB's OBJECT #561

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

amotl
Copy link
Member

@amotl amotl commented Jul 5, 2023

About

Following up on crate/sqlalchemy-cratedb#93, by using the generalized parts of SQLAlchemy's infrastructure for implementing JSON types for a few databases, the CrateDB dialect will be able to leverage the same interfaces, like data casters, thus improving compatibility.

Example

sa.select(Entity).where(Entity.field['attribute'].as_integer() == 42)

Details

SQLAlchemy's sqltypes.JSON provides a facade for vendor-specific JSON types. Since it supports JSON SQL operations, it only works on backends that have an actual JSON type, which are currently PostgreSQL, MySQL, SQLite, and Microsoft SQL Server.

This patch starts leveraging the same infrastructure, thus bringing corresponding interfaces to the CrateDB dialect. The major difference is that it will not actually do any JSON marshalling, but propagate corresponding data structures 1:1, because within CrateDB's SQL, OBJECTs do not need to be serialized into JSON strings before transfer.

Resources

Comment on lines +143 to +144
# Designated name to refer to. `Object` is too ambiguous.
ObjectType = MutableDict.as_mutable(ObjectTypeImpl)
Copy link
Member Author

@amotl amotl Jul 5, 2023

Choose a reason for hiding this comment

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

SQLAlchemy also offers a MutableDict implementation. It looks like using that would resolve some other specializations of the CrateDB dialect, which are a bit anomalous, as we discovered on behalf of the discussion at sqlalchemy/sqlalchemy#5915.

However, it did not work out of the box. SQLAlchemy's MutableDict, or associated code paths, did not seem to support nested object access well. Maybe indeed this is one of the essential specializations the CrateDB dialect has to offer.

This topic will need to be re-visited. /cc GH-425

@amotl amotl marked this pull request as ready for review July 5, 2023 09:47
Comment on lines +135 to 139
class ObjectTypeImpl(sqltypes.UserDefinedType, sqltypes.JSON):

__visit_name__ = "OBJECT"

cache_ok = False
Copy link
Member Author

@amotl amotl Jul 5, 2023

Choose a reason for hiding this comment

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

We still need to use cache_ok = False, otherwise things go south on indexed dictionary access. So, it does not improve the situation wrt. GH-559. I will ask for advise on the upstream issue tracker about this detail.

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good, but please wait for the rest of the reviewers.

CHANGES.txt Outdated
@@ -15,6 +15,8 @@ Unreleased

- SQLAlchemy: Fix SQL statement caching for CrateDB's ``OBJECT`` type.

- SQLAlchemy: Refactor ``OBJECT`` type to use SQLAlchemy's JSON type machinery.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally prefer infrastructure or functionality over machinery.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Updated the changelog item.

Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

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

👍

Base automatically changed from amo/fix-object-caching to master July 5, 2023 22:09
amotl added 2 commits July 6, 2023 00:09
SQLAlchemy's `sqltypes.JSON` provides a facade for vendor-specific JSON
types. Since it supports JSON SQL operations, it only works on backends
that have an actual JSON type, which are currently PostgreSQL, MySQL,
SQLite, and Microsoft SQL Server.

This patch starts leveraging the same infrastructure, thus bringing
corresponding interfaces to the CrateDB dialect. The major difference
is that it will not actually do any JSON marshalling, but propagate
corresponding data structures 1:1, because within CrateDB's SQL,
`OBJECT`s do not need to be serialized into JSON strings before
transfer.
def get_col_spec(self):
return 'OBJECT'
# Backward-compatibility aliases.
_deprecated_Craty = ObjectType

Check notice

Code scanning / CodeQL

Unused global variable

The global variable '_deprecated_Craty' is not used.
return 'OBJECT'
# Backward-compatibility aliases.
_deprecated_Craty = ObjectType
_deprecated_Object = ObjectType

Check notice

Code scanning / CodeQL

Unused global variable

The global variable '_deprecated_Object' is not used.
@amotl amotl merged commit ce15b62 into master Jul 5, 2023
@amotl amotl deleted the amo/refactor-craty branch July 5, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants