[
https://issues.apache.org/jira/browse/CASSANDRA-6377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15172316#comment-15172316
]
Sam Tunnicliffe commented on CASSANDRA-6377:
--------------------------------------------
The patches mostly look good to me, just a few minor comments:
2.2 patch:
* In {{SelectStatement::getValidatedIndexExpressions}} we skip validation for
Thrift static tables because they allow filtering on non-indexed columns. I
think we should probably still call {{SIM::validateIndexSearchersForQuery}} for
such tables so that we validate any expressions that do relate to indexed
columns. The {{isDense}} & {{isCompound}} checks would move into
{{validateIndexSearchersForQuery}} to suppress the {{IRE}} when appropriate.
The test removed from {{PerRowSecondaryIndexTest}} could be added back then too.
* It might be good to have a condition in
{{SelectTest::testFilteringWithoutIndices}} using a static column, just to
demonstrate expected behaviour
Out of curiosity, why did you add the tombstone handling conditions in
{{testFilteringOnCompactTablesWithoutIndices}}? FTR they look correct, they
just don't seem pertinent to this ticket, so may obscure the intent of the test
a little, but perhaps I'm missing something. (There's also a couple of typos
there - s/tomstones/tombstones).
Nits:
* in {{SIM::getColumnNames}} the {{SecondaryIndexManager}} qualifier is
unnecessary in the call to {{getColumnName}}
* can you make the line breaking in {{SelectTest::testFilteringWithoutIndices}}
consistent please?
* some of the assertions in {{SelectTest}} use
{{StatementRestrictions.REQUIRES_ALLOW_FILTERING_MESSAGE}} but others do not
(where they could/should)
3.0 patch:
Ideally, I think we could remove {{RowFilter.Expression::validateForIndexing}}
completely, and move the check to {{CassandraIndex::searcherFor}}. This isn't
used in any of the index implementations that I'm aware of, but it's probably
not cool to remove an external API method like this unannounced. How about copy
the logic to {{CassandraIndex}} and marking it {{@Deprecated}}? (applies to
trunk as well).
trunk patch:
I believe that in {{RowFilter.CQLFilter}}'s {{IsSatisfiedFilter}}, the second
{{applyToRow(partition.staticRow())}} call is redundant. We could remove the
first call, in line with the 3.0 patch, but splitting the 2 checks potentially
avoids an unnecessary transformation.
I've pushed a commit
[here|https://github.com/beobal/cassandra/commit/074c376bfa61f9e828a3515e053da191bae042d5]
to demonstrate. If you agree, the same should apply to 3.0
> ALLOW FILTERING should allow seq scan filtering
> -----------------------------------------------
>
> Key: CASSANDRA-6377
> URL: https://issues.apache.org/jira/browse/CASSANDRA-6377
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
> Reporter: Jonathan Ellis
> Assignee: Benjamin Lerer
> Labels: cql
> Fix For: 3.0.x, 3.x
>
>
> CREATE TABLE emp_table2 (
> empID int PRIMARY KEY,
> firstname text,
> lastname text,
> b_mon text,
> b_day text,
> b_yr text,
> );
> INSERT INTO emp_table2 (empID,firstname,lastname,b_mon,b_day,b_yr)
> VALUES (100,'jane','doe','oct','31','1980');
> INSERT INTO emp_table2 (empID,firstname,lastname,b_mon,b_day,b_yr)
> VALUES (101,'john','smith','jan','01','1981');
> INSERT INTO emp_table2 (empID,firstname,lastname,b_mon,b_day,b_yr)
> VALUES (102,'mary','jones','apr','15','1982');
> INSERT INTO emp_table2 (empID,firstname,lastname,b_mon,b_day,b_yr)
> VALUES (103,'tim','best','oct','25','1982');
>
> SELECT b_mon,b_day,b_yr,firstname,lastname FROM emp_table2
> WHERE b_mon='oct' ALLOW FILTERING;
> Bad Request: No indexed columns present in by-columns clause with Equal
> operator
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
