Skip to content

Conversation

@michaelsembwever
Copy link
Member

https://github.com/riptano/cndb/issues/16020

Port into main-5.0 commit 0c97397

CNDB-16020: CNDB-15760: Fix AbstractReadQuery.toCQLString (#2083)

Fix AbstractReadQuery.toCQLString to produce output that is as close to valid CQL as possible. The fixes here mainly come from CASSANDRA-16510, and there is also some removal of code duplication.

@github-actions
Copy link

github-actions bot commented Dec 3, 2025

Checklist before you submit for review

  • This PR adheres to the Definition of Done
  • Make sure there is a PR in the CNDB project updating the Converged Cassandra version
  • Use NoSpamLogger for log lines that may appear frequently in the logs
  • Verify test results on Butler
  • Test coverage for new/modified code is > 80%
  • Proper code formatting
  • Proper title for each commit staring with the project-issue number, like CNDB-1234
  • Each commit has a meaningful description
  • Each commit is not very long and contains related changes
  • Renames, moves and reformatting are in distinct commits
  • All new files should contain the DataStax copyright header instead of the Apache License one

operator = first.endInclusive ? Operator.LTE : Operator.LT;
sb.append(' ').append(operator).append(' ')
.append(column.type.toCQLString(first.endValue));
rowFilter = rowFilter.without(column, operator, first.endValue);
Copy link
Member Author

Choose a reason for hiding this comment

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

question @adelapena

in CASSANDRA-16510 you introduced rowFilter parameter to toCQLString()

and in Slices.ArrayBackedSlices.toCQLString() many rowFilter = rowFilter.without(…) lines.
( like the one highlighted )

but in CNDB-15760
0c97397
when you introduced the rowFilter parameter you didn't add those rowFilter = rowFilter.without(…) lines…

should they be there or not… ?

Copy link

@adelapena adelapena Dec 5, 2025

Choose a reason for hiding this comment

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

They should be there. Those rowFilter.withoutFirstLevelExpression calls are there to address a problem of duplicated expressions in the output of toCQLString that I think only happens with index-based ORDER BY. That feature doesn't exist on ASF, so it shouldn't be an issue there.

There are repro tests for ClusteringIndexNamesFilter, but unfortunately I missed to add one for Slices. Here is one:

assertToCQLString("SELECT * FROM %s WHERE k = 0 AND c > 0 ORDER BY n LIMIT 10",
                  "SELECT * FROM %s WHERE k = 0 AND c > 0 ORDER BY n ASC LIMIT 10 ALLOW FILTERING",
                  "SELECT * FROM %s WHERE k = ? AND c > ? ORDER BY n ASC LIMIT 10 ALLOW FILTERING");

If we didn't have the without call in the slices, it would have wrongly returned:

SELECT * FROM %s WHERE k = 0 AND c > 0 AND c > 0 ORDER BY n ASC LIMIT 10 ALLOW FILTERING

Copy link
Member Author

Choose a reason for hiding this comment

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

added in bed2448

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 5, 2025

Fix AbstractReadQuery.toCQLString to produce output that is as close to valid CQL as possible.
The fixes here mainly come from CASSANDRA-16510, and there is also some removal of code duplication.
@cassci-bot
Copy link

❌ Build ds-cassandra-pr-gate/PR-2148 rejected by Butler


1 regressions found
See build details here


Found 1 new test failures

Test Explanation Runs Upstream
o.a.c.cql3.validation.operations.AggregationQueriesTest.testAggregationQueryShouldNotTimeoutWhenItExceedesReadTimeout (compression) REGRESSION 🔴🔴 2 / 24

Found 7 known test failures

@michaelsembwever michaelsembwever merged commit 283f8ec into main-5.0 Dec 7, 2025
575 of 591 checks passed
@michaelsembwever michaelsembwever deleted the mck-cndb-15760-main-5.0 branch December 7, 2025 16:38
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.

5 participants