-
Notifications
You must be signed in to change notification settings - Fork 138
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: add JSON type, bindparam support #1147
base: main
Are you sure you want to change the base?
Conversation
def bind_expression(self, bindvalue): | ||
# JSON query parameters have type STRING | ||
# This hook ensures that the rendered expression has type JSON | ||
return sqlalchemy.func.PARSE_JSON(bindvalue, type_=self) |
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.
note An alternative here would be to just preserve the STRING type and let BigQuery handle the cast, but this seems hard to reason about. It feels right to have the expression type in BigQuery match the expression type here in SQLAlchemy.
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.
Looking at [1] and pondering this - it wouldn't be that surprising if a user wanted to submit invalid JSON as a query parameter at some point. This could be supported by parameterizing this behavior in the type, i.e: JSON(strict=False)
or similar. Not worried about it right now, though.
json_serializer=None, | ||
json_deserializer=None, |
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.
note See https://docs.sqlalchemy.org/en/20/core/type_basics.html#sqlalchemy.types.JSON section "Customizing the JSON Serializer"
Extracted from #1146, this is a smaller set of changes that establish a JSON type.
Changes:
json_serializer
/json_deserializer
dialect kwargsOnce this is merged, users will be able to:
In future PRs, we can add support for:
Fixes #399 🦕