You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2023/05/19 15:25:54 UTC
[GitHub] [ignite-3] rpuch opened a new pull request, #2093: Ignite 19531
rpuch opened a new pull request, #2093:
URL: https://github.com/apache/ignite-3/pull/2093
(no comment)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2093: IGNITE-19531 Switch table IDs from UUIDs to integers
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2093:
URL: https://github.com/apache/ignite-3/pull/2093#discussion_r1205658113
##########
modules/network-api/src/main/java/org/apache/ignite/network/serialization/MessageReader.java:
##########
@@ -95,6 +96,15 @@ public interface MessageReader {
*/
public int readInt(String name, int dflt);
+ /**
+ * Reads an {@code Integer} value.
+ *
+ * @param name Field name.
+ * @return {@code Integer} value.
+ */
+ @Nullable
+ public Integer readInteger(String name);
Review Comment:
```suggestion
public @Nullable Integer readInteger(String name);
```
##########
modules/client/src/test/java/org/apache/ignite/client/fakes/FakeInternalTable.java:
##########
@@ -91,7 +91,7 @@ public int partitions() {
/** {@inheritDoc} */
Review Comment:
```suggestion
```
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/DirectMessageReader.java:
##########
@@ -124,6 +124,18 @@ public int readInt(String name, int dflt) {
return readInt(name);
}
+ /** {@inheritDoc} */
Review Comment:
```suggestion
```
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/index/IndexValidatorImpl.java:
##########
@@ -127,6 +125,17 @@ private void validate(
}
}
+ @Nullable
+ private static TableView findTableById(int tableId, NamedListView<? extends TableView> tablesView) {
Review Comment:
```suggestion
private static @Nullable TableView findTableById(int tableId, NamedListView<? extends TableView> tablesView) {
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/HashIndexSpoolPlannerTest.java:
##########
@@ -91,6 +91,10 @@ public void testSingleKey() throws Exception {
assertNull(searchRow.get(2));
}
+ private static int randomTableId() {
+ return new Random().nextInt();
Review Comment:
Why not 1 ?)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/AbstractAggregatePlannerTest.java:
##########
@@ -541,7 +541,7 @@ private static IgniteSchema schema(IgniteDistribution distribution,
}
private static IgniteDistribution hash() {
- return IgniteDistributions.affinity(0, UUID.randomUUID(), DEFAULT_ZONE_ID);
+ return IgniteDistributions.affinity(0, new Random().nextInt(), DEFAULT_ZONE_ID);
Review Comment:
Why not 1 ?)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/SetOpPlannerTest.java:
##########
@@ -74,11 +74,15 @@ public void setup() {
createTable(publicSchema, "BROADCAST_TBL2", type, IgniteDistributions.broadcast());
createTable(publicSchema, "SINGLE_TBL1", type, IgniteDistributions.single());
createTable(publicSchema, "SINGLE_TBL2", type, IgniteDistributions.single());
- createTable(publicSchema, "AFFINITY_TBL1", type, IgniteDistributions.affinity(0, UUID.randomUUID(), DEFAULT_ZONE_ID));
+ createTable(publicSchema, "AFFINITY_TBL1", type, IgniteDistributions.affinity(0, randomTableId(), DEFAULT_ZONE_ID));
createTable(publicSchema, "HASH_TBL1", type, IgniteDistributions.hash(List.of(0)));
- createTable(publicSchema, "AFFINITY_TBL2", type, IgniteDistributions.affinity(0, UUID.randomUUID(), DEFAULT_ZONE_ID));
- createTable(publicSchema, "AFFINITY_TBL3", type, IgniteDistributions.affinity(1, UUID.randomUUID(), DEFAULT_ZONE_ID));
- createTable(publicSchema, "AFFINITY_TBL4", type, IgniteDistributions.affinity(0, UUID.randomUUID(), DEFAULT_ZONE_ID + 1));
+ createTable(publicSchema, "AFFINITY_TBL2", type, IgniteDistributions.affinity(0, randomTableId(), DEFAULT_ZONE_ID));
+ createTable(publicSchema, "AFFINITY_TBL3", type, IgniteDistributions.affinity(1, randomTableId(), DEFAULT_ZONE_ID));
+ createTable(publicSchema, "AFFINITY_TBL4", type, IgniteDistributions.affinity(0, randomTableId(), DEFAULT_ZONE_ID + 1));
+ }
+
+ private static int randomTableId() {
+ return new Random().nextInt();
Review Comment:
why not 1 ?)
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/HashIndexDescriptor.java:
##########
@@ -151,6 +153,17 @@ private static List<HashIndexColumnDescriptor> extractIndexColumnsConfiguration(
.collect(toUnmodifiableList());
}
+ @Nullable
+ private static TableView findTableById(int tableId, NamedListView<? extends TableView> tablesView) {
Review Comment:
```suggestion
private static @Nullable TableView findTableById(int tableId, NamedListView<? extends TableView> tablesView) {
```
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStream.java:
##########
@@ -290,6 +298,14 @@ <K, V> void writeMap(Map<K, V> map, MessageCollectionItemType keyType, MessageCo
*/
int readInt();
+ /**
+ * Reads {@code Integer}.
+ *
+ * @return Value.
+ */
+ @Nullable
+ Integer readInteger();
Review Comment:
```suggestion
@Nullable Integer readInteger();
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/HashIndexPlannerTest.java:
##########
@@ -64,13 +65,17 @@ public void hashIndexIsAppliedForEquiCondition() throws Exception {
assertThat(invalidPlanMsg, scan.indexName(), equalTo(indexName));
}
+ private static IgniteDistribution affinity() {
+ return IgniteDistributions.affinity(0, new Random().nextInt(), DEFAULT_ZONE_ID);
Review Comment:
Why not 1 ?)
##########
modules/client/src/test/java/org/apache/ignite/client/PartitionAwarenessTest.java:
##########
@@ -457,7 +456,7 @@ private Table defaultTable() {
private Table table(String name) {
// Create table on both servers with the same ID.
- var tableId = UUID.randomUUID();
Review Comment:
Maybe a random positive number?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -909,6 +924,24 @@ public int readInt() {
return val;
}
+ /** {@inheritDoc} */
Review Comment:
```suggestion
```
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/DirectMessageWriter.java:
##########
@@ -116,6 +116,16 @@ public boolean writeInt(String name, int val) {
return stream.lastFinished();
}
+ /** {@inheritDoc} */
Review Comment:
```suggestion
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/JoinColocationPlannerTest.java:
##########
@@ -77,14 +77,18 @@ public void joinSameTableSimpleAff() throws Exception {
assertThat(invalidPlanMsg, join.getRight(), instanceOf(IgniteIndexScan.class));
}
+ private static int randomTableId() {
+ return new Random().nextInt();
Review Comment:
why not 1 ?)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTest.java:
##########
@@ -192,6 +192,10 @@ public ColocationGroup colocationGroup(MappingQueryContext ctx) {
assertEquals(2, plan.fragments().size());
}
+ private static int randomTableId() {
+ return new Random().nextInt();
Review Comment:
why not 1 ?)
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/index/IndexValidatorImplTest.java:
##########
@@ -40,13 +39,20 @@
*/
@ExtendWith(ConfigurationExtension.class)
public class IndexValidatorImplTest {
+ private static final int TABLE_ID = 1;
+
@InjectConfiguration("mock.tables.fooTable {columns.column0 {type.type: STRING}}")
private TablesConfiguration tablesConfig;
+ @BeforeEach
+ void setupTableConfig() {
+ tablesConfig.tables().get("fooTable").id().update(TABLE_ID);
Review Comment:
should we wait for the future?
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/JoinCommutePlannerTest.java:
##########
@@ -83,12 +83,16 @@ public IgniteDistribution distribution() {
@Override
public IgniteDistribution distribution() {
- return IgniteDistributions.affinity(0, UUID.randomUUID(), DEFAULT_ZONE_ID);
+ return IgniteDistributions.affinity(0, randomTableId(), DEFAULT_ZONE_ID);
}
}
);
}
+ private static int randomTableId() {
+ return new Random().nextInt();
Review Comment:
why not 1 ?)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/TableSpoolPlannerTest.java:
##########
@@ -82,4 +83,8 @@ public void tableSpoolDistributed() throws Exception {
checkSplitAndSerialization(phys, publicSchema);
}
+
+ private static IgniteDistribution affinity() {
+ return IgniteDistributions.affinity(0, new Random().nextInt(), DEFAULT_ZONE_ID);
Review Comment:
why not 1 ?)
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexDescriptor.java:
##########
@@ -180,6 +181,17 @@ private static BinaryTupleSchema createSchema(List<SortedIndexColumnDescriptor>
return BinaryTupleSchema.create(elements);
}
+ @Nullable
+ private static TableView findTableById(int tableId, NamedListView<? extends TableView> tablesView) {
Review Comment:
```suggestion
private static @Nullable TableView findTableById(int tableId, NamedListView<? extends TableView> tablesView) {
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/UnionPlannerTest.java:
##########
@@ -109,9 +109,13 @@ private IgniteSchema prepareSchema() {
.add("NAME", f.createJavaType(String.class))
.add("SALARY", f.createJavaType(Double.class))
.build(),
- IgniteDistributions.affinity(0, UUID.randomUUID(), DEFAULT_ZONE_ID)
+ IgniteDistributions.affinity(0, randomTableId(), DEFAULT_ZONE_ID)
);
return publicSchema;
}
+
+ private static int randomTableId() {
+ return new Random().nextInt();
Review Comment:
why not 1 ?)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/SortedIndexSpoolPlannerTest.java:
##########
@@ -100,6 +101,10 @@ public void testNotColocatedEqJoin() throws Exception {
assertNull(searchBounds.get(1));
}
+ private static IgniteDistribution affinity() {
+ return IgniteDistributions.affinity(0, new Random().nextInt(), DEFAULT_ZONE_ID);
Review Comment:
why not 1 ?)
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -1121,7 +1124,7 @@ static boolean invalid(RocksIterator it) {
*/
private static ReadResult wrapUncommittedValue(RowId rowId, byte[] valueBytes, @Nullable HybridTimestamp newestCommitTs) {
UUID txId = bytesToUuid(valueBytes, TX_ID_OFFSET);
- UUID commitTableId = bytesToUuid(valueBytes, TABLE_ID_OFFSET);
+ int commitTableId = GridUnsafe.getInt(valueBytes, GridUnsafe.BYTE_ARR_OFF + TABLE_ID_OFFSET);
Review Comment:
Please use static import
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill merged pull request #2093: IGNITE-19531 Switch table IDs from UUIDs to integers
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #2093:
URL: https://github.com/apache/ignite-3/pull/2093
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #2093: IGNITE-19531 Switch table IDs from UUIDs to integers
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #2093:
URL: https://github.com/apache/ignite-3/pull/2093#discussion_r1206604680
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/HashIndexPlannerTest.java:
##########
@@ -64,13 +65,17 @@ public void hashIndexIsAppliedForEquiCondition() throws Exception {
assertThat(invalidPlanMsg, scan.indexName(), equalTo(indexName));
}
+ private static IgniteDistribution affinity() {
+ return IgniteDistributions.affinity(0, new Random().nextInt(), DEFAULT_ZONE_ID);
Review Comment:
Originally there was a random table ID, so it makes sense to make sure that the table IDs are different at each call. Random IDs don't guarantee uniqueness, so I changed it to using a global counter.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2093: IGNITE-19531 Switch table IDs from UUIDs to integers
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2093:
URL: https://github.com/apache/ignite-3/pull/2093#discussion_r1206603559
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/index/IndexValidatorImplTest.java:
##########
@@ -45,8 +46,8 @@ public class IndexValidatorImplTest {
private TablesConfiguration tablesConfig;
@BeforeEach
- void setupTableConfig() {
- tablesConfig.tables().get("fooTable").id().update(TABLE_ID);
+ void setupTableConfig() throws Exception {
+ tablesConfig.tables().get("fooTable").id().update(TABLE_ID).get(1, TimeUnit.SECONDS);
Review Comment:
Please use `org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher#willSucceedFast` or similar
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ptupitsyn commented on pull request #2093: IGNITE-19531 Switch table IDs from UUIDs to integers
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on PR #2093:
URL: https://github.com/apache/ignite-3/pull/2093#issuecomment-1563259245
Client-related changes (Java, C#) look good to me.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] arcolight commented on pull request #2093: IGNITE-19531 Switch table IDs from UUIDs to integers
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on PR #2093:
URL: https://github.com/apache/ignite-3/pull/2093#issuecomment-1563957349
C++ changes LGTM.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org