Jackrabbit Content Repository
  1. Jackrabbit Content Repository
  2. JCR-3047

OperandEvaluator should be able to handle Nodes as well, not just Rows

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3
    • Component/s: jackrabbit-jcr-commons
    • Labels:
      None

      Description

      OperandEvaluator is used to evaluate Operands values against given Rows, and in an effort to improve the sorting part of SQL2 (JCR-2959), I need it to handle plain Nodes as well.

      This is a small change, as the OperandEvaluator already extracts the Node info from the Row, so there is no obvious reason no to expose the Node operations directly.

        Issue Links

          Activity

          Hide
          Alex Parvulescu added a comment -

          I removed the 'getAffectedPropertyName' method in revision #1157200

          Show
          Alex Parvulescu added a comment - I removed the 'getAffectedPropertyName' method in revision #1157200
          Hide
          Alex Parvulescu added a comment -

          you are right!

          so, here goes my quick fix: you remove getAffectedPropertyName, and use operand.toString as a lucene field name.
          In your example that would be: LENGTH(nt:base.p1)

          By using this custom name, you then define a custom lucene index on this field, where you can safely use evaluator.getValues(o1.getOperand(), node), which will work.

          This way we can get rid of that persky getAffectedPropertyName, and use a more reliable source, the operand itself.

          Show
          Alex Parvulescu added a comment - you are right! so, here goes my quick fix: you remove getAffectedPropertyName, and use operand.toString as a lucene field name. In your example that would be: LENGTH(nt:base.p1) By using this custom name, you then define a custom lucene index on this field, where you can safely use evaluator.getValues(o1.getOperand(), node), which will work. This way we can get rid of that persky getAffectedPropertyName, and use a more reliable source, the operand itself.
          Hide
          Jukka Zitting added a comment -

          The mapping you suggest doesn't work. Consider "ORDER BY LENGTH(p1), p1" which is supposed to order results first by the length and then by the contents of a property. That's probably not a very likely scenario, but ideally the implementation should be able to handle also cases like that.

          More generally, you'll lose any performance benefits as soon as the custom Lucene sort field tries to access the full node instance. More about this in JCR-2959.

          Show
          Jukka Zitting added a comment - The mapping you suggest doesn't work. Consider "ORDER BY LENGTH(p1), p1" which is supposed to order results first by the length and then by the contents of a property. That's probably not a very likely scenario, but ideally the implementation should be able to handle also cases like that. More generally, you'll lose any performance benefits as soon as the custom Lucene sort field tries to access the full node instance. More about this in JCR-2959 .
          Hide
          Alex Parvulescu added a comment -

          Yes I agree

          To map Orderings to properties, you can use getAffectedPropertyName, otherwise there is no way of knowing from a list of Orderings which goes where.
          So you can go from (o1, o2) to a map p1 -> o1, p2 -> o2

          After having this info, you can create a custom sort field in lucene (one for p1 and another one for p2) that can apply via OperandEvaluator all the above mentioned functions, like v1 = evaluator.getValues(o1.getOperand(), node), then sort on v1, and so on

          But this sorting related talk should happen on the other issue, JCR-2959

          Show
          Alex Parvulescu added a comment - Yes I agree To map Orderings to properties, you can use getAffectedPropertyName, otherwise there is no way of knowing from a list of Orderings which goes where. So you can go from (o1, o2) to a map p1 -> o1, p2 -> o2 After having this info, you can create a custom sort field in lucene (one for p1 and another one for p2) that can apply via OperandEvaluator all the above mentioned functions, like v1 = evaluator.getValues(o1.getOperand(), node), then sort on v1, and so on But this sorting related talk should happen on the other issue, JCR-2959
          Hide
          Jukka Zitting added a comment -

          To properly implement the mapping from an Ordering to a Lucene SortField you'd need to consider also other information than just the property name. For example "ORDER BY UPPER(pname)" or "ORDER BY LENGTH(pname)" must be handled differently from "ORDER BY pname", and an ordering like "ORDER BY b.pname" can't be used if you're constructing the Lucene query for selector a. Thus I don't think an isolated method like getAffectedPropertyName() really serves a useful purpose.

          Show
          Jukka Zitting added a comment - To properly implement the mapping from an Ordering to a Lucene SortField you'd need to consider also other information than just the property name. For example "ORDER BY UPPER(pname)" or "ORDER BY LENGTH(pname)" must be handled differently from "ORDER BY pname", and an ordering like "ORDER BY b.pname" can't be used if you're constructing the Lucene query for selector a. Thus I don't think an isolated method like getAffectedPropertyName() really serves a useful purpose.
          Hide
          Alex Parvulescu added a comment -

          Actually in the context of Orderings, the property thing works ok.

          I tested with a prefix ('order by a.[pname]'), and without one ('order by [pname]') and it seems to be able to identify the property.
          I guess it is a bit confusing that the order info (Ordering) takes an Operand, as I haven't seen examples to promote such complexity in sql2 queries.

          Show
          Alex Parvulescu added a comment - Actually in the context of Orderings, the property thing works ok. I tested with a prefix ('order by a. [pname] '), and without one ('order by [pname] ') and it seems to be able to identify the property. I guess it is a bit confusing that the order info (Ordering) takes an Operand, as I haven't seen examples to promote such complexity in sql2 queries.
          Hide
          Alex Parvulescu added a comment -

          I've seen the option with wrapping a Node in a Row just to use the OperandEvaluator, but then it kinda feels hacky. Everything is there already.

          The property name thing will make sense when you take a look at JCR-2959. I couldn't find another way of extracting the property that is used by an Operand. But you may be right, this could fail in certain scenarios.

          I needed to build a lucene sort mechanism, and all the info you have is the Ordering, which holds an Operand, and a bunch of nodes. As the sort should happen before the Row is actually constructed.
          Ok, there may be better ways of extracting this info, I'll digg into it

          Show
          Alex Parvulescu added a comment - I've seen the option with wrapping a Node in a Row just to use the OperandEvaluator, but then it kinda feels hacky. Everything is there already. The property name thing will make sense when you take a look at JCR-2959 . I couldn't find another way of extracting the property that is used by an Operand. But you may be right, this could fail in certain scenarios. I needed to build a lucene sort mechanism, and all the info you have is the Ordering, which holds an Operand, and a bunch of nodes. As the sort should happen before the Row is actually constructed. Ok, there may be better ways of extracting this info, I'll digg into it
          Hide
          Jukka Zitting added a comment -

          I'm not too much of a fan of duplicating so much of the code in OperandEvaluator.

          It would be nice if instead the Node to be used was simply wrapped into a simple row instance with a default full text search score and then passed to the existing code. The attached patch does this by moving the generic Row implementations to -core to -jcr-commons and then using the SelectorRow class in the OperandEvaluator to support also Nodes in addition to Rows.

          PS. What's the use of the getAffectedPropertyName() method? Note that the property under a given Operand also depends on the selector name, so just getting the property name doesn't tell you much unless you already have more context information for interpreting it (in which case you might already have also the property name).

          Show
          Jukka Zitting added a comment - I'm not too much of a fan of duplicating so much of the code in OperandEvaluator. It would be nice if instead the Node to be used was simply wrapped into a simple row instance with a default full text search score and then passed to the existing code. The attached patch does this by moving the generic Row implementations to -core to -jcr-commons and then using the SelectorRow class in the OperandEvaluator to support also Nodes in addition to Rows. PS. What's the use of the getAffectedPropertyName() method? Note that the property under a given Operand also depends on the selector name, so just getting the property name doesn't tell you much unless you already have more context information for interpreting it (in which case you might already have also the property name).
          Hide
          Alex Parvulescu added a comment -

          fixed in revision 1157081

          Show
          Alex Parvulescu added a comment - fixed in revision 1157081
          Hide
          Alex Parvulescu added a comment -

          Yes, you are right, but then it doesn't make sense to get the score out of the node. So it will just return the default score.

          I got distracted, and forgot to commit

          Show
          Alex Parvulescu added a comment - Yes, you are right, but then it doesn't make sense to get the score out of the node. So it will just return the default score. I got distracted, and forgot to commit
          Hide
          Jukka Zitting added a comment -

          I had to use Row instead of a Node in the OperantEvaluator class to properly implement the FullTextSearchScore operand. The score is only available through Row.getScore().

          Show
          Jukka Zitting added a comment - I had to use Row instead of a Node in the OperantEvaluator class to properly implement the FullTextSearchScore operand. The score is only available through Row.getScore().

            People

            • Assignee:
              Alex Parvulescu
              Reporter:
              Alex Parvulescu
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development