SPARKC-403: Add CLUSTERING ORDER in cql statement #981
Conversation
|
I think this is a good addition but I have two major requests.
https://docs.datastax.com/en/drivers/java/2.0/com/datastax/driver/core/TableMetadata.html ClusteringOrder is represented by a list of Orderings so very close to what you currently have. |
…teringOrder and Added list of Clustering order to TableDef
|
@RussellSpitzer based on your suggestion i changed the code. Do i need to add & change anything else? |
|
Make a jira https://datastax-oss.atlassian.net/projects/SPARKC/ For tracking (this is how we catch everything up in release notes and track versioning) |
| @@ -138,7 +149,9 @@ case class TableDef( | |||
| clusteringColumns: Seq[ColumnDef], | |||
| regularColumns: Seq[ColumnDef], | |||
| indexes: Seq[IndexDef] = Seq.empty, | |||
| isView: Boolean = false) extends StructDef { | |||
| isView: Boolean = false, | |||
| clusteringOrder: Option[Seq[ClusteringOrder]] = None, | |||
There was a problem hiding this comment.
Shouldn't this be a property of the ColunDef now? Ie do you really need to specify it separately?
|
@RussellSpitzer , Jira is already there, this is the Link. According to discussion, we need to have list of ClusteringOrders inside TableDef, correct me if i miss understood. And for property part, it depends on use case, i don't have better project knowledge, so let me know what should be there? |
|
It looks to me like the information about ClusteringOrders is in two places within the TableDef.
I think only 2 is neccessary? Having both 1 and 2 gives us the possibility of having them not matching which seems like a bit of a hole in the implementation. I think it would be find to have a function on tableDef
If we want it exposed at that level. |
|
I changed according to you, Please verify. And do we need to add some functionality for this method also DataFrameFunctions.createCassandraTable? |
| @@ -138,7 +150,8 @@ case class TableDef( | |||
| clusteringColumns: Seq[ColumnDef], | |||
| regularColumns: Seq[ColumnDef], | |||
| indexes: Seq[IndexDef] = Seq.empty, | |||
| isView: Boolean = false) extends StructDef { | |||
| isView: Boolean = false, | |||
| options: String = "") extends StructDef { | |||
There was a problem hiding this comment.
I still don't like being able to just pass a string here, If you really think we need this I think it should at least be Seq[String] and we should just require that they not contain "AND" or "WITH". Then we can convert the append code below into a string join instead of having the more complicated logic.
This removes the need for the "appendOptions" function and replaces it with
require(options.forAll( option => !(option.toLowerCase.contains("and") && !(option.toLowerCase.contains("WITH")), "Table options must not contain "WITH OR AND"
(options +: clusteringOptions).mkString("WITH", "AND")```
bcantoni
changed the title
| clusteringColumns.map { col => s"${quote(col.columnName)} ${col.clusteringOrder}"}.mkString(", ") | ||
|
|
||
| def clusterOrder: Seq[ClusteringOrder] = clusteringColumns.map(_.clusteringOrder) | ||
| val ordered = clusteringColumns.map( col => s"${quote(col.columnName)} ${col.clusteringOrder}") |
There was a problem hiding this comment.
This is a bit of an overloaded variable name. Perhaps clusterOrderingClause ?
| @@ -151,11 +151,12 @@ case class TableDef( | |||
| regularColumns: Seq[ColumnDef], | |||
| indexes: Seq[IndexDef] = Seq.empty, | |||
| isView: Boolean = false, | |||
| options: String = "") extends StructDef { | |||
| options: Seq[String] = Seq.empty) extends StructDef { | |||
There was a problem hiding this comment.
Perhaps this should be tableOptions (matching the cql docs), and also now that I think about this perhaps it fits a Map better than a sequence? This would make it much clearer that we are looking for a set of key-value pairs.
|
|
||
| require(partitionKey.forall(_.isPartitionKeyColumn), "All partition key columns must have role PartitionKeyColumn") | ||
| require(clusteringColumns.forall(_.isClusteringColumn), "All clustering columns must have role ClusteringColumn") | ||
| require(regularColumns.forall(!_.isPrimaryKeyColumn), "Regular columns cannot have role PrimaryKeyColumn") | ||
| require(options.forall( option => !(option.toLowerCase.contains("and") && !(option.toLowerCase.contains("with")))), "Table options must not contain WITH OR AND") |
There was a problem hiding this comment.
What if I want to set my comment on a table to Sand Castles with Judge's Rankings. Basically I think we should let the driver validate the tableOptions. If we want to test them here we should probably only test keys (once we change the tableOptions to a map). I think we are probably best off without the requires here.
There was a problem hiding this comment.
I changed accordingly. What will be the better option to validate tableOptions keys?
| else if (!stmt.contains("WITH") && opts.startsWith("AND")) s"WITH ${opts.substring(3)}" | ||
| else if (opts == "") s"$stmt" | ||
| else s"$stmt${Properties.lineSeparator}$opts" | ||
| val orderWithOptions:Seq[String] = if (clusteringColumns.size > 0) options.+:(ordered) else options |
There was a problem hiding this comment.
lets just go straight to the treble quote here, I think it may be a bit clearer like
val tableOptionsClause = s"WITH $clusteringOrderingClause ${(tableOptions...).mkString(AND)}"
"""$stmt $tableOptionsClause"""
I added code for Clustering Order support for table creation. Please review the code.