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.
