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

Cast value to text to make filter work with uuid primary key columns #154

Closed
wants to merge 3 commits into from
Closed

Conversation

vesterbaek
Copy link

Check for uuid primary key and add typecast if required to support join between model and watson tables

@@ -274,7 +279,8 @@ def do_filter(self, engine_slug, queryset, search_text):
"watson_searchentry.search_tsv @@ to_tsquery('{search_config}', %s)".format(
search_config = self.search_config
),
"watson_searchentry.{ref_name} = {table_name}.{pk_name}".format(
"watson_searchentry.{ref_name}{ref_name_typecast} = {table_name}.{pk_name}".format(
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting. I wonder if we couldn't make it more generic by casting {table_name}.{pk_name} to string if it's not an integer primary key. That would presumably account more more fields than a UUID.

@vesterbaek
Copy link
Author

Yup, I actually did that in my first implementation. The title of this pull request is a hint about that :). I added explicit support for the uuid type for performance reasons. We could add a fallback to text cast after the uuid check

@etianen
Copy link
Owner

etianen commented Mar 2, 2016

How does the uuid conversion improve performance over the string
conversion? I'd have thought that both blow the use of indices out of the
water.

On Wed, 2 Mar 2016 at 14:36 Jeppe Vesterbæk notifications@github.com
wrote:

Yup, I actually did that in my first implementation. The title of this
pull request is a hint about that :). I added explicit support for the uuid
type for performance reasons. We could add a fallback to text cast after
the uuid check


Reply to this email directly or view it on GitHub
#154 (comment)
.

@vesterbaek
Copy link
Author

uuid is a native type with 128 bit length: http://www.postgresql.org/docs/9.1/static/datatype-uuid.html. Representing the uuid as text leads takes more storage (see eg. http://stackoverflow.com/questions/33836749/postgresql-using-uuid-vs-text-as-primary-key). In this case where one table has text and one has uuid, my knowledge of PostgreSQL is not good enough and I'm not sure if there is a performance gain by casting to uuid.

If we added a object_id_uuid column to watson_searchentry (like you have for int) we would get improvements in both required storage space and in performance.

@etianen
Copy link
Owner

etianen commented Mar 3, 2016

I think it's the sort of thing that could only be shown using benchmarks. I
suspect, however, that any performance difference would be negligible,
since the database won't be able to use indices either way. What's certain,
however, is that the code required for the "cast to string" approach is
much smaller, which is a definite win!

On Wed, 2 Mar 2016 at 21:46 Jeppe Vesterbæk notifications@github.com
wrote:

uuid is a native type with 128 bit length:
http://www.postgresql.org/docs/9.1/static/datatype-uuid.html.
Representing the uuid as text leads takes more storage (see eg.
http://stackoverflow.com/questions/33836749/postgresql-using-uuid-vs-text-as-primary-key).
In this case where one table has text and one has uuid, my knowledge of
PostgreSQL is not good enough and I'm not sure if there is a performance
gain by casting to uuid.


Reply to this email directly or view it on GitHub
#154 (comment)
.

@vesterbaek
Copy link
Author

Closing this one on favor of #169

@vesterbaek vesterbaek closed this Jul 4, 2016
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.

None yet

2 participants