ibessonov commented on code in PR #3589:
URL: https://github.com/apache/ignite-3/pull/3589#discussion_r1560533746
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/StorageHashIndexDescriptor.java:
##########
@@ -33,9 +33,7 @@
* @see HashIndexStorage
*/
public class StorageHashIndexDescriptor implements StorageIndexDescriptor {
- /**
- * Descriptor of a Hash Index column.
- */
+ /** Descriptor of a Hash Index column. */
Review Comment:
Please revert all these useless javadoc updates
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/StorageHashIndexDescriptor.java:
##########
@@ -112,17 +114,22 @@ public List<StorageHashIndexColumnDescriptor> columns() {
return columns;
}
+ @Override
+ public boolean isPk() {
Review Comment:
Why does it have to be an internal flag instead of the external comparison,
like you do in the constructor? Seems excessive to me
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/StorageSortedIndexDescriptor.java:
##########
@@ -150,20 +145,28 @@ private static List<StorageSortedIndexColumnDescriptor>
extractIndexColumnsConfi
CatalogTableDescriptor table,
CatalogSortedIndexDescriptor index
) {
- assert table.id() == index.tableId() : "tableId=" + table.id() + ",
indexTableId=" + index.tableId();
+ assert table.id() == index.tableId() : "indexId=" + index.id() + ",
tableId=" + table.id() + ", indexTableId=" + index.tableId();
return index.columns().stream()
.map(columnDescriptor -> {
String columnName = columnDescriptor.name();
CatalogTableColumnDescriptor column =
table.column(columnName);
- assert column != null : columnName;
+ assert column != null : "indexId=" + index.id() + ",
columnName=" + columnName;
CatalogColumnCollation collation =
columnDescriptor.collation();
return new StorageSortedIndexColumnDescriptor(columnName,
getNativeType(column), column.nullable(), collation.asc());
})
.collect(toList());
}
+
+ private static BinaryTupleSchema
createSchema(List<StorageSortedIndexColumnDescriptor> columns) {
Review Comment:
You moved this method to another place. Was there any specific reason for
that? It bloats the PR for no good reason
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractIndexStorageTest.java:
##########
@@ -79,9 +85,11 @@ public abstract class AbstractIndexStorageTest<S extends
IndexStorage, D extends
protected static final int TEST_PARTITION = 12;
- protected static final String INDEX_NAME = "TEST_IDX";
+ protected static final String TABLE_NAME = "FOO";
+
+ protected static final String PK_INDEX_NAME = pkIndexName(TABLE_NAME);
- protected static final String TABLE_NAME = "foo";
+ protected static final String INDEX_NAME = "TEST_IDX";
Review Comment:
Did you change the order of constants manually, or it's been done
automatically by IDEA somehow?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/StorageIndexDescriptor.java:
##########
@@ -26,40 +26,29 @@
import org.apache.ignite.internal.type.NativeType;
import org.apache.ignite.internal.type.NativeTypes;
-/**
- * Index descriptor.
- */
+/** Index descriptor. */
Review Comment:
Please revert all these useless javadoc updates
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/StorageSortedIndexDescriptor.java:
##########
@@ -99,34 +95,30 @@ public String toString() {
private final BinaryTupleSchema binaryTupleSchema;
+ private final boolean pk;
+
/**
* Constructor.
*
* @param table Catalog table descriptor.
* @param index Catalog index descriptor.
*/
public StorageSortedIndexDescriptor(CatalogTableDescriptor table,
CatalogSortedIndexDescriptor index) {
- this(index.id(), extractIndexColumnsConfiguration(table, index));
+ this(index.id(), extractIndexColumnsConfiguration(table, index),
table.primaryKeyIndexId() == index.id());
}
/**
* Creates an Index Descriptor from a given set of column descriptors.
*
* @param indexId Index ID.
* @param columnDescriptors Column descriptors.
+ * @param pk Primary index flag.
*/
- public StorageSortedIndexDescriptor(int indexId,
List<StorageSortedIndexColumnDescriptor> columnDescriptors) {
+ public StorageSortedIndexDescriptor(int indexId,
List<StorageSortedIndexColumnDescriptor> columnDescriptors, boolean pk) {
Review Comment:
I have the same list of questions:
- if this constructor has to be public for some reason, then please make it
`@TestOnly`
- why does it have to be an internal state rather than an external check?
This is a calculable property, after all
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/StorageHashIndexDescriptor.java:
##########
@@ -81,25 +79,29 @@ public String toString() {
private final List<StorageHashIndexColumnDescriptor> columns;
+ private final boolean pk;
+
/**
* Constructor.
*
* @param table Catalog table descriptor.
* @param index Catalog index descriptor.
*/
public StorageHashIndexDescriptor(CatalogTableDescriptor table,
CatalogHashIndexDescriptor index) {
- this(index.id(), extractIndexColumnsConfiguration(table, index));
+ this(index.id(), extractIndexColumnsConfiguration(table, index),
table.primaryKeyIndexId() == index.id());
}
/**
* Creates an Index Descriptor from a given set of columns.
*
* @param indexId Index id.
* @param columns Columns descriptors.
+ * @param pk Primary index flag.
*/
- public StorageHashIndexDescriptor(int indexId,
List<StorageHashIndexColumnDescriptor> columns) {
+ public StorageHashIndexDescriptor(int indexId,
List<StorageHashIndexColumnDescriptor> columns, boolean pk) {
Review Comment:
Please mark this constructor as `@TestOnly`
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/StorageSortedIndexDescriptor.java:
##########
@@ -36,9 +36,7 @@
* @see SortedIndexStorage
*/
public class StorageSortedIndexDescriptor implements StorageIndexDescriptor {
- /**
- * Descriptor of a Sorted Index column (column name and column sort order).
- */
+ /** Descriptor of a Sorted Index column (column name and column sort
order). */
Review Comment:
Please revert all these useless javadoc updates
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -123,25 +126,24 @@ public abstract class AbstractMvTableStorageTest extends
BaseMvStoragesTest {
protected StorageHashIndexDescriptor hashIdx;
+ protected StorageIndexDescriptor pkIdx;
+
private final CatalogService catalogService = mock(CatalogService.class);
- protected final StorageIndexDescriptorSupplier indexDescriptorSupplier =
new StorageIndexDescriptorSupplier() {
- @Override
- public @Nullable StorageIndexDescriptor get(int indexId) {
- int catalogVersion = catalogService.latestCatalogVersion();
+ protected final StorageIndexDescriptorSupplier indexDescriptorSupplier =
indexId -> {
Review Comment:
Another useless code change. What was your motivation behind it?
--
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]
