-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for MySQL database #556
Conversation
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.
LGTM!
haystack/document_store/sql.py
Outdated
value = Column(String, index=True) | ||
document_id = Column(String, ForeignKey("document.id", ondelete="CASCADE"), nullable=False) | ||
name = Column(String(100), index=True) | ||
value = Column(String(100), index=True) |
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.
Title of the doc is stored as value of meta.name
key and there is possibility that it can breach 100 char limit. And if Text
is used then need to consider how Text filed is indexed. I feel high value for this would be good ie 1024 or 512.
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.
Well spotted @lalitpagaria! I switched the value
field to Text
.
haystack/document_store/sql.py
Outdated
value = Column(String, index=True) | ||
document_id = Column(String, ForeignKey("document.id", ondelete="CASCADE"), nullable=False) | ||
name = Column(String(100), index=True) | ||
value = Column(Text, index=True) |
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.
As per mysql docs -
BLOB and TEXT columns also can be indexed, but a prefix length must be given.
I think one integration test with mysql will show these issue if any.
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.
I think we could revert to a larger string field instead of text as you suggested earlier. I tested that the tables get created without any errors on a MySQL instance.
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.
Yes I agree with you.
This PR adds makes the
SQLDocumentStore
more generic to work with MySQL database.Resolves #543.