CASSANALYTICS-31 SAI index support in analytics#220
Conversation
|
|
||
| // This builder always produces an index-less table (indexes are applied later by the 5.0 bridge), so a | ||
| // rebuild never carries indexes even if the caller passed index statements. buildSchema runs repeatedly per | ||
| // table in a JVM; if an earlier call already registered indexes, copy them onto this rebuild so it matches | ||
| // the registered table. | ||
| if (maybeExistingTableMetadata != null | ||
| && !maybeExistingTableMetadata.indexes.isEmpty() | ||
| && tableMetadata.indexes.isEmpty()) | ||
| { | ||
| tableMetadata = tableMetadata.unbuild() | ||
| .indexes(maybeExistingTableMetadata.indexes) | ||
| .build(); | ||
| } | ||
|
|
There was a problem hiding this comment.
RecordWriter is using 5 arg buildSchema() method which is not attaching index statements, you could eliminate copying indexes here if you use the 8 arg buildSchema() method in RecordWriter and can eventually get rid of 5 arg buildSchema() method
cqlTable = writerContext.bridge()
.buildSchema(writerContext.schema().getTableSchema().createStatement,
writerContext.job().qualifiedTableName().keyspace(),
IGNORED_REPLICATION_FACTOR,
writerContext.cluster().getPartitioner(),
writerContext.schema().getUserDefinedTypeStatements(),
null,
writerContext.schema().getTableSchema().getIndexStatements(),
false);
There was a problem hiding this comment.
I did restructure and moved SAI specific code to extended class. Now this comment not applicable I believe
There was a problem hiding this comment.
@skoppu22 This block of code is moved to FiveZeroSchemaBuilder.beforeTableRegistered but the issue still exists. You can get rid of the logic to not drop indexes in beforeTableRegistered and even the method beforeTableRegistered if you use 8 argument constructor for schemaBuilder in Recordwriter passing writerContext.schema().getTableSchema().getIndexStatements() in the constructor.
There was a problem hiding this comment.
@jyothsnakonisa I have applied your recommended changes to pass index statements as part of cqlTable. We still need beforeTableRegistered. Because
Reason 1, Some builders only receive a CREATE TABLE string, with no CqlTable and no CREATE INDEX statements, so they can only build index‑less. These are public CassandraBridge APIs:
encodePartitionKeys(...) / toTokens(...) / encodePartitionKey(...) → new FiveZeroSchemaBuilder(createTableStmt, ks, rf, partitioner)
readPartitionKeys(...) → same createStmt‑only builder
We can't make these pass statements without changing the public bridge API, and their callers frequently don't have the CREATE INDEX statements to pass. If one of these is the first build of a table in a JVM, the table registers without its SAI index.
Reason 2, The same table is built more than once per JVM in normal flows:
Write path, multiple partitions/executor: RecordWriter is constructed per Spark partition (mapPartitions); the 2nd+ partition on an executor rebuilds the already‑registered table.
Write path, single partition: after writing, SortedSSTableWriter.validateSSTables(...) → buildLocalDataLayer → new LocalDataLayer → buildSchema rebuilds the same table in the same JVM.
Read path: getCompactionScanner, getPartitionSizeIterator, rebuildBloomFilter all build from a CqlTable, repeatedly within a JVM.
There was a problem hiding this comment.
Looks like there are no callers for encodePartitionKeys and readPartitionKeys. If we can remove them, then we can switch to 8 arg constructor as you mentioned, and we can simplify beforeTableRegistered just to avoid duplicate registration
There was a problem hiding this comment.
Yes encodePartitionKeys & readPartitionKeys are not used anywhere in production code hence removing them should be safe. I will leave it to you whether to remove those methods or not.
| java.util.Collections.emptySet(), | ||
| java.util.Collections.emptySet()); |
There was a problem hiding this comment.
Please remove fully qualified class names.
|
|
||
| // This builder always produces an index-less table (indexes are applied later by the 5.0 bridge), so a | ||
| // rebuild never carries indexes even if the caller passed index statements. buildSchema runs repeatedly per | ||
| // table in a JVM; if an earlier call already registered indexes, copy them onto this rebuild so it matches | ||
| // the registered table. | ||
| if (maybeExistingTableMetadata != null | ||
| && !maybeExistingTableMetadata.indexes.isEmpty() | ||
| && tableMetadata.indexes.isEmpty()) | ||
| { | ||
| tableMetadata = tableMetadata.unbuild() | ||
| .indexes(maybeExistingTableMetadata.indexes) | ||
| .build(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Yes encodePartitionKeys & readPartitionKeys are not used anywhere in production code hence removing them should be safe. I will leave it to you whether to remove those methods or not.
2 participants
Generate SAI index files as well while producing sstable files, so no need of async rebuilding of SAI indexes after bulk write.
Circle CI : https://app.circleci.com/pipelines/github/skoppu22/cassandra-analytics/148/workflows/5eaeec43-e43b-444c-badb-ef696a3828fc