Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$not operator #760

Merged
merged 25 commits into from
Jan 16, 2024
Merged

$not operator #760

merged 25 commits into from
Jan 16, 2024

Conversation

maheshrajamani
Copy link
Contributor

@maheshrajamani maheshrajamani commented Jan 3, 2024

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@maheshrajamani maheshrajamani requested a review from a team as a code owner January 3, 2024 14:54
@maheshrajamani maheshrajamani self-assigned this Jan 3, 2024
@maheshrajamani maheshrajamani changed the title [WIP] Not operator [WIP] $not operator Jan 3, 2024
@maheshrajamani maheshrajamani changed the title [WIP] $not operator $not operator Jan 3, 2024
return new JsonLiteral<Boolean>(!((Boolean) operand.value()), operand.type());
} else if (operator == ArrayComparisonOperator.SIZE) {
return new JsonLiteral<BigDecimal>(
((BigDecimal) operand.value()).multiply(new BigDecimal(-1)), operand.type());
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is done to flip between 1 and -1, more efficient way would be to use BigDecimal.negate() (https://docs.oracle.com/javase/8/docs/api/java/math/BigDecimal.html#negate-- )

But I guess I am not sure what the logic here really is? Negative values can't match anything, I guess that is the idea?

EDIT: this ^^^ makes sense when at later point negative value is converted to "$size NOT equals X"

Copy link
Contributor

@tatu-at-datastax tatu-at-datastax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: added some questions, but nothing blocking merge.

Copy link
Contributor

@tatu-at-datastax tatu-at-datastax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about why $not is an array. In MongoDB, $not is an object like the following:

find({
  num: {
    $not: {
      $lt: 42
    }
  }
}); // Equivalent to `find({ num: { $gte: 42 } })`

Is there a reason for making $not an array?

@maheshrajamani
Copy link
Contributor Author

I'm curious about why $not is an array. In MongoDB, $not is an object like the following:

I made it in line with $and and $or operator. It does and of conditions below it. Do we have to change this?

@tatu-at-datastax
Copy link
Contributor

I'm curious about why $not is an array. In MongoDB, $not is an object like the following:

I made it in line with $and and $or operator. It does and of conditions below it. Do we have to change this?

Hmmmh. Good point, thank you for pointing it out. It is interesting Mongo has this discrepancy: $and and $or take Array, $not for some reason Object. Go figure.

Earlier we wanted to be highly Mongo-compatible, and I would have suggested we follow its lead.
But now we are moving away from that.
Still, we are mostly extending things and not changing handling of features Mongo has.
So I am not 100% sure which way we should go.

I think we need to discuss this on our daily meeting @maheshrajamani see what Aaron thinks.

@tatu-at-datastax
Copy link
Contributor

Based on discussion with @amorton, I think the preferred option is to use JSON Object, same as how Mongo behaves.
I guess it also makes sense logically, come to think of it: not is typically unary operation whereas and and or are binary. So taking in Array would be more likely implicit "not and" (and possibly more confusing).

@vkarpov15
Copy link
Collaborator

Another reason why we should make $not a unary operator is that Mongoose's filter casting logic assumes $not is unary. In the following script, Mongoose throws a CastError CastError: Cast to number failed for value "[ { '$gt': 42 } ]" (type Array) at path "age" for model "Test":

'use strict';

const mongoose = require('mongoose');

mongoose.set('debug', true);

run().catch(err => console.log(err));

async function run() {
  await mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');

  const schema = new mongoose.Schema({ age: Number });
  const Test = mongoose.model('Test', schema);

  await Test.find({ age: { $not: [{ $gt: 42 }] } });
  console.log('Done');
}

However, await Test.find({ age: { $not: { $gt: 42 } } }); (object rather than array for $not) casts correctly. So if we want to be compatible with what Mongoose expects, then $not should be unary.

@maheshrajamani
Copy link
Contributor Author

@vkarpov15 @tatu-at-datastax Have made the $not to accept one object as value. If you want to do another review of it.

}

@Test
public void multipleNotCancelling() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I'm glad you tested this. I checked whether multiple $not works in MongoDB and Mongoose: MongoDB supports it, but Mongoose doesn't. Something for Mongoose to fix.

@maheshrajamani maheshrajamani merged commit a9071c7 into main Jan 16, 2024
2 checks passed
@maheshrajamani maheshrajamani deleted the not-operator branch January 16, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants