Skip to content

PYTHON-968: Support IS NOT NULL operator in cqlengine #942

Merged
beltran merged 7 commits into
masterfrom
python-968-2
Apr 12, 2018
Merged

PYTHON-968: Support IS NOT NULL operator in cqlengine #942
beltran merged 7 commits into
masterfrom
python-968-2

Conversation

@beltran
Copy link
Copy Markdown
Contributor

@beltran beltran commented Apr 12, 2018

No description provided.

@beltran beltran requested a review from mambocab April 12, 2018 15:13
Copy link
Copy Markdown
Contributor

@mambocab mambocab left a comment

Choose a reason for hiding this comment

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

I've pushed a few small commits -- go ahead and squash into yours, just for history readability. The nit can be ignored if you like, I was just wondering if this could be sped up at all.

The caveat to all this is that I don't understand the update_context or get_context_size methods at all, so I'm just trusting that the tests catch any problems there would be with that.



class TestIsNotNull(BaseCassEngTestCase):
def test_is_not_null_to_cql(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Could this be a unit test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It definitely can, but we don't have unit tests for cqlengine, so I figured at some point we didn't want to create a new folder just for a few tests. They don't start C* so they don't add more time to the run for being there.
If we moved them to unit I'd do so in a future PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 awesome. filed a low-priority ticket: https://datastax-oss.atlassian.net/browse/PYTHON-975

@beltran
Copy link
Copy Markdown
Contributor Author

beltran commented Apr 12, 2018

They track count of how many parameters have been processed because the way cqlengine builds the queries is:
SELECT clolumn1, column2, ... from keyspace.table WHERE A = %(0)s AND B = %(1)s, parameters = {0: "parameter for A", 1: "parameter for B"}.
Using 0, 1, 2, ... to track the parameters.

So each clause need to know how many parameters the clauses from before have taken so they can put the "0" or the "1" or the number that it corresponds to them. Since IS NOT NULL uses zero get_context_size returns 0 and in update_context the context is not updated since it doesn't affect the global count nor uses parameters.

@mambocab
Copy link
Copy Markdown
Contributor

Ohhh, cool. Thanks for the explanation.

@mambocab
Copy link
Copy Markdown
Contributor

Looks great, +1 for merging.

@beltran
Copy link
Copy Markdown
Contributor Author

beltran commented Apr 12, 2018

Awesome, thank you for the review!

@beltran beltran merged commit 4827711 into master Apr 12, 2018
@beltran beltran deleted the python-968-2 branch April 12, 2018 19:07

q = Automobile.objects.filter(model__like='%Civic%').allow_filtering()

:attr:`IS NOT NULL (IsNotNull(column_name)) <query.QueryOperator.IsNotNullOperator>`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should probably point to the Clause and not the Operator. In this case, I think the Operator can't be used directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's correct, the clause has to be used for this case. I'll change it

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