search

googleapis / python-ndb

Apache License 2.0
150 stars 66 forks source link

NDB: Queries with != filter are broken in the last version. #962

Closed alexrwix closed 6 months ago

alexrwix commented 8 months ago

Environment: OS: macOs Sonoma 14.2.1 Python 3.11 google-cloud-ndb==2.3.0

@sorced-jim Looks like queries with != filter were broken in https://github.com/googleapis/python-ndb/pull/950 PR (Use server side != for queries).

The issue is that in model.py function def ne(self, value): """FilterNode: Represents the != comparison.""" return self._comparison("!=", value)

parameter server_op=True was not passed to the query.FilterNode function threw the _comparison(self, op, value) function.

As a result queries with != filter are failing.

Steps to reproduce:

class TestModel(ndb.Model):
    int_prop = ndb.IntegerProperty()
    str_prop = ndb.StringProperty()

query = TestModel.query(TestModel.int_prop != 2)
results = query.fetch()

...

When running the test query.fetch() is failing with the error google.api_core.exceptions.RetryError: Maximum number of 3 retries exceeded while calling <function make_call..rpc_call at 0x1032d99e0>, last exception: None 

[datastore] Mar 07, 2024 10:25:43 AM io.grpc.internal.SerializingExecutor run
[datastore] SEVERE: Exception while executing runnable io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed@33a9178e
[datastore] java.lang.IllegalArgumentException: Unable to perform filter using operator 9
[datastore]     at com.google.appengine.api.datastore.FilterMatcher.addFilter(FilterMatcher.java:216)
[datastore]     at com.google.appengine.api.datastore.BaseEntityComparator.makeFilterMatchers(BaseEntityComparator.java:117)
[datastore]     at com.google.appengine.api.datastore.BaseEntityComparator.<init>(BaseEntityComparator.java:55)
[datastore]     at com.google.appengine.api.datastore.EntityProtoComparators$EntityProtoComparator.<init>(EntityProtoComparators.java:52)
[datastore]     at com.google.cloud.datastore.emulator.impl.LocalDatastoreFileStub.runQuery(LocalDatastoreFileStub.java:341)
[datastore]     at com.google.cloud.datastore.emulator.impl.CloudDatastoreV1.runQuery(CloudDatastoreV1.java:627)
[datastore]     at com.google.cloud.datastore.emulator.v1.DatastoreV1Emulator.runQuery(DatastoreV1Emulator.java:64)
[datastore]     at com.google.cloud.datastore.emulator.v1.DatastoreEmulatorGrpcAdapter$1.runQuery(DatastoreEmulatorGrpcAdapter.java:87)
[datastore]     at com.google.datastore.v1.DatastoreGrpc$MethodHandlers.invoke(DatastoreGrpc.java:695)
[datastore]     at io.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:182)
[datastore]     at io.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:346)
[datastore]     at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:860)
[datastore]     at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)
[datastore]     at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:133)
[datastore]     at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
[datastore]     at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
[datastore]     at java.base/java.lang.Thread.run(Thread.java:834)
sorced-jim commented 8 months ago

Weird, there is a system test for this already:

@pytest.mark.usefixtures("client_context")
def test_filter_not_equal(ds_entity):
    for i in range(5):
        entity_id = test_utils.system.unique_resource_id()
        ds_entity(KIND, entity_id, foo=i)

    class SomeKind(ndb.Model):
        foo = ndb.IntegerProperty()

    eventually(SomeKind.query().fetch, length_equals(5))

    query = SomeKind.query(SomeKind.foo != 2)
    results = query.fetch()
    results = sorted(results, key=operator.attrgetter("foo"))
    assert [entity.foo for entity in results] == [0, 1, 3, 4]
sorced-jim commented 8 months ago

What version of the emulator is being used? Try using the Datastore mode emulator gcloud emulators firestore start --database-mode=datastore-mode

alexrwix commented 8 months ago

@sorced-jim Current versions: Google Cloud SDK 467.0.0 cloud-datastore-emulator 2.3.1

We used: gcloud beta emulators datastore start --no-store-on-disk --host-port=127.0.0.1:9090 --consistency=1.0 -q

Getting the same errors also with gcloud emulators firestore start --database-mode=datastore-mode --host-port=127.0.0.1:9090 -q

sorced-jim commented 8 months ago

What's the Firestore emulator you are using? I tested with cloud-firestore-emulator 1.19.2 and the not equal tests pass.

alexrwix commented 8 months ago

The current Cloud Firestore Emulator version is 1.19.2 Was installed as a part of Google Cloud SDK 467.0.0

sorced-jim commented 8 months ago

What's the stack trace for the Firestore in Datasatore mode emulator? It should be different from the Cloud Datastore emulator.

alexrwix commented 8 months ago 
Waiting for emulator to launch on port 9090...
Connection to 127.0.0.1 port 9090 [tcp/websm] succeeded!
Executing: /Users/alexr/google-cloud-sdk/platform/cloud-firestore-emulator/cloud_firestore_emulator start --host=127.0.0.1 --port=9090 --database-mode=datastore-mode
sorced-jim commented 8 months ago

So there is no stack trace from the emulator with the != query?

alexrwix commented 8 months ago

That's what we have on the failed test

======================================================================
ERROR: test_query_filter_not_equal (wix_privatemedia_service_tests.ndb.test_ndb_query.TestNDBQuery.test_query_filter_not_equal)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/alexr/dev/wix/wix-privatemedia-service/tests/wix_privatemedia_service_tests/utils/ndb.py", line 12, in wrapper
    return func(self, *args, **kw)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/tests/wix_privatemedia_service_tests/ndb/test_ndb_query.py", line 123, in test_query_filter_not_equal
    equal_results = query.fetch()
                    ^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/query.py", line 1202, in wrapper
    return wrapped(self, *dummy_args, _options=query_options)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/utils.py", line 118, in wrapper
    return wrapped(*args, **new_kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/utils.py", line 150, in positional_wrapper
    return wrapped(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/query.py", line 1744, in fetch
    return self.fetch_async(_options=kwargs["_options"]).result()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/tasklets.py", line 210, in result
    self.check_success()
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/tasklets.py", line 157, in check_success
    raise self._exception
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/tasklets.py", line 319, in _advance_tasklet
    yielded = self.generator.throw(type(error), error, traceback)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/_datastore_query.py", line 116, in fetch
    while (yield results.has_next_async()):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/tasklets.py", line 319, in _advance_tasklet
    yielded = self.generator.throw(type(error), error, traceback)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/_datastore_query.py", line 343, in has_next_async
    yield self._next_batch()  # First time
    ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/tasklets.py", line 319, in _advance_tasklet
    yielded = self.generator.throw(type(error), error, traceback)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/_datastore_query.py", line 373, in _next_batch
    response = yield _datastore_run_query(query)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/tasklets.py", line 319, in _advance_tasklet
    yielded = self.generator.throw(type(error), error, traceback)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/_datastore_query.py", line 1030, in _datastore_run_query
    response = yield _datastore_api.make_call(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/tasklets.py", line 323, in _advance_tasklet
    yielded = self.generator.send(send_value)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexr/dev/wix/wix-privatemedia-service/run/lib/python3.11/site-packages/google/cloud/ndb/_retry.py", line 107, in retry_wrapper
    raise core_exceptions.RetryError(
google.api_core.exceptions.RetryError: Maximum number of 3 retries exceeded while calling <function make_call.<locals>.rpc_call at 0x14235e7a0>, last exception: None
sorced-jim commented 8 months ago

I can reproduce this error with the Datastore emulator (gcloud beta emulators datastore start), but not with the Firestore emulator. Please recheck your environment variables aren't being override in your test set up.

Niorlys commented 8 months ago

I am having the same problem with the new release 2.3.0

tlatin commented 7 months ago

So will it no longer be possible to use the datastore emulator locally if we use '!=' operators? Only the firestore emulator in datastore-mode will work?

tlatin commented 7 months ago

The firestore emulator in datastore-mode doesn't currently support resetting the database. https://github.com/firebase/firebase-tools/issues/6902

@sorced-jim Is there a path to continue to support the datastore emulator?

aetherknight commented 7 months ago

Using the firestore emulator is not a replacement for the datastore emulator for us as well during testing. It also doesn't support any way to validate/test composite indexes, not even in firestore mode: https://github.com/firebase/firebase-tools/issues/2027

michaelkedar commented 7 months ago

I've run into this problem as well. As have been mentioned, it's happened because the Datastore emulator does not support != queries which #950 introduced server-side.

Using the server-side != has actually also unintentionally changed the behaviour of the queries on repeated fields. Previously, x != y would become x < y OR x > y, which would de-duplicate the results since it uses an OR query (according to these legacy docs (see note above linked heading)). The server-side != doesn't perform de-duplication (which we were implicitly relying on).

For now, since we don't use != in many places, my solution is to replace the x != y with a ndb.OR(x < y, x > y), which restores the old behaviour.

usmanmani1122 commented 6 months ago

The firestore emulator in datastore-mode doesn't currently support resetting the database. firebase/firebase-tools#6902

@tlatin why don't you use the --data-dir cli parameter to control the emulator data file path and just delete that file for resetting the database?

tlatin commented 6 months ago

@tlatin why don't you use the --data-dir cli parameter to control the emulator data file path and just delete that file for resetting the database?

The feature was added to the firestore emulator in data-store mode per the linked issue in your comment (https://github.com/firebase/firebase-tools/issues/6902).

In general I wouldn't want to put the filesystem on the critical path of the hundred of tests. Also the previous datastore emulator supported the feature, so it's reasonable to request parity when being asked to migrate to a new solution.

usmanmani1122 commented 6 months ago

In general I wouldn't want to put the filesystem on the critical path of the hundred of tests. Also the previous datastore emulator supported the feature, so it's reasonable to request parity when being asked to migrate to a new solution.

You are right that the feature should be there. But I am just suggesting a work around. And --data-dir only gives you the control over the path of the database file but it must be using a location on the filesystem by default.

sorced-jim commented 6 months ago

The bug has two work around and those are documented on https://github.com/googleapis/python-ndb/wiki/Upgrade-Notes .