Also, forking in filter_expr makes this work for both ds.filter and ds.where.
Shawn On Apr 12, 10:14 am, Shawn <[email protected]> wrote: > Hi Jeremy, > > Yes, I've used the ds = ds.filter approach as a workaround. However, > if there are a number of attributes being added conditionally, then > the repeated dataset filters are less efficient than defining a hash > of attributes and calling filter once. > > I believe your patch could be made more efficient. You're using > Array.include to look for empty condition expressions, when the empty? > method should be a lot faster: > > + if cond.responds_to?(:empty?) && cond.empty? > + clone > + else > > This would not match cond == [[]], but I don't know that filter should > honor that condition expression. [], {}, and '' would all be empty > and result in a clone. > > A more universally applicable way to implement this would be to fork > inside the filter_expr method: > > if expr?(:empty?) && expr.empty? > return BOOL_TRUE # not sure how you get the true value for the > dataset's database, but insert it here > end > > Then a fork would not be necessary inside Dataset.filter: > > cond = filter_expr(cond, &block) #=> BOOL_TRUE > cond = SQL::BooleanExpression.new(:AND, @opts[clause], cond) if > @opts[clause] > clone(clause => cond) #=> where TRUE (1, 't', etc.) > > Shawn > > On Apr 10, 7:16 am, Jeremy Evans <[email protected]> wrote: > > > > > On Apr 9, 1:57 pm, John Firebaugh <[email protected]> wrote: > > > > I encounter this situation when I have an external datastructure and I > > > need > > > to convert it to a filter hash, via inject({}) {...} or similar. With the > > > current API, I need to create an extra branch in my code to handle the > > > empty > > > case. > > > Are these cases where you wouldn't have access to the dataset? > > Because anything possible with a filter hash is going to be possible > > with individual filter calls: > > > ds = ds.filter(data.inject({}){|d, (k,v)| d[k] = v; d}) > > # vs. > > ds = data.inject(ds){|d, (k,v)| d.filter(k=>v)} > > > > I consider it a code smell when an API does not have a sensible behavior > > > for > > > obvious boundary conditions in its input. This is of particular concern > > > for > > > functions like filter that are commonly used in situations where the > > > arguments are not a static condition but built dynamically from runtime > > > input. > > > Agreed. Like I said, I'm certainly willing to consider this patch. > > The implementation isn't difficult and the performance hit is modest. > > Simply because I think there are generally better ways doesn't mean I > > think Sequel should violate the POLS. > > > BTW, how do you think this should be handled?: > > > ds.filter(nil) > > > That currently raises an error. I think it should probably use WHERE > > NULL (there just isn't a when clause for nil), but I could see some > > people thinking filter(nil) should be the same as filter(). > > > Jeremy -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/sequel-talk?hl=en.
