Support for NOT / NOT IN operator #139

rposkocil · 2017-01-04T23:38:57Z

fixes #METAMODEL-1134
https://issues.apache.org/jira/browse/METAMODEL-1134

- added filter attribute for negation - added new negation operator - added test of it

kaspersorensen · 2017-01-08T13:52:54Z

Your PR suggests to solve the problem in two ways, and that seems to me to be one too many. One way is with the NegationOperator which is in a way nice and general, but introduces some redundancy since there are also operators available for "different from". And it is made more obvious with the other solution that you introduce at the same time: Introducing the "not in" operator. Honestly I think that from an API evolution point of view, the second approach ("not in" operator) is the best. Introducing a new generic concept like negation into FilterItem is going to have a lot more side-effects on existing query evaluation. I would just add this one new operator and not handle negation generically.

rposkocil · 2017-01-08T21:14:41Z

Correct me, please @kaspersorensen, but NegationOperator is much more general than different from and additing another way to propositional logic of filters. Operator Different from is used just for childs of filters and it's represented with <>. For example SQL NOT keyword has a wider usage. You can negate also composed filters on all levels or different from, too. This functionality I didn't found there and thought that it could be fine.
There is no way to negate IN construct in the currecnt version of MetaModel. That's why Hans created this requirement. There can be IN and NOT IN operators like = and <>. A similiar case.
So both parts make me sense and don't brake the concept. But I understand that there could be other problems that's why constructors were doubled with negationa and without.

kaspersorensen · 2017-01-09T01:04:25Z

Yes but having two ways of doing it means that we can now also have "NOT NOT_IN" (same as IN) and "NOT DIFFERENT_FROM" (same as EQUALS_TO) which to me seems like we've done our design wrong. I agree that ideally we would then just have the negation option, but that train has already left the station since we have negations built in to DIFFERENT_FROM. I also wonder why you would want to introduce NOT_IN if you really think we should be using the negation option instead?

kaspersorensen · 2017-01-09T01:08:36Z

Alternatively we should deprecate DIFFERENT_FROM and not introduce NOT_IN. That would technically speaking be fine. But my last argument is that from an evolution perspective, it would be worse, because it forces a lot of existing implementations to evaluate a new attribute which was not there before. It would implicitly introduce a ton of bugs because data contexts out there would no longer execute queries correctly.

rposkocil · 2017-01-09T16:37:51Z

I was thinking about it and for a complete option to be able to express all, what propositional logic allows, it should be enough so that we will add NOT IN and NOT LIKE operators. All operators should have a positive and negative form then every conditions can be transfomed to allowed constructions.
The next point is that I have forgoten on the fluent api for queries where the operators have to impletented, too.

Do you agree with this solution?

kaspersorensen · 2017-01-09T17:45:05Z

Yes that sounds correct to me. I think this is much more manageable for existing implementations which typically do something like this:

if (operator == EQUALS_TO) {
  return somethingUsefull();
} else if (operator == LIKE) {
  return somethingElseUsefull();
} else {
  // this is not supported - return null to make MetaModel evaluate the filter item on the client side.
  return null;
}

(the above snippet would still be valid even though other operators are added. If we added the negation option, it would not).

rposkocil · 2017-01-09T22:35:06Z

Prepared by the accepted way.

kaspersorensen · 2017-01-10T08:19:01Z

core/src/main/java/org/apache/metamodel/query/OperatorTypeImpl.java

+                    return OperatorType.LESS_THAN_OR_EQUAL;
+                case "LIKE":
+                    return OperatorType.LIKE;
+                case "NOT_LIKE":


We should definately add "NOT LIKE" here also (with space instead of underscore).

kaspersorensen · 2017-01-10T08:19:30Z

core/src/main/java/org/apache/metamodel/query/builder/AbstractFilterBuilder.java

@@ -64,6 +64,21 @@ public B in(String... strings) {
    }

    @Override
+    public B not_in(Collection<?> values) {


We should keep using regular java method names here (and below), so notIn

kaspersorensen · 2017-01-10T08:19:43Z

core/src/main/java/org/apache/metamodel/query/builder/AbstractFilterBuilder.java

@@ -460,6 +475,15 @@ public B like(String string) {
    }

    @Override
+    public B not_like(String string) {


and notLike here

kaspersorensen · 2017-01-11T07:27:08Z

core/src/main/java/org/apache/metamodel/query/OperatorTypeImpl.java

+                    return OperatorType.LESS_THAN_OR_EQUAL;
+                case "LIKE":
+                    return OperatorType.LIKE;
+                case "NOT LIKE":


Sorry, I meant to add this, not replace the old, so ...

case "NOT_LIKE": case "NOT LIKE": ...

Ok, sorry for all mistakes

rposkocil · 2017-01-11T19:42:27Z

Can you see any other weird things or issues, @kaspersorensen ? Thank you.

kaspersorensen · 2017-01-12T08:00:22Z

LGTM!

kaspersorensen · 2017-01-28T04:17:56Z

Applied the patch to master. Thank you very much @rposkocil and sorry for the long wait (another time you can bump it if you want to reinvigorate our attention ;-))

rposkocil added 2 commits January 4, 2017 23:33

Possible to negate the filter condition

766a7fb

- added filter attribute for negation - added new negation operator - added test of it

New NOT IN operator

2d6f1d0

rposkocil added 3 commits January 9, 2017 21:45

Removed NOT construct

9fd26e8

New NOT LIKE operator

f2ac40d

Added new operators to fluent api and tests

78cd781

kaspersorensen suggested changes Jan 10, 2017

View reviewed changes

fix name convention

ec8ad6f

kaspersorensen reviewed Jan 11, 2017

View reviewed changes

fix operator fixes

c402a88

asfgit closed this in 41a708f Jan 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for NOT / NOT IN operator #139

Support for NOT / NOT IN operator #139

rposkocil commented Jan 4, 2017

kaspersorensen commented Jan 8, 2017

rposkocil commented Jan 8, 2017

kaspersorensen commented Jan 9, 2017

kaspersorensen commented Jan 9, 2017

rposkocil commented Jan 9, 2017

kaspersorensen commented Jan 9, 2017

rposkocil commented Jan 9, 2017

kaspersorensen Jan 10, 2017

kaspersorensen Jan 10, 2017

kaspersorensen Jan 10, 2017

kaspersorensen Jan 11, 2017

rposkocil Jan 11, 2017

rposkocil commented Jan 11, 2017

kaspersorensen commented Jan 12, 2017

kaspersorensen commented Jan 28, 2017

Support for NOT / NOT IN operator #139

Support for NOT / NOT IN operator #139

Conversation

rposkocil commented Jan 4, 2017

kaspersorensen commented Jan 8, 2017

rposkocil commented Jan 8, 2017

kaspersorensen commented Jan 9, 2017

kaspersorensen commented Jan 9, 2017

rposkocil commented Jan 9, 2017

kaspersorensen commented Jan 9, 2017

rposkocil commented Jan 9, 2017

kaspersorensen Jan 10, 2017

Choose a reason for hiding this comment

kaspersorensen Jan 10, 2017

Choose a reason for hiding this comment

kaspersorensen Jan 10, 2017

Choose a reason for hiding this comment

kaspersorensen Jan 11, 2017

Choose a reason for hiding this comment

rposkocil Jan 11, 2017

Choose a reason for hiding this comment

rposkocil commented Jan 11, 2017

kaspersorensen commented Jan 12, 2017

kaspersorensen commented Jan 28, 2017