CASSANDRA-14825 Expose schema in virtual table#284
CASSANDRA-14825 Expose schema in virtual table#284clohfink wants to merge 6 commits intoapache:trunkfrom
Conversation
clohfink
force-pushed
the
describe_vtables
branch
5 times, most recently
from
3ec70d0 to
8d6d03c
Compare
clohfink
force-pushed
the
describe_vtables
branch
from
f8845bf to
410aa0d
Compare
| { | ||
| AbstractDescribeTable(String keyspace, String name) | ||
| { | ||
| super(TableMetadata.builder(keyspace, "describe_" + name) |
There was a problem hiding this comment.
Given all these tables will have the same schema, I think it might be more easily used if the “type” was just a column, rather than having to go to a whole new table. So you could just “select * from virtual_schema” and get everything.
There was a problem hiding this comment.
complexity there is there are things like having a UDT with same name as a table in the same keyspace. We can have something like ((type), keyspace, name) so that way we can provide uniqueness to everything and it would only be funny for keyspaces.
tolbertam
left a comment
tolbertam
left a comment
There was a problem hiding this comment.
Tried this out and its working great! Had a few suggestions and noticed one issue, but 👍 on the approach.
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.cassandra.db; |
There was a problem hiding this comment.
Maybe nitpicking, but should this be in schema package? CQLTypeParser.parseRaw could remain default visibility this way too.
| sb.append("\n(this should not be used to reproduce this schema)\n\n"); | ||
| } | ||
|
|
||
| sb.append("CREATE TABLE "); |
There was a problem hiding this comment.
Noticed previous implementation included IF NOT EXISTS, but only for tables, not other elements. Agree it should be consistent. Was thinking would be kinda nice to have a way of expressing schema with these directives, but not sure of a good way of doing this without duplicating data/complicating things. Probably better that it's done this way, especially since consistent with describe output.
| if (metaData.isSuper() || metaData.isVirtual()) | ||
| return false; | ||
|
|
||
| return !(metaData.isCompactTable() |
There was a problem hiding this comment.
Since compact storage is not around anymore should be sufficient to use return metaData.isCQLTable() && !metaData.isVirtual(); for this I think.
| .kind(TableMetadata.Kind.VIRTUAL) | ||
| .partitioner(new LocalPartitioner(utfComposite)) | ||
| .addPartitionKeyColumn(KEYSPACE, UTF8Type.instance) | ||
| .addPartitionKeyColumn(name + "_name", UTF8Type.instance) |
There was a problem hiding this comment.
It would be nice if <x>_name was a clustering column so you could do something like select * from system_views.describe_table where keyspace_name='x'
|
|
||
| SimpleDataSet result = new SimpleDataSet(metadata()); | ||
| result.row(keyspace) | ||
| .column(CQL, SchemaCQLHelper.getKeyspaceAsCQL(keyspace)); |
There was a problem hiding this comment.
This surfaces a ServerError with an NPE if the keyspace doesn't exist, instead of returning 0 rows:
ERROR [Native-Transport-Requests-1] 2019-09-04 23:05:26,228 QueryMessage.java:124 - Unexpected error during query
java.lang.NullPointerException: null
at org.apache.cassandra.db.SchemaCQLHelper.toCQL(SchemaCQLHelper.java:152)
at org.apache.cassandra.db.SchemaCQLHelper.getKeyspaceAsCQL(SchemaCQLHelper.java:65)
at org.apache.cassandra.db.virtual.DescribeTables$DescribeKeyspaceTable.data(DescribeTables.java:88)
| @Override | ||
| public DataSet data(DecoratedKey partitionKey) | ||
| { | ||
| String keyspace = UTF8Type.instance.compose(partitionKey.getKey()); |
There was a problem hiding this comment.
One thing that popped into my mind is that this requires the internal representation of the identifier instead of the cql representation. i.e. if your keyspace name is "MiXeDCASE" to access this you need to query it in an unquoted manner, i.e. select * from system_views.describe_keyspace where keyspace_name='MiXeDCASE'.
This is consistent with system_schema, but those coming from CQL, or working with quoted CQL identifiers might be surprised to see that querying with the quoted name, i.e. '"MiXeDCASE"' doesn't work. That being said I think I like it the way it currently is, primarily for the consistency with system_schema tables.
| case CRC_CHECK_CHANCE: | ||
| sb.append(tableParams.crcCheckChance); | ||
| return; | ||
| case DEFAULT_TIME_TO_LIVE: |
There was a problem hiding this comment.
Slight exception needed for materialized views in that this should not be included:
cql_from_vt.txt:157:InvalidRequest: Error from server: code=2200 [Invalid query] message="Cannot set default_time_to_live for a materialized view. Data in a materialized view always expire at the same time than the corresponding data in the parent table."
smiklosovic
changed the title
4 participants
No description provided.