Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

PathCreator Bloom filters #51

Merged
merged 1 commit into from Sep 23, 2011

Conversation

Projects
None yet
3 participants
Contributor

ncreep commented Sep 22, 2011

An initial attempt at adding Bloom filters to selection with PathCreator.

Not sure that this is the best approach, as now it is not evident from the Selector trait that you can participate in optimizations on Elem types.

@ncreep ncreep Applying partial Bloom filter optimization to PathCreator.
- Using Bloom filter with the "directChildren" path.
- Removing the "elementName" from Selector and replacing it with a
separate class.

Basically, decoupling the Group class from the Selector class and
delegating the Bloom filter optimization to PathCreator.
daffb10
Contributor

josharnold52 commented Sep 23, 2011

Why the change to Selector? I think you could make dispatchSelector work with the original version. Restricting the filter optimization to Selectors that return Elements seems unnecessary.

I do like the idea of adding a matches(String): Boolean method to Group that provides direct access to the filter.

Contributor

ncreep commented Sep 23, 2011

As I see it the Selector trait should be as general as possible, coupling it with a specific implementation/optimization kind of looks wrong to me.

So if some mechanism wants to optimize, the responsibility for it is on its side, not the Selector's, which should know nothing about it.

Owner

djspiewak commented Sep 23, 2011

Actually, I think this is fairly nice. The optimization was nearly hard-coded for element section anyway, and those semantics were spread around somewhat unnecessarily. This change localizes the tangling quite a bit and makes it possible for us to optimize other sorts of selectors later simply by adding a case statement. +1

The one change I'm going to make is ElemSelector should be private[antixml] and the implicit conversion should return an instance of Selector[Elem] to avoid leaking implementation details.

@djspiewak djspiewak merged commit daffb10 into djspiewak:zipper-replacement Sep 23, 2011

Contributor

josharnold52 commented Sep 23, 2011

Agreed that the optimization is really only useful for Element selection. But why should that restrict what the selector can return? Couldn't you just as well define ElemSelector as something like:

/** A selector that selects an element by name. */
class ElemSelector[A](val elementName: String, mapping: (Elem) => A) extends Selector[A] {
  // not using a case class to allow inheritance
  private val pf: PartialFunction[Node, Elem] = {
    case e @ Elem(_, `elementName`, _, _, _) => mapping(e)
  }

  def apply(node: Node) = pf(node)
  def isDefinedAt(node: Node) = pf isDefinedAt node
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment