[jira] [Comment Edited] (OAK-2511) Support for faceted search in Lucene index

["Chetan Mehrotra (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-2511?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15059600#comment-15059600 ] 

Chetan Mehrotra edited comment on OAK-2511 at 12/16/15 7:09 AM:
----------------------------------------------------------------

[~teofili] Had a look at the commit and have some comments. Kindly have a look

*A - Config*

# Property definition name - Instead of {{facets}} it might be better to use {{faceted}}
{noformat}
     /**
+     * Optional (property definition) property indicating facet can be retrieved together with plain queries.
+     * Default is false
+     */
+    String PROP_FACET = "facets";
{noformat}
# Facets config - Currently there is one {{secureFacets}}. Would we later expose more config to ow facets are managed? If yes then have a node {{facets}} and then move this config under that. This would help in keeping all facet config at one place



*B - Change in LuceneIndexEditor*

# facetsConfig is static. It does not look like to be stateless
{code}

+    private static final FacetsConfig facetsConfig = new FacetsConfig();
+
{code}
# Move facet field name logic to {{FieldNames}} class
{code}
+            String facetFieldName = pname + "_facet";
{code}
# Better to move this logic to PropertyDefinition#validate and have it as stats set. Easier to update later!
{code}
+    private boolean addFacetFields(List<Field> fields, PropertyState property, String pname, PropertyDefinition pd) {
+        // force skipping some JCR properties whose faceting wouldn't make sense (unique or possibly very large values)
+        if (!pname.equals(JCR_DATA) && !pname.equals(JCR_CREATED) && !pname.equals(JCR_UUID)) {
{code}
# *Facet and field Type* - Current impl looks like creats faceted field for all type. It would be better to restrict it to String type. For other types I am not sure if it makes sense. If we see valid usecase later we can enable that
# *Faceting and relative properties* - Current approach would not work for relative properties. If you create facet fields from within {{indexProperty}} then it would be called from both {{makeDocument}} for immediate properties and {{indexAggregates}} for relative properties. 

*C - Changes in LucenePropertyIndex*

# {{loadDocs}} method is now becoming too big and doing lots of things. We should move faceting logic to separate method. I can take care of this
# (minor)Move Facet field logic to {{IndexPlan}} as part of {{IndexPlanner#getPlanBuilder}} call where it iterates over the property definitions. 
{code}
+                        List<String> facetFields = new LinkedList<String>();
+                        List<PropertyRestriction> facetRestriction = filter.getPropertyRestrictions(QueryImpl.REP_FACET);
+                        if (facetRestriction != null && facetRestriction.size() > 0) {
+                            for (PropertyRestriction pr : facetRestriction) {
+                                String value = pr.first.getValue(Type.STRING);
+                                facetFields.add(value.substring(QueryImpl.REP_FACET.length() + 1, value.length() - 1));
+                            }
+                        }
{code}
# Use PERF_LOGGER instead of System.currentTimeMillis()
{code}
+                                long f = System.currentTimeMillis();
+                                for (String facetField : facetFields) {
{code}
# (minor) Facet field name logic to a method
{code}
#395 - facetFields.add(value.substring(QueryImpl.REP_FACET.length() + 1, value.length() - 1));
#1548 String facetFieldName = columnName.substring(QueryImpl.REP_FACET.length() + 1, columnName.length() - 1);
{code}


was (Author: chetanm):
[~teofili] Had a look at the commit and have some comments. Kindly have a look

*A - Config*

# Property definition name - Instead of {{facets}} it might be better to use {{faceted}}
{noformat}
     /**
+     * Optional (property definition) property indicating facet can be retrieved together with plain queries.
+     * Default is false
+     */
+    String PROP_FACET = "facets";
{noformat}
# Facets config - Currently there is one {{secureFacets}}. Would we later expose more config to ow facets are managed? If yes then have a node {{facets}} and then move this config under that. This would help in keeping all facet config at one place



*B - Change in LuceneIndexEditor*

# facetsConfig is static. It does not look like to be stateless
{code}

+    private static final FacetsConfig facetsConfig = new FacetsConfig();
+
{code}
# Move facet field name logic to {{FieldNames}} class
{code}
+            String facetFieldName = pname + "_facet";
{code}
# *Facet and field Type* - Current impl looks like creats faceted field for all type. It would be better to restrict it to String type. For other types I am not sure if it makes sense. If we see valid usecase later we can enable that
# *Faceting and relative properties* - Current approach would not work for relative properties. If you create facet fields from within {{indexProperty}} then it would be called from both {{makeDocument}} for immediate properties and {{indexAggregates}} for relative properties. 

*C - Changes in LucenePropertyIndex*

# {{loadDocs}} method is now becoming too big and doing lots of things. We should move faceting logic to separate method. I can take care of this
# (minor)Move Facet field logic to {{IndexPlan}} as part of {{IndexPlanner#getPlanBuilder}} call where it iterates over the property definitions. 
{noformat}
+                        List<String> facetFields = new LinkedList<String>();
+                        List<PropertyRestriction> facetRestriction = filter.getPropertyRestrictions(QueryImpl.REP_FACET);
+                        if (facetRestriction != null && facetRestriction.size() > 0) {
+                            for (PropertyRestriction pr : facetRestriction) {
+                                String value = pr.first.getValue(Type.STRING);
+                                facetFields.add(value.substring(QueryImpl.REP_FACET.length() + 1, value.length() - 1));
+                            }
+                        }
{noformat}
# Use PERF_LOGGER instead of System.currentTimeMillis()
{noformat}
+                                long f = System.currentTimeMillis();
+                                for (String facetField : facetFields) {
{noformat}





> Support for faceted search in Lucene index
> ------------------------------------------
>
>                 Key: OAK-2511
>                 URL: https://issues.apache.org/jira/browse/OAK-2511
>             Project: Jackrabbit Oak
>          Issue Type: Sub-task
>          Components: lucene
>            Reporter: Tommaso Teofili
>            Assignee: Tommaso Teofili
>             Fix For: 1.3.13
>
>
> A Lucene index should be able to provide faceted search results, based on the support provided in the query engine.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)