Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9279

Add greater than, less than, etc in Solr function queries

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.2
    • Component/s: search
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      If you use the "if" function query, you'll often expect to be able to use greater than/less than functions. For example, you might want to boost books written in the past 7 years. Unfortunately, there's no "greater than" function query that will return non-zero when the lhs > rhs. Instead to get this, you need to create really awkward function queries like I do here (http://opensourceconnections.com/blog/2014/11/26/stepwise-date-boosting-in-solr/):

      if(min(0,sub(ms(mydatefield),sub(ms(NOW),315569259747))),0.8,1)

      The pull request attached to this Jira adds the following function queries
      (https://github.com/apache/lucene-solr/pull/49)

      -gt(lhs, rhs) (returns 1 if lhs > rhs, 0 otherwise)
      -lt(lhs, rhs) (returns 1 if lhs < rhs, 0 otherwise)
      -gte
      -lte
      -eq

      So instead of

      if(min(0,sub(ms(mydatefield),sub(ms(NOW),315569259747))),0.8,1)

      one could now write

      if(lt(ms(mydatefield),315569259747,0.8,1)

      (if mydatefield < 315569259747 then 0.8 else 1)

      A bit more readable and less puzzling

      1. SOLR-9279.patch
        18 kB
        David Smiley

        Issue Links

          Activity

          Hide
          softwaredoug Doug Turnbull added a comment -
          Show
          softwaredoug Doug Turnbull added a comment - Associated Pull request https://github.com/apache/lucene-solr/pull/49
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69671503

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          +
          + public final ValueSource lhs;
          — End diff –

          eek; public?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69671503 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { + + public final ValueSource lhs; — End diff – eek; public?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69671763

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          +
          + public final ValueSource lhs;
          + public final ValueSource rhs;
          +
          + public CompareNumericFunction(ValueSource lhs, ValueSource rhs)

          { + this.lhs = lhs; + this.rhs = rhs; + }

          +
          + // Perform the comparison, returning true or false
          + public abstract boolean compareNumeric(double lhs, double rhs);
          — End diff –

          Instead of making this an abstract class, what do you think of making it concrete and take the label and a simple interface named something like LongBinaryPredicate (a specialization of JDK java.util.BiPredicate), then creating some singleton instances? I would reduce all these subclasses. A smaller change that reduces the top-level classes you create here is to make the subclasses in fact inner classes or even anonymous inner classes to initialize some singleton instances of them. I think I like that a little better in that there is no need to create a new interface (even if it is an inner one). I welcome your input.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69671763 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { + + public final ValueSource lhs; + public final ValueSource rhs; + + public CompareNumericFunction(ValueSource lhs, ValueSource rhs) { + this.lhs = lhs; + this.rhs = rhs; + } + + // Perform the comparison, returning true or false + public abstract boolean compareNumeric(double lhs, double rhs); — End diff – Instead of making this an abstract class, what do you think of making it concrete and take the label and a simple interface named something like LongBinaryPredicate (a specialization of JDK java.util.BiPredicate), then creating some singleton instances? I would reduce all these subclasses. A smaller change that reduces the top-level classes you create here is to make the subclasses in fact inner classes or even anonymous inner classes to initialize some singleton instances of them. I think I like that a little better in that there is no need to create a new interface (even if it is an inner one). I welcome your input.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69671790

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          — End diff –

          Extend Lucene "BoolFunction" instead?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69671790 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { — End diff – Extend Lucene "BoolFunction" instead?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69671875

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          +
          + public final ValueSource lhs;
          + public final ValueSource rhs;
          +
          + public CompareNumericFunction(ValueSource lhs, ValueSource rhs)

          { + this.lhs = lhs; + this.rhs = rhs; + }

          +
          + // Perform the comparison, returning true or false
          + public abstract boolean compareNumeric(double lhs, double rhs);
          +
          + // Uniquely identify the operation (ie "gt", "lt" "gte, etc)
          + public abstract String getLabel();
          +
          + // string comparison? Probably should be a seperate function
          + // public abstract boolean compareString(String lhs, String rhs);
          +
          + public FunctionValues getValues(Map context, LeafReaderContext readerContext) throws IOException {
          + final FunctionValues lhsVal = this.lhs.getValues(context, readerContext);
          + final FunctionValues rhsVal = this.rhs.getValues(context, readerContext);
          + final String compLabel = this.getLabel();
          +
          + return new FunctionValues() {
          — End diff –

          Extend Lucene BoolDocValues instead.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69671875 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { + + public final ValueSource lhs; + public final ValueSource rhs; + + public CompareNumericFunction(ValueSource lhs, ValueSource rhs) { + this.lhs = lhs; + this.rhs = rhs; + } + + // Perform the comparison, returning true or false + public abstract boolean compareNumeric(double lhs, double rhs); + + // Uniquely identify the operation (ie "gt", "lt" "gte, etc) + public abstract String getLabel(); + + // string comparison? Probably should be a seperate function + // public abstract boolean compareString(String lhs, String rhs); + + public FunctionValues getValues(Map context, LeafReaderContext readerContext) throws IOException { + final FunctionValues lhsVal = this.lhs.getValues(context, readerContext); + final FunctionValues rhsVal = this.rhs.getValues(context, readerContext); + final String compLabel = this.getLabel(); + + return new FunctionValues() { — End diff – Extend Lucene BoolDocValues instead.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69672138

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          — End diff –

          Maybe rename ComparisonPredicateValueSource? Using "predicate" seems appropriate; Numeric seems redundant with anything ValueSource. Not sure on Function suffix; there's sadly a mix in Lucene but I think it's better to prefer the subclass's name.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69672138 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { — End diff – Maybe rename ComparisonPredicateValueSource? Using "predicate" seems appropriate; Numeric seems redundant with anything ValueSource. Not sure on Function suffix; there's sadly a mix in Lucene but I think it's better to prefer the subclass's name.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69723224

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          +
          + public final ValueSource lhs;
          — End diff –

          whoops, thanks

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69723224 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { + + public final ValueSource lhs; — End diff – whoops, thanks
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69724481

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          — End diff –

          I thought about BoolFunction, but it seemed unused. I certainly could with no change, it would only have documentation value.

          Would removing "value" imply this also works on strings? I'm not thoroughly versed here but when I was looking at IfFunction, I intentionally omitted non-numeric comparions (ie string comparisons). Hence the name.

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69724481 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { — End diff – I thought about BoolFunction, but it seemed unused. I certainly could with no change, it would only have documentation value. Would removing "value" imply this also works on strings? I'm not thoroughly versed here but when I was looking at IfFunction, I intentionally omitted non-numeric comparions (ie string comparisons). Hence the name.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69724555

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          — End diff –

          oh wait I'm wrong on BoolFunction, it is used. I'll at least switch to that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69724555 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { — End diff – oh wait I'm wrong on BoolFunction, it is used. I'll at least switch to that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69726431

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          +
          + public final ValueSource lhs;
          + public final ValueSource rhs;
          +
          + public CompareNumericFunction(ValueSource lhs, ValueSource rhs)

          { + this.lhs = lhs; + this.rhs = rhs; + }

          +
          + // Perform the comparison, returning true or false
          + public abstract boolean compareNumeric(double lhs, double rhs);
          — End diff –

          I think I like this idea in general, it would get rid of a lot of cruft. I can pass a lot in as constructor args instead of creating lots of noisy classes.

          I'm not sure I want to use LongBinaryPredicate (do you mean [LongPredicate](https://docs.oracle.com/javase/8/docs/api/java/util/function/LongPredicate.html)?. Taking a glance it seems like the value in using it would be in the support for and/or/not operators in a Java-consistent way. It would be nice, though a bit out of scope for this PR, to extend this idea to the other boolean operations so that valuesource's were compatible with this API.

          Let me push up a change that makes more anonymous instances and let me know what you think (I'll model in on how and, or, xor, etc are done)

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69726431 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { + + public final ValueSource lhs; + public final ValueSource rhs; + + public CompareNumericFunction(ValueSource lhs, ValueSource rhs) { + this.lhs = lhs; + this.rhs = rhs; + } + + // Perform the comparison, returning true or false + public abstract boolean compareNumeric(double lhs, double rhs); — End diff – I think I like this idea in general, it would get rid of a lot of cruft. I can pass a lot in as constructor args instead of creating lots of noisy classes. I'm not sure I want to use LongBinaryPredicate (do you mean [LongPredicate] ( https://docs.oracle.com/javase/8/docs/api/java/util/function/LongPredicate.html)? . Taking a glance it seems like the value in using it would be in the support for and/or/not operators in a Java-consistent way. It would be nice, though a bit out of scope for this PR, to extend this idea to the other boolean operations so that valuesource's were compatible with this API. Let me push up a change that makes more anonymous instances and let me know what you think (I'll model in on how and, or, xor, etc are done)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69765065

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java —
          @@ -0,0 +1,131 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class CompareNumericFunction extends ValueSource {
          +
          + public final ValueSource lhs;
          + public final ValueSource rhs;
          +
          + public CompareNumericFunction(ValueSource lhs, ValueSource rhs)

          { + this.lhs = lhs; + this.rhs = rhs; + }

          +
          + // Perform the comparison, returning true or false
          + public abstract boolean compareNumeric(double lhs, double rhs);
          — End diff –

          > I'm not sure I want to use LongBinaryPredicate (do you mean LongPredicate?.

          I am referring to no JDK interfaces other than saying it's similar to BiPredicate, but specialized to the primitive type. And I definitely didn't mean to suggest anything involving a refactor to Lucene's existing APIs.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69765065 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/CompareNumericFunction.java — @@ -0,0 +1,131 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class CompareNumericFunction extends ValueSource { + + public final ValueSource lhs; + public final ValueSource rhs; + + public CompareNumericFunction(ValueSource lhs, ValueSource rhs) { + this.lhs = lhs; + this.rhs = rhs; + } + + // Perform the comparison, returning true or false + public abstract boolean compareNumeric(double lhs, double rhs); — End diff – > I'm not sure I want to use LongBinaryPredicate (do you mean LongPredicate?. I am referring to no JDK interfaces other than saying it's similar to BiPredicate, but specialized to the primitive type. And I definitely didn't mean to suggest anything involving a refactor to Lucene's existing APIs.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          (note the last few commits based on your comments)

          On unifying somewhat with MultiBoolValues, or creating a BiBoolValues, I went down that path a bit David and it seems to complicate things. A couple of notes:

          • The bool values functions takes as input other boolfunctions, whereas the comparison value source takes in scalar values. You can see this in how or, and, xor work: they loop over several boolean value sources and perform and, or, xor etc. We just need to pluck out two scalar values and compare them
          • The name `func` seems descriptive of this general behavior, whereas `compare` is more descriptive of the operation being perfomed by the comparison value source

          I think the comparison functions are more readable now as they are, but I'd be curious to get your thoughts.

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 (note the last few commits based on your comments) On unifying somewhat with MultiBoolValues, or creating a BiBoolValues, I went down that path a bit David and it seems to complicate things. A couple of notes: The bool values functions takes as input other boolfunctions, whereas the comparison value source takes in scalar values. You can see this in how or, and, xor work: they loop over several boolean value sources and perform and, or, xor etc. We just need to pluck out two scalar values and compare them The name `func` seems descriptive of this general behavior, whereas `compare` is more descriptive of the operation being perfomed by the comparison value source I think the comparison functions are more readable now as they are, but I'd be curious to get your thoughts.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69934693

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/ComparisonValueSource.java —
          @@ -0,0 +1,104 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.queries.function.docvalues.BoolDocValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class ComparisonValueSource extends BoolFunction {
          +
          + private final ValueSource lhs;
          + private final ValueSource rhs;
          + private final String name;
          +
          + public ComparisonValueSource(ValueSource lhs, ValueSource rhs, String name)

          { + this.lhs = lhs; + this.rhs = rhs; + this.name = name; + }

          +
          + // Perform the comparison, returning true or false
          + public abstract boolean compare(double lhs, double rhs);
          +
          + // Uniquely identify the operation (ie "gt", "lt" "gte", etc)
          + public String name()

          { + return this.name; + }

          +
          + // string comparison? Probably should be a seperate function
          + // public abstract boolean compareString(String lhs, String rhs);
          +
          + public FunctionValues getValues(Map context, LeafReaderContext readerContext) throws IOException {
          + final FunctionValues lhsVal = this.lhs.getValues(context, readerContext);
          + final FunctionValues rhsVal = this.rhs.getValues(context, readerContext);
          + final String compLabel = this.name();
          +
          + return new BoolDocValues(this) {
          + @Override
          + public boolean boolVal(int doc)

          { + return compare(lhsVal.floatVal(doc), rhsVal.floatVal(doc)); + }

          +
          + @Override
          + public String toString(int doc)

          { + return compLabel + "(" + lhsVal.toString(doc) + "," + rhsVal.toString(doc) + ")"; + }

          + };
          + }
          +
          + @Override
          + public boolean equals(Object o) {
          + if (this.getClass() != o.getClass()) return false;
          + if (!(o instanceof ComparisonValueSource)) return false;
          — End diff –

          This line is not needed; the classes must be equal in the previous line.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69934693 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/ComparisonValueSource.java — @@ -0,0 +1,104 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.queries.function.docvalues.BoolDocValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class ComparisonValueSource extends BoolFunction { + + private final ValueSource lhs; + private final ValueSource rhs; + private final String name; + + public ComparisonValueSource(ValueSource lhs, ValueSource rhs, String name) { + this.lhs = lhs; + this.rhs = rhs; + this.name = name; + } + + // Perform the comparison, returning true or false + public abstract boolean compare(double lhs, double rhs); + + // Uniquely identify the operation (ie "gt", "lt" "gte", etc) + public String name() { + return this.name; + } + + // string comparison? Probably should be a seperate function + // public abstract boolean compareString(String lhs, String rhs); + + public FunctionValues getValues(Map context, LeafReaderContext readerContext) throws IOException { + final FunctionValues lhsVal = this.lhs.getValues(context, readerContext); + final FunctionValues rhsVal = this.rhs.getValues(context, readerContext); + final String compLabel = this.name(); + + return new BoolDocValues(this) { + @Override + public boolean boolVal(int doc) { + return compare(lhsVal.floatVal(doc), rhsVal.floatVal(doc)); + } + + @Override + public String toString(int doc) { + return compLabel + "(" + lhsVal.toString(doc) + "," + rhsVal.toString(doc) + ")"; + } + }; + } + + @Override + public boolean equals(Object o) { + if (this.getClass() != o.getClass()) return false; + if (!(o instanceof ComparisonValueSource)) return false; — End diff – This line is not needed; the classes must be equal in the previous line.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69934986

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/ComparisonValueSource.java —
          @@ -0,0 +1,104 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.queries.function.docvalues.BoolDocValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class ComparisonValueSource extends BoolFunction {
          +
          + private final ValueSource lhs;
          + private final ValueSource rhs;
          + private final String name;
          +
          + public ComparisonValueSource(ValueSource lhs, ValueSource rhs, String name)

          { + this.lhs = lhs; + this.rhs = rhs; + this.name = name; + }

          +
          + // Perform the comparison, returning true or false
          — End diff –

          Use javadoc method comments, not //

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69934986 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/ComparisonValueSource.java — @@ -0,0 +1,104 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.queries.function.docvalues.BoolDocValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class ComparisonValueSource extends BoolFunction { + + private final ValueSource lhs; + private final ValueSource rhs; + private final String name; + + public ComparisonValueSource(ValueSource lhs, ValueSource rhs, String name) { + this.lhs = lhs; + this.rhs = rhs; + this.name = name; + } + + // Perform the comparison, returning true or false — End diff – Use javadoc method comments, not //
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on a diff in the pull request:

          https://github.com/apache/lucene-solr/pull/49#discussion_r69935281

          — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/ComparisonValueSource.java —
          @@ -0,0 +1,104 @@
          +package org.apache.lucene.queries.function.valuesource;
          +
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +import java.io.IOException;
          +import java.util.Map;
          +
          +import org.apache.lucene.index.LeafReaderContext;
          +import org.apache.lucene.queries.function.ValueSource;
          +import org.apache.lucene.queries.function.FunctionValues;
          +import org.apache.lucene.queries.function.docvalues.BoolDocValues;
          +import org.apache.lucene.search.IndexSearcher;
          +
          +
          +/**
          + * Base class for comparison operators used within if statements
          + * To Solr's if function query a 0 is considered "false", all other values are "true"
          + */
          +public abstract class ComparisonValueSource extends BoolFunction {
          +
          + private final ValueSource lhs;
          + private final ValueSource rhs;
          + private final String name;
          +
          + public ComparisonValueSource(ValueSource lhs, ValueSource rhs, String name)

          { + this.lhs = lhs; + this.rhs = rhs; + this.name = name; + }

          +
          + // Perform the comparison, returning true or false
          + public abstract boolean compare(double lhs, double rhs);
          +
          + // Uniquely identify the operation (ie "gt", "lt" "gte", etc)
          + public String name()

          { + return this.name; + }

          +
          + // string comparison? Probably should be a seperate function
          + // public abstract boolean compareString(String lhs, String rhs);
          +
          + public FunctionValues getValues(Map context, LeafReaderContext readerContext) throws IOException {
          + final FunctionValues lhsVal = this.lhs.getValues(context, readerContext);
          + final FunctionValues rhsVal = this.rhs.getValues(context, readerContext);
          + final String compLabel = this.name();
          +
          + return new BoolDocValues(this) {
          + @Override
          + public boolean boolVal(int doc) {
          + return compare(lhsVal.floatVal(doc), rhsVal.floatVal(doc));
          — End diff –

          Can you instead pass lhsVal & rhsVal to let the compare() function choose if it wants to call floatVal, doubleVal, or longVal or whatever?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/49#discussion_r69935281 — Diff: lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/ComparisonValueSource.java — @@ -0,0 +1,104 @@ +package org.apache.lucene.queries.function.valuesource; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.IOException; +import java.util.Map; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.function.ValueSource; +import org.apache.lucene.queries.function.FunctionValues; +import org.apache.lucene.queries.function.docvalues.BoolDocValues; +import org.apache.lucene.search.IndexSearcher; + + +/** + * Base class for comparison operators used within if statements + * To Solr's if function query a 0 is considered "false", all other values are "true" + */ +public abstract class ComparisonValueSource extends BoolFunction { + + private final ValueSource lhs; + private final ValueSource rhs; + private final String name; + + public ComparisonValueSource(ValueSource lhs, ValueSource rhs, String name) { + this.lhs = lhs; + this.rhs = rhs; + this.name = name; + } + + // Perform the comparison, returning true or false + public abstract boolean compare(double lhs, double rhs); + + // Uniquely identify the operation (ie "gt", "lt" "gte", etc) + public String name() { + return this.name; + } + + // string comparison? Probably should be a seperate function + // public abstract boolean compareString(String lhs, String rhs); + + public FunctionValues getValues(Map context, LeafReaderContext readerContext) throws IOException { + final FunctionValues lhsVal = this.lhs.getValues(context, readerContext); + final FunctionValues rhsVal = this.rhs.getValues(context, readerContext); + final String compLabel = this.name(); + + return new BoolDocValues(this) { + @Override + public boolean boolVal(int doc) { + return compare(lhsVal.floatVal(doc), rhsVal.floatVal(doc)); — End diff – Can you instead pass lhsVal & rhsVal to let the compare() function choose if it wants to call floatVal, doubleVal, or longVal or whatever?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          I think after addressing the comments I just added, it's probably good to go. I still don't love the name, especially since it extends BoolFunction it ought to now end with Function.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/49 I think after addressing the comments I just added, it's probably good to go. I still don't love the name, especially since it extends BoolFunction it ought to now end with Function.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Renaming is a good idea. I can push that up. ComparisonFunction?

          On Thu, Jul 7, 2016 at 12:13 PM David Smiley <notifications@github.com>
          wrote:

          > I think after addressing the comments I just added, it's probably good to
          > go. I still don't love the name, especially since it extends BoolFunction
          > it ought to now end with Function.
          >
          > —
          > You are receiving this because you authored the thread.
          > Reply to this email directly, view it on GitHub
          > <https://github.com/apache/lucene-solr/pull/49#issuecomment-231126560>,
          > or mute the thread
          > <https://github.com/notifications/unsubscribe/AAmZRMlf6o2flc_NCh8LYXm1Ha2_S7_Iks5qTSPtgaJpZM4JFqfq>
          > .
          >

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 Renaming is a good idea. I can push that up. ComparisonFunction? On Thu, Jul 7, 2016 at 12:13 PM David Smiley <notifications@github.com> wrote: > I think after addressing the comments I just added, it's probably good to > go. I still don't love the name, especially since it extends BoolFunction > it ought to now end with Function. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < https://github.com/apache/lucene-solr/pull/49#issuecomment-231126560 >, > or mute the thread > < https://github.com/notifications/unsubscribe/AAmZRMlf6o2flc_NCh8LYXm1Ha2_S7_Iks5qTSPtgaJpZM4JFqfq > > . >
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Renamed to ComparisonBoolFunction

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 Renamed to ComparisonBoolFunction
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Dumb question, what's the next steps? Do I need to do anything else here or at the JIRA ticket?

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 Dumb question, what's the next steps? Do I need to do anything else here or at the JIRA ticket?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Just one thing – have compare() take the FunctionValue so that a compare impl can choose to call doubleVal vs longVal or whatever else. And the impls you add to Solr can call doubleVal. Someone truly might want to extend this to call something other than doubleVal; the set of values of doubleVal is disjoint from longVal. Or maybe someone has got the data in objectVal for some reason.

          After that please post a .patch file to JIRA. https://wiki.apache.org/lucene-java/HowToContribute#Creating_a_patch though those instructions should be modified to indicate how to generate a diff from the point the current branch diverged from master.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/49 Just one thing – have compare() take the FunctionValue so that a compare impl can choose to call doubleVal vs longVal or whatever else. And the impls you add to Solr can call doubleVal. Someone truly might want to extend this to call something other than doubleVal; the set of values of doubleVal is disjoint from longVal. Or maybe someone has got the data in objectVal for some reason. After that please post a .patch file to JIRA. https://wiki.apache.org/lucene-java/HowToContribute#Creating_a_patch though those instructions should be modified to indicate how to generate a diff from the point the current branch diverged from master.
          Hide
          hossman Hoss Man added a comment -

          I didn't look at the pull request in depth, but in general i applaud the idea and the general approach from skimming the issue description and CompareNumericFunction.java

          3 things that did jump out at me when skimming the patch as a whole:

          • i see edits to ValueSourceParser.java but no edits to QueryEqualityTest.java ... that gives me 99% confidence that this patch breaks QueryEqualityTest
          • I see tests of using these new functions when wrapped in if(...) but no (obvious to me) tests of these new functions being used directly for their return value – ex: fl=id,gte(price,0) – and demonstrating what the expected result should be
            • in this example, i would expect the result type to be a Boolean (ie: <bool name="gte(price,0)">true</bool>
            • Since i don't see FunctionValues.objectVal overridden anywhere in the patch, i'm assuming this doesn't work as I expect
          • I don't see FunctionValues.exists overridden anywhere in this patch, which IIRC means these functions are always going to "exists=true", which does not seem like a good hardcoded behavior for a ValueSource thta wraps other ValueSources.
            • some thought/javadocs should be given to how exactly these functions should behave if/when one or more of the ValueSources they wrap do not exist for a given document – and some tests demonstrating the expected behavior in these situations seem crucial.
            • see LUCENE-5961, and the core premise expressed in the first comment on that issue, which seems just as relevant to me for this issue.
            • I would suggest implementing exists using MultiFunction.allExists(...), that way callers can decide for themselves how it should behave, by wrapping the inner ValueSources in DefFunction, and/or by wrapping their CompareNumericFunction in a DefFunction as they see fit.
          Show
          hossman Hoss Man added a comment - I didn't look at the pull request in depth, but in general i applaud the idea and the general approach from skimming the issue description and CompareNumericFunction.java 3 things that did jump out at me when skimming the patch as a whole: i see edits to ValueSourceParser.java but no edits to QueryEqualityTest.java ... that gives me 99% confidence that this patch breaks QueryEqualityTest I see tests of using these new functions when wrapped in if(...) but no (obvious to me) tests of these new functions being used directly for their return value – ex: fl=id,gte(price,0) – and demonstrating what the expected result should be in this example, i would expect the result type to be a Boolean (ie: <bool name="gte(price,0)">true</bool> Since i don't see FunctionValues.objectVal overridden anywhere in the patch, i'm assuming this doesn't work as I expect I don't see FunctionValues.exists overridden anywhere in this patch, which IIRC means these functions are always going to "exists=true", which does not seem like a good hardcoded behavior for a ValueSource thta wraps other ValueSources. some thought/javadocs should be given to how exactly these functions should behave if/when one or more of the ValueSources they wrap do not exist for a given document – and some tests demonstrating the expected behavior in these situations seem crucial. see LUCENE-5961 , and the core premise expressed in the first comment on that issue, which seems just as relevant to me for this issue. I would suggest implementing exists using MultiFunction.allExists(...) , that way callers can decide for themselves how it should behave, by wrapping the inner ValueSources in DefFunction, and/or by wrapping their CompareNumericFunction in a DefFunction as they see fit.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Actually yes, I would see that as a bug right now. I bet comparing two
          longs one off would fail a > or < comparison due to loss of precision as a
          double. I'm going to try to create a test that recreates that.

          On Fri, Jul 8, 2016 at 2:37 PM David Smiley <notifications@github.com>
          wrote:

          > Just one thing – have compare() take the FunctionValue so that a compare
          > impl can choose to call doubleVal vs longVal or whatever else. And the
          > impls you add to Solr can call doubleVal. Someone truly might want to
          > extend this to call something other than doubleVal; the set of values of
          > doubleVal is disjoint from longVal. Or maybe someone has got the data in
          > objectVal for some reason.
          >
          > After that please post a .patch file to JIRA.
          > https://wiki.apache.org/lucene-java/HowToContribute#Creating_a_patch
          > though those instructions should be modified to indicate how to generate a
          > diff from the point the current branch diverged from master.
          >
          > —
          > You are receiving this because you commented.
          >
          >
          > Reply to this email directly, view it on GitHub
          > <https://github.com/apache/lucene-solr/pull/49#issuecomment-231439077>,
          > or mute the thread
          > <https://github.com/notifications/unsubscribe/AAmZRIJmnJcZ3hLpIOLcti_z7ZyVWCxnks5qTpjYgaJpZM4JFqfq>
          > .
          >

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 Actually yes, I would see that as a bug right now. I bet comparing two longs one off would fail a > or < comparison due to loss of precision as a double. I'm going to try to create a test that recreates that. On Fri, Jul 8, 2016 at 2:37 PM David Smiley <notifications@github.com> wrote: > Just one thing – have compare() take the FunctionValue so that a compare > impl can choose to call doubleVal vs longVal or whatever else. And the > impls you add to Solr can call doubleVal. Someone truly might want to > extend this to call something other than doubleVal; the set of values of > doubleVal is disjoint from longVal. Or maybe someone has got the data in > objectVal for some reason. > > After that please post a .patch file to JIRA. > https://wiki.apache.org/lucene-java/HowToContribute#Creating_a_patch > though those instructions should be modified to indicate how to generate a > diff from the point the current branch diverged from master. > > — > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > < https://github.com/apache/lucene-solr/pull/49#issuecomment-231439077 >, > or mute the thread > < https://github.com/notifications/unsubscribe/AAmZRIJmnJcZ3hLpIOLcti_z7ZyVWCxnks5qTpjYgaJpZM4JFqfq > > . >
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Ok this needs another look at @dsmiley as I've addressed a bug where Long comparisons failed

          • Adds test that demonstrates bug comparing Longs
          • Changed compare to take two Comparables instead of doubles
          • Perform an integer comparison when it appears the two input values are effectively integers
          • Otherwise does a floating point comparison

          Unfortunately, I think tihs interface can't take arbitrary value sources. I feel like I need to enforce the safest comparison (integer vs floating pt) to avoid bugs.

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 Ok this needs another look at @dsmiley as I've addressed a bug where Long comparisons failed Adds test that demonstrates bug comparing Longs Changed compare to take two Comparables instead of doubles Perform an integer comparison when it appears the two input values are effectively integers Otherwise does a floating point comparison Unfortunately, I think tihs interface can't take arbitrary value sources. I feel like I need to enforce the safest comparison (integer vs floating pt) to avoid bugs.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tflobbe commented on the issue:

          https://github.com/apache/lucene-solr/pull/53

          This PR doesn't seem to be linked to any Jira. Is there one? If you include the Jira issue id in the title of the PR it will automatically show in Jira (e.g. https://issues.apache.org/jira/browse/SOLR-9279 ). You can create a new Jira issue if there isn't one for this fix

          Show
          githubbot ASF GitHub Bot added a comment - Github user tflobbe commented on the issue: https://github.com/apache/lucene-solr/pull/53 This PR doesn't seem to be linked to any Jira. Is there one? If you include the Jira issue id in the title of the PR it will automatically show in Jira (e.g. https://issues.apache.org/jira/browse/SOLR-9279 ). You can create a new Jira issue if there isn't one for this fix
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Sorry but I think it's terrible to invoke both doubleVal & longVal potentially twice per doc on the same FunctionValues. I think what I suggested in earlier feedback is much closer – let the subclass choose which longVal/doubleVal/whatever to call and to make whatever comparison needed. On the Solr end... we could always work with the doubles, even though some 'long' values are out of range. I believe some other FunctionValue impls are implemented similarly as well , despite not representing say Long.MAX_VALUE.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/49 Sorry but I think it's terrible to invoke both doubleVal & longVal potentially twice per doc on the same FunctionValues. I think what I suggested in earlier feedback is much closer – let the subclass choose which longVal/doubleVal/whatever to call and to make whatever comparison needed. On the Solr end... we could always work with the doubles, even though some 'long' values are out of range. I believe some other FunctionValue impls are implemented similarly as well , despite not representing say Long.MAX_VALUE.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          I just think we have to tread carefully. Comparing two 64 bit timestamps could result in surprising bugs where an event seconds after another isn't greater than the earlier event due to a loss of precision casting to double. So I'd rather enforce the safest possible and most correct comparison and make the interface not that general.

          Is your concern performance based? Can we reduce the number of times we call doubleVal/longVal. Sorry I was not aware of the performance implications.

          We could also make the values themselves implement Comparable and let them be compared to other function values. But this seems complex and we'd still need to enforce the correct comparison.

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 I just think we have to tread carefully. Comparing two 64 bit timestamps could result in surprising bugs where an event seconds after another isn't greater than the earlier event due to a loss of precision casting to double. So I'd rather enforce the safest possible and most correct comparison and make the interface not that general. Is your concern performance based? Can we reduce the number of times we call doubleVal/longVal. Sorry I was not aware of the performance implications. We could also make the values themselves implement Comparable and let them be compared to other function values. But this seems complex and we'd still need to enforce the correct comparison.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          I think at the Lucene level we should make this widely useful by simply exposing a boolean compare(FunctionValues lhs, FunctionValues rhs). If we go the way of your latest commit, it would be less useful as some potential users would skip over it to avoid the performance overhead of calling double & longVal, or because they have more interesting requirements and want to call some other method on functionValues. So lets not tie their hands.

          Yes my concern is definitely performance based. These things are super-sensitive to it as it operates once per matching document, which could be a ton.

          What you could perhaps do at the Solr layer is check if both the lhs & rhs extend from LongDocValues and if so then use the Long version... otherwise use the Double version which I think can represent Integer completely. But it'd be very nice not to do that double instanceof check at every comparison... so perhaps that would lead to actually implementing getValues().

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/49 I think at the Lucene level we should make this widely useful by simply exposing a boolean compare(FunctionValues lhs, FunctionValues rhs). If we go the way of your latest commit, it would be less useful as some potential users would skip over it to avoid the performance overhead of calling double & longVal, or because they have more interesting requirements and want to call some other method on functionValues. So lets not tie their hands. Yes my concern is definitely performance based. These things are super-sensitive to it as it operates once per matching document, which could be a ton. What you could perhaps do at the Solr layer is check if both the lhs & rhs extend from LongDocValues and if so then use the Long version... otherwise use the Double version which I think can represent Integer completely. But it'd be very nice not to do that double instanceof check at every comparison... so perhaps that would lead to actually implementing getValues().
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          I can do that, but in the Solr version I will end up overriding compare, then providing further abstract methods to do numerical comparisons. Otherwise I'm going to just end up with lots of boiler plate repeated in the ValueSourceParser

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 I can do that, but in the Solr version I will end up overriding compare, then providing further abstract methods to do numerical comparisons. Otherwise I'm going to just end up with lots of boiler plate repeated in the ValueSourceParser
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          I believe I've done what you requested. There's a lot of value sources that inherit directly from FunctionValues, so without cataloging which ones are best treated as longs and which are best treated as floats, its going to be impossible to always do the safest comparison. The best we can do is test for Long or Int values

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug commented on the issue: https://github.com/apache/lucene-solr/pull/49 I believe I've done what you requested. There's a lot of value sources that inherit directly from FunctionValues, so without cataloging which ones are best treated as longs and which are best treated as floats, its going to be impossible to always do the safest comparison. The best we can do is test for Long or Int values
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          Ok; this is fine. It could be optimized later Did you see Hoss's comments on implementing exists() ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/49 Ok; this is fine. It could be optimized later Did you see Hoss's comments on implementing exists() ?
          Hide
          softwaredoug Doug Turnbull added a comment -

          Hoss Man Thanks for your help! Great points. – I think I addressed your comments other than the Object value one. Is there documentation on an object value source? I'm not sure what's expected here.

          Show
          softwaredoug Doug Turnbull added a comment - Hoss Man Thanks for your help! Great points. – I think I addressed your comments other than the Object value one. Is there documentation on an object value source? I'm not sure what's expected here.
          Hide
          hossman Hoss Man added a comment -

          I haven't been following the updates since my last comment, but to answer your question...

          Is there documentation on an object value source? I'm not sure what's expected here.

          ...well, per the javadocs it's "Native Java Object representation of the value" for the doc – ie: whatever type makes the most sense for the given value source (as opposed to byteVal, intVal, floatVal, etc... where the caller says what type they want)

          For the DocValue based FunctionValues, it's the type of the field (IntFieldSource.getValues returns an IntDocValues which implements objectVal by delegating to intVal if exists is true, etc.

          For ValueSources that "wrap" other value sources, things get more complicated – in the IfFunction, the FunctionValues delegates to one wrapped FunctionValues or the other, depending on the conditional.

          practically speaking: the main place objectValue comes into play is when end users ask for the value of a ValueSource in the fl list, ie: fl=id,value:div(popularity,price),inStock:gte(quantityonHand,1)

          since these new functions represent boolean concepts (even if they are wrapping numeric inputs), i would expect them to implement objectValue the same way BoolDocValues does...

            @Override
            public Object objectVal(int doc) {
              return exists(doc) ? boolVal(doc) : null;
            }
          

          ...ideally you'd just extend BoolDocValues since all of it's methods should also apply to your usecase. (maybe you were doing that in your previous patch and i just missed it – i dunno? ... from my comment it looks like i mainly just noticed you weren't testing for this usage to return Boolean objects)

          Show
          hossman Hoss Man added a comment - I haven't been following the updates since my last comment, but to answer your question... Is there documentation on an object value source? I'm not sure what's expected here. ...well, per the javadocs it's "Native Java Object representation of the value" for the doc – ie: whatever type makes the most sense for the given value source (as opposed to byteVal, intVal, floatVal, etc... where the caller says what type they want) For the DocValue based FunctionValues, it's the type of the field (IntFieldSource.getValues returns an IntDocValues which implements objectVal by delegating to intVal if exists is true, etc. For ValueSources that "wrap" other value sources, things get more complicated – in the IfFunction, the FunctionValues delegates to one wrapped FunctionValues or the other, depending on the conditional. practically speaking: the main place objectValue comes into play is when end users ask for the value of a ValueSource in the fl list, ie: fl=id,value:div(popularity,price),inStock:gte(quantityonHand,1) since these new functions represent boolean concepts (even if they are wrapping numeric inputs), i would expect them to implement objectValue the same way BoolDocValues does... @Override public Object objectVal( int doc) { return exists(doc) ? boolVal(doc) : null ; } ...ideally you'd just extend BoolDocValues since all of it's methods should also apply to your usecase. (maybe you were doing that in your previous patch and i just missed it – i dunno? ... from my comment it looks like i mainly just noticed you weren't testing for this usage to return Boolean objects)
          Hide
          dsmiley David Smiley added a comment -

          Your last patch was pretty good Doug. I spent some time last night and this morning cleaning it up a tad (fixed ant precommit issues) and then I went a bit further. I don't really like "SafeNumericComparisonValueSource" in Lucene so I moved it to Solr naming it SolrComparisonValueSource. I also changed it to not do primitive to object conversion, and in so doing changed the api a bit to make the implementations do their job via a lambda.

          What do you think?

          Show
          dsmiley David Smiley added a comment - Your last patch was pretty good Doug. I spent some time last night and this morning cleaning it up a tad (fixed ant precommit issues) and then I went a bit further. I don't really like "SafeNumericComparisonValueSource" in Lucene so I moved it to Solr naming it SolrComparisonValueSource. I also changed it to not do primitive to object conversion, and in so doing changed the api a bit to make the implementations do their job via a lambda. What do you think?
          Hide
          softwaredoug Doug Turnbull added a comment -

          Looks great David Smiley! Definitely a big improvement. Appreciate your attention, I've learned a lot through this issue.

          What do you think about adding an objectValue override as suggested by Hoss Man?

           @Override
            public Object objectVal(int doc) {
              return exists(doc) ? boolVal(doc) : null;
            }
          
          Show
          softwaredoug Doug Turnbull added a comment - Looks great David Smiley ! Definitely a big improvement. Appreciate your attention, I've learned a lot through this issue. What do you think about adding an objectValue override as suggested by Hoss Man ? @Override public Object objectVal( int doc) { return exists(doc) ? boolVal(doc) : null ; }
          Hide
          dsmiley David Smiley added a comment -

          Sure – trivial enough. Unless there are further suggestions on this issue, I'll commit it with that change later this week. I'll update Lucene & Solr's CHANGES.txt since both get something here.

          Show
          dsmiley David Smiley added a comment - Sure – trivial enough. Unless there are further suggestions on this issue, I'll commit it with that change later this week. I'll update Lucene & Solr's CHANGES.txt since both get something here.
          Hide
          softwaredoug Doug Turnbull added a comment -

          +1

          Show
          softwaredoug Doug Turnbull added a comment - +1
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d12b93e2729036b0c04621114429c25739499243 in lucene-solr's branch refs/heads/master from David Smiley
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d12b93e ]

          SOLR-9279: new function queries: gt, gte, lt, lte, eq
          Lucene Queries module: new ComparisonBoolFunction base class

          Show
          jira-bot ASF subversion and git services added a comment - Commit d12b93e2729036b0c04621114429c25739499243 in lucene-solr's branch refs/heads/master from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d12b93e ] SOLR-9279 : new function queries: gt, gte, lt, lte, eq Lucene Queries module: new ComparisonBoolFunction base class
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit c434eff828702f32251c3a225623b65ae869ea82 in lucene-solr's branch refs/heads/branch_6x from David Smiley
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c434eff ]

          SOLR-9279: new function queries: gt, gte, lt, lte, eq
          Lucene Queries module: new ComparisonBoolFunction base class
          (cherry picked from commit d12b93e)

          Show
          jira-bot ASF subversion and git services added a comment - Commit c434eff828702f32251c3a225623b65ae869ea82 in lucene-solr's branch refs/heads/branch_6x from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c434eff ] SOLR-9279 : new function queries: gt, gte, lt, lte, eq Lucene Queries module: new ComparisonBoolFunction base class (cherry picked from commit d12b93e)
          Hide
          dsmiley David Smiley added a comment -

          BTW I didn't need to override objectVal as specified as the superclass implements it that way.

          Thanks for the contribution Doug!

          Show
          dsmiley David Smiley added a comment - BTW I didn't need to override objectVal as specified as the superclass implements it that way. Thanks for the contribution Doug!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dsmiley commented on the issue:

          https://github.com/apache/lucene-solr/pull/49

          I forgot to use the magic works in the commit to close this. Please close it Doug.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/49 I forgot to use the magic works in the commit to close this. Please close it Doug.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user softwaredoug closed the pull request at:

          https://github.com/apache/lucene-solr/pull/49

          Show
          githubbot ASF GitHub Bot added a comment - Github user softwaredoug closed the pull request at: https://github.com/apache/lucene-solr/pull/49
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d12b93e2729036b0c04621114429c25739499243 in lucene-solr's branch refs/heads/apiv2 from David Smiley
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d12b93e ]

          SOLR-9279: new function queries: gt, gte, lt, lte, eq
          Lucene Queries module: new ComparisonBoolFunction base class

          Show
          jira-bot ASF subversion and git services added a comment - Commit d12b93e2729036b0c04621114429c25739499243 in lucene-solr's branch refs/heads/apiv2 from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d12b93e ] SOLR-9279 : new function queries: gt, gte, lt, lte, eq Lucene Queries module: new ComparisonBoolFunction base class
          Hide
          dsmiley David Smiley added a comment -

          Documented in ref guide: https://cwiki.apache.org/confluence/display/solr/Function+Queries (after refactoring out the boolean functions into a separate table)

          Show
          dsmiley David Smiley added a comment - Documented in ref guide: https://cwiki.apache.org/confluence/display/solr/Function+Queries (after refactoring out the boolean functions into a separate table)
          Hide
          mikemccand Michael McCandless added a comment -

          Bulk close resolved issues after 6.2.0 release.

          Show
          mikemccand Michael McCandless added a comment - Bulk close resolved issues after 6.2.0 release.

            People

            • Assignee:
              dsmiley David Smiley
              Reporter:
              softwaredoug Doug Turnbull
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development