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

Switch to scylla 6.2 #397

Merged
merged 3 commits into from
Dec 23, 2024
Merged

Switch to scylla 6.2 #397

merged 3 commits into from
Dec 23, 2024

Conversation

dkropachev
Copy link
Collaborator

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@dkropachev dkropachev marked this pull request as draft December 20, 2024 17:19
@dkropachev dkropachev force-pushed the dk/switch-to-scylla-6.2 branch 2 times, most recently from 3eacd55 to 8483fa0 Compare December 20, 2024 19:43
@dkropachev dkropachev marked this pull request as ready for review December 20, 2024 19:44
@dkropachev dkropachev force-pushed the dk/switch-to-scylla-6.2 branch from 8483fa0 to d655cb6 Compare December 20, 2024 19:47
tests/integration/__init__.py Show resolved Hide resolved
tests/integration/cqlengine/query/test_queryset.py Outdated Show resolved Hide resolved
cassandra/cqlengine/management.py Outdated Show resolved Hide resolved
@dkropachev dkropachev force-pushed the dk/switch-to-scylla-6.2 branch 6 times, most recently from 8d48572 to f7b7995 Compare December 22, 2024 12:40
@dkropachev dkropachev requested a review from Lorak-mmk December 22, 2024 15:11
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Looks quite good now!

Nit (you can ignore it): First commit should actually be last to keep the tests passing on each commit.

Comment on lines 823 to 834

for col_name in colnames:
col_name, order = self._get_ordering_condition(col_name)
col_name = '"{0}"'.format(col_name)
# Check if there is existing condition that targets same column and replace it if it's order is different
for cond_id, existing_condition in enumerate(clone._order):
if col_name in existing_condition:
if not existing_condition.endswith(order):
clone._order[cond_id] = '{0} {1}'.format(col_name, order)
break
else:
clone._order.append('{0} {1}'.format(col_name, order))

Choose a reason for hiding this comment

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

Question regarding commit "Fix AbstractQuerySet.order_by to consider existing conditions ":
The root cause of the problem is that Scylla handles such weird queries differently than Cassandra, right? If so, could you add a comment on this code that explains it?

Note: I know nothing about cqlengine code so I don't feel competent to review it and I don't feel like I understand the change. @fruch can you take a look?

Copy link

Choose a reason for hiding this comment

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

this fix not related to cqlengine per se,
seems it cover from some mismatch between scylla and cassandra

it's weird a bit, cassandra accepts this ? what the CQL spec say about it ?

seems not much:
https://cassandra.apache.org/doc/4.0/cassandra/cql/dml.html#ordering-clause

I think we should raise it as issue in scylla, and mark it as compatibility issue with cassandra

Copy link

Choose a reason for hiding this comment

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

That's something the server should be doing/guarding, but for the meanwhile, it's seems o.k. to me that that cqlengine would prevent it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Problem is that scylla does that and fails the query with multiple records of the same column in order by while cassandra ignores it and goes with order mentioned last.

Choose a reason for hiding this comment

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

Problem is that scylla does that and fails the query with multiple records of the same column in order by while cassandra ignores it and goes with order mentioned last.

And that fix makes cqlengine avoid creating such queries, so they are basically ignored like in cassandra, even when executing on Scylla?
Is it the right thing to do here? It fixes the test, but isn't the Scylla behavior better for the user (failing early rather than somehow executing it)?

What do you think about changing / xfailing the test instead?

Copy link
Collaborator Author

@dkropachev dkropachev Dec 23, 2024

Choose a reason for hiding this comment

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

And that fix makes cqlengine avoid creating such queries, so they are basically ignored like in cassandra, even when executing on Scylla? Is it the right thing to do here?
Yes.

It fixes the test, but isn't the Scylla behavior better for the user (failing early rather than somehow executing it)?
Yes, I think scylla behavior is more correct, it does not make sense to have opposite ordering direction for the same column.

What do you think about changing / xfailing the test instead?

As far as understand this test, this is cqlengine test for order_by functionality that generates one query executes it and then reverse order direction and executes it again, so it should work just fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, after thinking about it just a bit, I decided to fix the test instead, commit is dropped.

tests/integration/__init__.py Show resolved Hide resolved
@dkropachev dkropachev force-pushed the dk/switch-to-scylla-6.2 branch from f7b7995 to 17cf722 Compare December 23, 2024 14:43
Copy link

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

Scylla does not support bucket_low higher than 1
At the same time cassandra is ok with from 0 to 10
@dkropachev dkropachev force-pushed the dk/switch-to-scylla-6.2 branch from 17cf722 to 05cbf94 Compare December 23, 2024 20:09
Scylla does not support statement with opposite ordering direction for
the same collumn:
SELECT * FROM table ORDER BY a ASC, a DESC;
@dkropachev dkropachev merged commit 1316cd1 into master Dec 23, 2024
27 of 30 checks passed
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