ibessonov commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241761480
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -44,6 +44,8 @@ public interface CatalogService {
CatalogTableDescriptor table(int tableId, long timestamp);
+ CatalogTableDescriptor table(int tableId, int catalogVersion);
Review Comment:
Are we sure that we need methods with version parameter? This will
eventually multiply the number of methods by two. I would propose having a
single "versionToTimestamp" method instead. We should discuss it and probably
implement it later.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -275,11 +281,16 @@ public CompletableFuture<Void>
createTable(CreateTableParams params) {
CatalogZoneDescriptor zone = getZone(catalog,
Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
- CatalogTableDescriptor table =
CatalogUtils.fromParams(catalog.objectIdGenState(), zone.id(), params);
+ int id = catalog.objectIdGenState();
+
+ CatalogTableDescriptor table = CatalogUtils.fromParams(id++,
zone.id(), params);
+
+ CatalogHashIndexDescriptor pkIndex =
createHashIndexDescriptor(table, id++, createPkIndexParams(params));
Review Comment:
Table descriptor is created using "CatalogUtils.fromParams", and index is
created using "createHash...", it's a little inconsistent. Can you please check
other methods and what they do? It would be nice if all methods were similar to
each other
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -191,6 +191,12 @@ public CatalogTableDescriptor table(int tableId, long
timestamp) {
return catalogAt(timestamp).table(tableId);
}
+ @Override
+ public CatalogTableDescriptor table(int tableId, int catalogVersion) {
+ return catalog(catalogVersion).table(tableId);
+ }
+
+ /** {@inheritDoc} */
Review Comment:
You added `/** {@inheritDoc} */`? I can't believe my eyes!
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column
'{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams
params) {
+ return CreateHashIndexParams.builder()
+ .schemaName(params.schemaName())
+ .tableName(params.tableName())
+ .indexName(params.tableName() + "_PK")
+ .columns(params.primaryKeyColumns())
+ .unique()
+ .build();
+ }
+
+ private static CatalogHashIndexDescriptor createHashIndexDescriptor(
Review Comment:
Are you sure that extracting this method helps the code readability? I don't
think so:
- it's only two lines
- it's only used twice
- it breaks code flow by introducing different name and usage pattern (see
one of the comments above)
What if we don't introduce it?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/DropZoneEventParameters.java:
##########
@@ -30,8 +30,8 @@ public class DropZoneEventParameters extends
CatalogEventParameters {
* @param causalityToken Causality token.
* @param zoneId An id of dropped distribution zone.
*/
- public DropZoneEventParameters(long causalityToken, int zoneId) {
- super(causalityToken);
+ public DropZoneEventParameters(long causalityToken, int catalogVersion,
int zoneId) {
Review Comment:
Please add catalog version to javadoc
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column
'{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams
params) {
+ return CreateHashIndexParams.builder()
Review Comment:
Maybe I'm missing something, but where's the index state?
PK index must be created as an RW index, but such enum/classification
doesn't seem to exist. Are we planning to implement it later?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -44,6 +44,8 @@ public interface CatalogService {
CatalogTableDescriptor table(int tableId, long timestamp);
+ CatalogTableDescriptor table(int tableId, int catalogVersion);
Review Comment:
Or the opposite, migrate all methods to versions. I don't have a clear answer
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1638,6 +1681,78 @@ void testGetCatalogEntityInCatalogEvent() {
assertThat(result, willCompleteSuccessfully());
}
+ @Test
+ void testGetTableByIdAndCatalogVersion() {
+ assertThat(service.createTable(simpleTable(TABLE_NAME)),
willBe(nullValue()));
+
+ assertNull(service.table(2, 0));
+ assertNotNull(service.table(2, 1));
+ }
+
+ @Test
+ void testGetTableIdOnDropIndexEvent() {
+ assertThat(service.createTable(simpleTable(TABLE_NAME)),
willBe(nullValue()));
+
+ assertThat(
+ service.createIndex(
+ CreateHashIndexParams.builder()
+ .schemaName(SCHEMA_NAME)
+ .tableName(TABLE_NAME)
+ .indexName(INDEX_NAME)
+ .columns(List.of("VAL"))
+ .build()
+ ),
+ willBe(nullValue())
+ );
+
+ EventListener<CatalogEventParameters> eventListener =
mock(EventListener.class);
+
+ ArgumentCaptor<DropIndexEventParameters> captor =
ArgumentCaptor.forClass(DropIndexEventParameters.class);
+
+
doReturn(completedFuture(false)).when(eventListener).notify(captor.capture(),
any());
+
+ service.listen(CatalogEvent.INDEX_DROP, eventListener);
+
+ // Let's remove the index.
+ assertThat(
+
service.dropIndex(DropIndexParams.builder().schemaName(SCHEMA_NAME).indexName(INDEX_NAME).build()),
+ willBe(nullValue())
+ );
+
+ DropIndexEventParameters eventParameters = captor.getValue();
+
+ assertEquals(4L, eventParameters.indexId());
+ assertEquals(2L, eventParameters.tableId());
+
+ // Let's delete the table.
+ assertThat(
+
service.dropTable(DropTableParams.builder().schemaName(SCHEMA_NAME).tableName(TABLE_NAME).build()),
+ willBe(nullValue())
+ );
+
+ // Let's make sure that the pk index has been deleted.
Review Comment:
```suggestion
// Let's make sure that the PK index has been deleted.
```
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1250,24 +1286,31 @@ public void testCreateIndexEvents() {
assertThat(service.createIndex(createIndexParams),
willThrow(TableNotFoundException.class));
verifyNoInteractions(eventListener);
- // Create table.
+ // Create table with pk index.
Review Comment:
```suggestion
// Create table with PK index.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -295,7 +306,7 @@ public CompletableFuture<Void> dropTable(DropTableParams
params) {
Arrays.stream(schema.indexes())
.filter(index -> index.tableId() == table.id())
- .forEach(index -> updateEntries.add(new
DropIndexEntry(index.id())));
+ .forEach(index -> updateEntries.add(new
DropIndexEntry(index.id(), index.tableId())));
Review Comment:
Isn't this information excessive?
Does alive index have the "tableId", so that you could read it before
deletion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
