[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14227943#comment-14227943
]
Benjamin Lerer commented on CASSANDRA-7981:
---
The 2 tests that does not have {{COMPACT STORAGE}} are new ones that test
secondary indices with multi-column relation. They do not use {{COMPACT
STORAGE}} simply because secondary indices are not supported on clustering
columns with compact storage.
Refactor SelectStatement
Key: CASSANDRA-7981
URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
Project: Cassandra
Issue Type: Bug
Reporter: Benjamin Lerer
Assignee: Benjamin Lerer
Fix For: 3.0
The current state of the code of SelectStatement make fixing some issues or
adding new functionnalities really hard. It also contains some
functionnalities that we would like to reuse in ModificationStatement but
cannot for the moment.
Ideally I would like to:
* Perform as much validation as possible on Relations instead of performing
it on Restrictions as it will help for problem like the one of
#CASSANDRA-6075 (I believe that by clearly separating validation and
Restrictions building we will also make the code a lot clearer)
* Provide a way to easily merge restrictions on the same columns as needed
for #CASSANDRA-7016
* Have a preparation logic (validation + pre-processing) that we can easily
reuse for Delete statement #CASSANDRA-6237
* Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[ https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14224874#comment-14224874 ] Benjamin Lerer commented on CASSANDRA-7981: --- I have rebased my refactoring and added new tests but it does seems that the coverage did not take it into account. Refactor SelectStatement Key: CASSANDRA-7981 URL: https://issues.apache.org/jira/browse/CASSANDRA-7981 Project: Cassandra Issue Type: Bug Reporter: Benjamin Lerer Assignee: Benjamin Lerer Fix For: 3.0 The current state of the code of SelectStatement make fixing some issues or adding new functionnalities really hard. It also contains some functionnalities that we would like to reuse in ModificationStatement but cannot for the moment. Ideally I would like to: * Perform as much validation as possible on Relations instead of performing it on Restrictions as it will help for problem like the one of #CASSANDRA-6075 (I believe that by clearly separating validation and Restrictions building we will also make the code a lot clearer) * Provide a way to easily merge restrictions on the same columns as needed for #CASSANDRA-7016 * Have a preparation logic (validation + pre-processing) that we can easily reuse for Delete statement #CASSANDRA-6237 * Make the code much easier to read and safer to modify. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14225222#comment-14225222
]
Tyler Hobbs commented on CASSANDRA-7981:
I was mistaken, that Jenkins job is scheduled periodically instead of being
triggered by new commits. From #cassandra-dev, you can trigger the test run by
doing this: {{/msg cassci !build scratch-7981_coverage_unit_and_dtest}}. I
started a new run earlier today, but it's slow.
In the meantime, one comment on the latest changes: it looks like you
(accidentally?) removed the {{COMPACT STORAGE}} cases for a couple of
MultiColumnRelationTest methods.
Refactor SelectStatement
Key: CASSANDRA-7981
URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
Project: Cassandra
Issue Type: Bug
Reporter: Benjamin Lerer
Assignee: Benjamin Lerer
Fix For: 3.0
The current state of the code of SelectStatement make fixing some issues or
adding new functionnalities really hard. It also contains some
functionnalities that we would like to reuse in ModificationStatement but
cannot for the moment.
Ideally I would like to:
* Perform as much validation as possible on Relations instead of performing
it on Restrictions as it will help for problem like the one of
#CASSANDRA-6075 (I believe that by clearly separating validation and
Restrictions building we will also make the code a lot clearer)
* Provide a way to easily merge restrictions on the same columns as needed
for #CASSANDRA-7016
* Have a preparation logic (validation + pre-processing) that we can easily
reuse for Delete statement #CASSANDRA-6237
* Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14219394#comment-14219394
]
Benjamin Lerer commented on CASSANDRA-7981:
---
The new fixes are there: [branch |
https://github.com/blerer/cassandra/compare/CASSANDRA-7981]
{quote}It looks like the mergeWith() conflict is still present. For some
reason, the ant build doesn't normally complain, but my IDE does. Either way,
it's a legit problem that needs to be fixed.{quote}
It is a perfectly valid thing from the Java point of view but I agree that it
is a bit of a hack.
I have tried to simplify a bit the hierachy by having
{{SingleColumnPrimaryKeyRestrictions}} using an instance of
{{SingleColumnRestrictions}} instead of extending it. This as allowed me to
remove the {{mergeWith()}} problem.
{quote}Can you explain why that's correct (and add a comment)?{quote}
I cannot. This code is completly wrong.
Hopefully it should now be fixed. Have a look at the new unit tests and tell me
if they look fine to you.
Refactor SelectStatement
Key: CASSANDRA-7981
URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
Project: Cassandra
Issue Type: Bug
Reporter: Benjamin Lerer
Assignee: Benjamin Lerer
Fix For: 3.0
The current state of the code of SelectStatement make fixing some issues or
adding new functionnalities really hard. It also contains some
functionnalities that we would like to reuse in ModificationStatement but
cannot for the moment.
Ideally I would like to:
* Perform as much validation as possible on Relations instead of performing
it on Restrictions as it will help for problem like the one of
#CASSANDRA-6075 (I believe that by clearly separating validation and
Restrictions building we will also make the code a lot clearer)
* Provide a way to easily merge restrictions on the same columns as needed
for #CASSANDRA-7016
* Have a preparation logic (validation + pre-processing) that we can easily
reuse for Delete statement #CASSANDRA-6237
* Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[ https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14219621#comment-14219621 ] Tyler Hobbs commented on CASSANDRA-7981: I should be able to review the latest changes today, but for now, we finally have some test coverage analysis (unit tests + dtests): https://cassci.datastax.com/job/scratch-7981_coverage_unit_and_dtest/JaCoCo_Coverage_Report/. Since trunk still has problems with some of the tests, there are a lot of failures, but that should give you a pretty good idea of the test coverage on the new code. It looks like there are a few spots in the new code that could use some additional tests. Refactor SelectStatement Key: CASSANDRA-7981 URL: https://issues.apache.org/jira/browse/CASSANDRA-7981 Project: Cassandra Issue Type: Bug Reporter: Benjamin Lerer Assignee: Benjamin Lerer Fix For: 3.0 The current state of the code of SelectStatement make fixing some issues or adding new functionnalities really hard. It also contains some functionnalities that we would like to reuse in ModificationStatement but cannot for the moment. Ideally I would like to: * Perform as much validation as possible on Relations instead of performing it on Restrictions as it will help for problem like the one of #CASSANDRA-6075 (I believe that by clearly separating validation and Restrictions building we will also make the code a lot clearer) * Provide a way to easily merge restrictions on the same columns as needed for #CASSANDRA-7016 * Have a preparation logic (validation + pre-processing) that we can easily reuse for Delete statement #CASSANDRA-6237 * Make the code much easier to read and safer to modify. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[ https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14216909#comment-14216909 ] Tyler Hobbs commented on CASSANDRA-7981: bq. The refactoring break the IN ordering. The results are now returned ordered in the normal order not in the order specified in the IN condition. Ah, yes. Can you update NEWS.txt to point this out in the upgrading section? Refactor SelectStatement Key: CASSANDRA-7981 URL: https://issues.apache.org/jira/browse/CASSANDRA-7981 Project: Cassandra Issue Type: Bug Reporter: Benjamin Lerer Assignee: Benjamin Lerer Fix For: 3.0 The current state of the code of SelectStatement make fixing some issues or adding new functionnalities really hard. It also contains some functionnalities that we would like to reuse in ModificationStatement but cannot for the moment. Ideally I would like to: * Perform as much validation as possible on Relations instead of performing it on Restrictions as it will help for problem like the one of #CASSANDRA-6075 (I believe that by clearly separating validation and Restrictions building we will also make the code a lot clearer) * Provide a way to easily merge restrictions on the same columns as needed for #CASSANDRA-7016 * Have a preparation logic (validation + pre-processing) that we can easily reuse for Delete statement #CASSANDRA-6237 * Make the code much easier to read and safer to modify. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14217032#comment-14217032
]
Tyler Hobbs commented on CASSANDRA-7981:
bq. Did not found a way to simplify it but I am also not fully sure about what
it is making it confusing.
I think it's primarily the classes that implement both Restriction and
Restrictions (AbstractPrimaryKeyRestrictions,
SingleColumnPrimaryKeyRestrictions). It makes some sense to me now, but I had
to diagram it.
It looks like the {{mergeWith()}} conflict is still present. For some reason,
the ant build doesn't normally complain, but my IDE does. Either way, it's a
legit problem that needs to be fixed.
I don't understand this piece of code in {{MultiColumnRestriction.Slice}}, and
it seems incorrect to me:
{code}
protected boolean isSupportedBy(ColumnDefinition columnDef, SecondaryIndex
index)
{
if (columnDefs.indexOf(columnDef) != (columnDefs.size() - 1))
return index.supportsOperator(Operator.EQ);
return slice.isSupportedBy(index);
}
{code}
Can you explain why that's correct (and add a comment)?
I'm still working on getting the test coverage analysis.
Refactor SelectStatement
Key: CASSANDRA-7981
URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
Project: Cassandra
Issue Type: Bug
Reporter: Benjamin Lerer
Assignee: Benjamin Lerer
Fix For: 3.0
The current state of the code of SelectStatement make fixing some issues or
adding new functionnalities really hard. It also contains some
functionnalities that we would like to reuse in ModificationStatement but
cannot for the moment.
Ideally I would like to:
* Perform as much validation as possible on Relations instead of performing
it on Restrictions as it will help for problem like the one of
#CASSANDRA-6075 (I believe that by clearly separating validation and
Restrictions building we will also make the code a lot clearer)
* Provide a way to easily merge restrictions on the same columns as needed
for #CASSANDRA-7016
* Have a preparation logic (validation + pre-processing) that we can easily
reuse for Delete statement #CASSANDRA-6237
* Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14214469#comment-14214469
]
Benjamin Lerer commented on CASSANDRA-7981:
---
I updated the
[branch|https://github.com/blerer/cassandra/compare/CASSANDRA-7981].
The patch address most of the review comments.
{quote}The restriction class hierarchy is confusing, but I don't see an obvious
way to simplify it{quote}
Did not found a way to simplify it but I am also not fully sure about what it
is making it confusing. I probably have worked already too much with it. May be
[~slebresne] will have an idea of how we can improve the things.
{quote}There seems to be a conflict between the {mergeWith(Restriction)}
signatures in {{Restriction}} and {{Restrictions}}.{quote}
It is due to the fact that {{PrimaryKeyRestrictions}} is a composite of
{{Restriction}}.
The problem with such a big refactoring is that in the end you lost a part of
your ability to see if the changes that you have made improve really the
readability of the code compare to the original version.
Refactor SelectStatement
Key: CASSANDRA-7981
URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
Project: Cassandra
Issue Type: Bug
Reporter: Benjamin Lerer
Assignee: Benjamin Lerer
Fix For: 3.0
The current state of the code of SelectStatement make fixing some issues or
adding new functionnalities really hard. It also contains some
functionnalities that we would like to reuse in ModificationStatement but
cannot for the moment.
Ideally I would like to:
* Perform as much validation as possible on Relations instead of performing
it on Restrictions as it will help for problem like the one of
#CASSANDRA-6075 (I believe that by clearly separating validation and
Restrictions building we will also make the code a lot clearer)
* Provide a way to easily merge restrictions on the same columns as needed
for #CASSANDRA-7016
* Have a preparation logic (validation + pre-processing) that we can easily
reuse for Delete statement #CASSANDRA-6237
* Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[ https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14214473#comment-14214473 ] Benjamin Lerer commented on CASSANDRA-7981: --- Fogot to mention one important thing: The refactoring break the IN ordering. The results are now returned ordered in the normal order not in the order specified in the IN condition. Refactor SelectStatement Key: CASSANDRA-7981 URL: https://issues.apache.org/jira/browse/CASSANDRA-7981 Project: Cassandra Issue Type: Bug Reporter: Benjamin Lerer Assignee: Benjamin Lerer Fix For: 3.0 The current state of the code of SelectStatement make fixing some issues or adding new functionnalities really hard. It also contains some functionnalities that we would like to reuse in ModificationStatement but cannot for the moment. Ideally I would like to: * Perform as much validation as possible on Relations instead of performing it on Restrictions as it will help for problem like the one of #CASSANDRA-6075 (I believe that by clearly separating validation and Restrictions building we will also make the code a lot clearer) * Provide a way to easily merge restrictions on the same columns as needed for #CASSANDRA-7016 * Have a preparation logic (validation + pre-processing) that we can easily reuse for Delete statement #CASSANDRA-6237 * Make the code much easier to read and safer to modify. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14210048#comment-14210048
]
Benjamin Lerer commented on CASSANDRA-7981:
---
The patch is there:
[branch|https://github.com/blerer/cassandra/compare/CASSANDRA-7981]
The Restriction building and handling has been extracted form
{{SelectStatement}} into {{StatementRestrictions}} so it should be reusable. As
restrictions on the partition key or on the clustering columns where always
used together as if they were one restriction I wrapped them behind a
{{PrimaryKeyRestrictions}} interface and made an implementation for normal
restrictions, multi-column restrictions and token restrictions. Doing so
removed the reliability problems caused by the fact that we were trying to a
make muti-column restrictions or token restrictions fit to the single
restriction model.
Refactor SelectStatement
Key: CASSANDRA-7981
URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
Project: Cassandra
Issue Type: Bug
Reporter: Benjamin Lerer
Assignee: Benjamin Lerer
Fix For: 3.0
The current state of the code of SelectStatement make fixing some issues or
adding new functionnalities really hard. It also contains some
functionnalities that we would like to reuse in ModificationStatement but
cannot for the moment.
Ideally I would like to:
* Perform as much validation as possible on Relations instead of performing
it on Restrictions as it will help for problem like the one of
#CASSANDRA-6075 (I believe that by clearly separating validation and
Restrictions building we will also make the code a lot clearer)
* Provide a way to easily merge restrictions on the same columns as needed
for #CASSANDRA-7016
* Have a preparation logic (validation + pre-processing) that we can easily
reuse for Delete statement #CASSANDRA-6237
* Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[ https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14210052#comment-14210052 ] Benjamin Lerer commented on CASSANDRA-7981: --- [~thobbs] could you review that monster? Unit tests and DTest pass so I do not think that I have broken to many stuff. Refactor SelectStatement Key: CASSANDRA-7981 URL: https://issues.apache.org/jira/browse/CASSANDRA-7981 Project: Cassandra Issue Type: Bug Reporter: Benjamin Lerer Assignee: Benjamin Lerer Fix For: 3.0 The current state of the code of SelectStatement make fixing some issues or adding new functionnalities really hard. It also contains some functionnalities that we would like to reuse in ModificationStatement but cannot for the moment. Ideally I would like to: * Perform as much validation as possible on Relations instead of performing it on Restrictions as it will help for problem like the one of #CASSANDRA-6075 (I believe that by clearly separating validation and Restrictions building we will also make the code a lot clearer) * Provide a way to easily merge restrictions on the same columns as needed for #CASSANDRA-7016 * Have a preparation logic (validation + pre-processing) that we can easily reuse for Delete statement #CASSANDRA-6237 * Make the code much easier to read and safer to modify. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14211385#comment-14211385
]
Tyler Hobbs commented on CASSANDRA-7981:
Preliminary review comments:
* The restriction class hierarchy is confusing, but I don't see an obvious way
to simplify it
* {{AbstracPrimaryKeyRestrictions}} has a typo in the name
* {{MultiColumnRestriction}}
** {{hasIndexedColumns()}} always returns on the first item
** In any case, I don't think the behavior of {{hasIndexedColumns()}} would be
correct for slice relations, since that would require a composite index
(assuming CASSANDRA-4476 gets implemented)
* I think {{hasSupportingIndex()}} would be a more accurate name for
{{hasIndexedColumns()}}
* {{ForwardingPrimaryKeyRestrictions}} could use class-level javadocs
explaining why we need it
* {{StatementRestrictions.getPartitionKeyUnrestrictedComponents()}} should use
{{list.removeAll()}} instead of {{list.remove()}} (and we'll want a test to
cover that)
* {{SelectStatement}} has a ton of {{cql3}} and {{db}} imports that could be
reverted back to a {{*}} import
* There seems to be a conflict between the {{mergeWith(Restriction)}}
signatures in {{Restriction}} and {{Restrictions}}. Perhaps rename
{{Restrictions.mergeWith()}} to {{Restrictions.addRestriction()}} and make sure
the subclasses implement both correctly?
* There are a few javadoc params that are misnamed here and there in the
restriction classes (hopefully Eclipse can point those out for you)
* StatementRestrictions has some unneeded {{RowPosition}} generics that can
be replaced with {{}}
Other than that, I'm having QA do test coverage analysis on your branch so we
can see if there are any gaps in the testing on the new code.
Refactor SelectStatement
Key: CASSANDRA-7981
URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
Project: Cassandra
Issue Type: Bug
Reporter: Benjamin Lerer
Assignee: Benjamin Lerer
Fix For: 3.0
The current state of the code of SelectStatement make fixing some issues or
adding new functionnalities really hard. It also contains some
functionnalities that we would like to reuse in ModificationStatement but
cannot for the moment.
Ideally I would like to:
* Perform as much validation as possible on Relations instead of performing
it on Restrictions as it will help for problem like the one of
#CASSANDRA-6075 (I believe that by clearly separating validation and
Restrictions building we will also make the code a lot clearer)
* Provide a way to easily merge restrictions on the same columns as needed
for #CASSANDRA-7016
* Have a preparation logic (validation + pre-processing) that we can easily
reuse for Delete statement #CASSANDRA-6237
* Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-7981) Refactor SelectStatement
[ https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14144570#comment-14144570 ] Benjamin Lerer commented on CASSANDRA-7981: --- We should also fix as part of this ticket the problem of the token function. The current approach is not able to dectect as invalid queries like: * SELECT * FROM %s WHERE token(a, b) token(?, ?) and token(b) token(?, ?) * SELECT * FROM %s WHERE token(a) token(?, ?) and token(b) token(?, ?) Refactor SelectStatement Key: CASSANDRA-7981 URL: https://issues.apache.org/jira/browse/CASSANDRA-7981 Project: Cassandra Issue Type: Bug Reporter: Benjamin Lerer Assignee: Benjamin Lerer Fix For: 3.0 The current state of the code of SelectStatement make fixing some issues or adding new functionnalities really hard. It also contains some functionnalities that we would like to reuse in ModificationStatement but cannot for the moment. Ideally I would like to: * Perform as much validation as possible on Relations instead of performing it on Restrictions as it will help for problem like the one of #CASSANDRA-6075 (I believe that by clearly separating validation and Restrictions building we will also make the code a lot clearer) * Provide a way to easily merge restrictions on the same columns as needed for #CASSANDRA-7016 * Have a preparation logic (validation + pre-processing) that we can easily reuse for Delete statement #CASSANDRA-6237 * Make the code much easier to read and safer to modify. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
