CONTAINSSTRING should move to storm.expr

Bug #387840 reported by Stuart Bishop on 2009-06-16
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Storm
Wishlist
James Henstridge

Bug Description

storm.sqlobject.CONTAINSSTRING is useful, and there is no comparable implementation in storm core.

I'd like to propose that the storm.sqlobject.CONTAINSSTRING implementation is moved to storm.expr, and a case_sensitive flag added similar to the existing Like.

storm.sqlobject.CONTAINSSTRING would become a stub.

The new expression should be exposed in storm.locals.

This could be a new class, perhaps named Contains, or storm.expr.In could be overloaded to handle the case where both arguments are strings.

Related branches

Stuart Bishop (stub) on 2009-06-16
Changed in storm:
importance: Undecided → Wishlist
James Henstridge (jamesh) wrote :

I think it might be useful to add startswith(), endswith() and containsstring() methods to Comparable, implemented with LIKE similar to CONTAINSSTRING(). That would allow more natural usage (e.g. store.find(Table, Table.column.startswith("foo"))).

If Comparable.__contains__() could work for "substring in column" type expressions, then that might be even better (although wouldn't allow for a case_sensitive optional argument).

On Thu, Jun 18, 2009 at 10:38 PM, James
Henstridge<email address hidden> wrote:
> I think it might be useful to add startswith(), endswith() and
> containsstring() methods to Comparable, implemented with LIKE similar to
> CONTAINSSTRING().  That would allow more natural usage (e.g.
> store.find(Table, Table.column.startswith("foo"))).
>
> If Comparable.__contains__() could work for "substring in column" type
> expressions, then that might be even better (although wouldn't allow for
> a case_sensitive optional argument).

store.find(Table, Table.column.lower.startswith(somestring.lower()))
would be the case insensitive version, wouldn't it?

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

James Henstridge (jamesh) wrote :

I guess we could go with that. I guess the difference would be in the generated SQL: "lower(Table.column) LIKE 'foo%'" vs. "Table.column ILIKE 'foo%'". You probably know more than me about how these two compare performance wise.

Stuart Bishop (stub) wrote :

On Fri, Jun 19, 2009 at 6:57 PM, James Henstridge<email address hidden> wrote:
> I guess we could go with that.  I guess the difference would be in the
> generated SQL: "lower(Table.column) LIKE 'foo%'" vs. "Table.column ILIKE
> 'foo%'".  You probably know more than me about how these two compare
> performance wise.

For PostgreSQL, they generate identical plans and both make use of an index on lower(Table.column).

Mixing LIKE and lower() (or upper) is the SQL standard way of doing case insensitive searches. ILIKE is a PostgreSQL extension.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

James Henstridge (jamesh) wrote :

Attached is a branch that adds Comparable.startswith() and endswith().

It doesn't yet have an equivalent to sqlobject.CONTAINSSTRING (not sure if it should use __contains__() or a normal method), but it'd be trivial to add.

Does this look like it would cover the use cases you are after? (assuming a "contains"-style method is added to complete it).

Stuart Bishop (stub) wrote :

Launchpad needs to be able to do one of:

    store.find(Foo, 'abc' in Foo.bar.lower())
    store.find(Foo, In('abc', Foo.bar.lower()))
    store.find(Foo, Foo.bar.lower().contains('abc')

startswith() will be useful in a few locations too.

James Henstridge (jamesh) wrote :

Right. I realised that you needed the "contains" functionality (however it is exposed). I was wondering if exposing it on Comparable like I've done in the branch would be enough, which it seems would be the case for the examples you've given (columns are Comparable instances, as is the result of column.lower()).

Changed in storm:
assignee: nobody → James Henstridge (jamesh)
milestone: none → 0.16
status: New → In Progress
James Henstridge (jamesh) wrote :

Fix merged to trunk as r338.

Changed in storm:
status: In Progress → Fix Committed
Jamu Kakar (jkakar) on 2009-11-29
Changed in storm:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers