You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "Alex Petrov (Jira)" <ji...@apache.org> on 2020/01/17 12:52:00 UTC
[jira] [Comment Edited] (CASSANDRA-13917) COMPACT STORAGE queries
on dense static tables accept hidden column1 and value columns
[ https://issues.apache.org/jira/browse/CASSANDRA-13917?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17017897#comment-17017897 ]
Alex Petrov edited comment on CASSANDRA-13917 at 1/17/20 12:51 PM:
-------------------------------------------------------------------
[~slebresne] you are right. In fact, I've found several more places where this could cause a problem. What we should've done initially is just add a separate method for CQL layer, and let the rest of the calls (e.g., all internal stuff) to use the old one. Committed version of the patch also caused problems in mixed version environments which was caught by one of the upgrade in-jvm dtests. Also, things like thrift and compact storage compatibility layer was impacted in a way, too.
I'm still thinking if {{RowUpdateBuilder}} should be using CQL columns or all columns. I'd argue that CQL only (e.g., no hidden), since this is also how we query them for the most part. We don't really those hidden column even on internal paths.
I've made a patch and have added a couple more tests that validates the situations that weren't covered previously. Thanks to [CASSANDRA-15506], we can also make sure that upgrade tests are going to be running.
|[patch|https://github.com/apache/cassandra/compare/trunk...ifesdjeen:CASSANDRA-13917-followup]|[CI|https://circleci.com/gh/ifesdjeen/cassandra/tree/CASSANDRA-13917-followup]|
|[patch|https://github.com/apache/cassandra/compare/trunk...ifesdjeen:CASSANDRA-13917-followup-3.11]|[CI|https://circleci.com/gh/ifesdjeen/cassandra/tree/CASSANDRA-13917-followup-3.11]|
was (Author: ifesdjeen):
[~slebresne] you are right. In fact, I've found several more places where this could cause a problem. What we should've done initially is just add a separate method for CQL layer, and let the rest of the calls (e.g., all internal stuff) to use the old one. Committed version of the patch also caused problems in mixed version environments which was caught by one of the upgrade in-jvm dtests. Also, things like thrift and compact storage compatibility layer was impacted in a way, too.
I'm still thinking if {{RowUpdateBuilder}} should be using CQL columns or all columns. I'd argue that CQL only (e.g., no hidden), since this is also how we query them for the most part. We don't really those hidden column even on internal paths.
I've made a patch and have added a couple more tests that validates the situations that weren't covered previously. Thanks to [CASSANDRA-15506], we can also make sure that upgrade tests are going to be running.
|[patch|https://github.com/apache/cassandra/compare/trunk...ifesdjeen:CASSANDRA-13917-followup]|[CI|https://circleci.com/gh/ifesdjeen/cassandra/tree/CASSANDRA-13917-followup]|
> COMPACT STORAGE queries on dense static tables accept hidden column1 and value columns
> --------------------------------------------------------------------------------------
>
> Key: CASSANDRA-13917
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13917
> Project: Cassandra
> Issue Type: Bug
> Components: Legacy/Core
> Reporter: Alex Petrov
> Assignee: Aleksandr Sorokoumov
> Priority: Low
> Labels: lhf
> Fix For: 3.0.x, 3.11.x
>
> Attachments: 13917-3.0-testall-13.12.2019, 13917-3.0-testall-16.01.2020, 13917-3.0-testall-2.png, 13917-3.0-testall-20.11.2019.png, 13917-3.0-upgrade-16.01.2020, 13917-3.0.png, 13917-3.11-testall-13.12.2019, 13917-3.11-testall-16.01.2020.png, 13917-3.11-testall-2.png, 13917-3.11-testall-20.11.2019.png, 13917-3.11-upgrade-16.01.2020.png, 13917-3.11.png
>
>
> Test for the issue:
> {code}
> @Test
> public void testCompactStorage() throws Throwable
> {
> createTable("CREATE TABLE %s (a int PRIMARY KEY, b int, c int) WITH COMPACT STORAGE");
> assertInvalid("INSERT INTO %s (a, b, c, column1) VALUES (?, ?, ?, ?)", 1, 1, 1, ByteBufferUtil.bytes('a'));
> // This one fails with Some clustering keys are missing: column1, which is still wrong
> assertInvalid("INSERT INTO %s (a, b, c, value) VALUES (?, ?, ?, ?)", 1, 1, 1, ByteBufferUtil.bytes('a'));
> assertInvalid("INSERT INTO %s (a, b, c, column1, value) VALUES (?, ?, ?, ?, ?)", 1, 1, 1, ByteBufferUtil.bytes('a'), ByteBufferUtil.bytes('b'));
> assertEmpty(execute("SELECT * FROM %s"));
> }
> {code}
> Gladly, these writes are no-op, even though they succeed.
> {{value}} and {{column1}} should be completely hidden. Fixing this one should be as easy as just adding validations.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org