You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/02/17 14:23:58 UTC
[GitHub] [ignite-3] AMashenkov opened a new pull request #672: IGNITE-16266 Add unique id for indexes
AMashenkov opened a new pull request #672:
URL: https://github.com/apache/ignite-3/pull/672
https://issues.apache.org/jira/browse/IGNITE-16266
--
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] zstan commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r840432235
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/SqlSchemaManagerImpl.java
##########
@@ -106,6 +111,18 @@ public IgniteTable tableById(UUID id) {
return table;
}
+ /** {@inheritDoc} */
+ @Override
+ public InternalSortedIndex indexById(UUID id, String idxName) {
+ InternalSortedIndex idx = indexManager.getIndexById(id);
+
+ if (idx == null) {
+ throw new IndexNotFoundException(idxName, id);
Review comment:
refactored here.
--
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] zstan commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r838614754
##########
File path: modules/index-api/src/main/java/org/apache/ignite/internal/idx/IndexManager.java
##########
@@ -102,4 +102,12 @@ InternalSortedIndex createIndex(
* @throws NodeStoppingException If an implementation stopped before the method was invoked.
*/
CompletableFuture<InternalSortedIndex> indexAsync(String idxCanonicalName);
+
+ /**
+ * Check index presence with current id.
+ *
+ * @param id Index id.
+ * @return {@code true} if found.
+ */
+ boolean getIndexById(UUID id);
Review comment:
I make : InternalSortedIndex getIndexById(UUID id);
--
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] korlov42 commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r840405546
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelInputEx.java
##########
@@ -38,8 +40,17 @@
/**
* Returns table by its id.
*
- * @param tag Tag under which id is serialised.
+ * @param tag Tag under which id is serialized.
* @return A table with given id.
*/
RelOptTable getTableById(String tag);
+
+ /**
+ * Check that appropriate index is available.
+ *
+ * @param tag Tag represents id serialization.
+ * @param idxName Index name.
Review comment:
I believe the index name doesn't matter here
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelInputEx.java
##########
@@ -38,8 +40,17 @@
/**
* Returns table by its id.
*
- * @param tag Tag under which id is serialised.
+ * @param tag Tag under which id is serialized.
* @return A table with given id.
*/
RelOptTable getTableById(String tag);
+
+ /**
+ * Check that appropriate index is available.
Review comment:
```suggestion
* Returns an index by its id.
```
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/AbstractIndexScan.java
##########
@@ -88,8 +98,8 @@ protected RelWriter explainTerms0(RelWriter pw) {
* Get index name.
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
*/
Review comment:
could you please fix javadoc as well?
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/SqlSchemaManagerImpl.java
##########
@@ -106,6 +111,18 @@ public IgniteTable tableById(UUID id) {
return table;
}
+ /** {@inheritDoc} */
+ @Override
+ public InternalSortedIndex indexById(UUID id, String idxName) {
+ InternalSortedIndex idx = indexManager.getIndexById(id);
+
+ if (idx == null) {
+ throw new IndexNotFoundException(idxName, id);
Review comment:
This doesn't seem right to pass an `idxName` param only for case when index is not found. Probably, there should be one more constructor of `IndexNotFoundException` accepting the `Id` only
##########
File path: modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/externalize/RelJsonReaderTest.java
##########
@@ -71,4 +76,45 @@ void fromJson() {
assertThat(node.getTable().unwrap(IgniteTable.class), is(igniteTableMock));
Mockito.verify(schemaMock).tableById(tableId);
}
+
+ @Test
+ void fromJsonIdxScan() {
+ UUID indexId = UUID.randomUUID();
+ UUID tableId = UUID.randomUUID();
+
+ IgniteTable igniteTableMock = mock(IgniteTable.class);
+ InternalSortedIndex igniteIdxMock = mock(InternalSortedIndex.class);
+ when(igniteTableMock.getStatistic()).thenReturn(new Statistic() {});
+ when(mock(RelInputEx.class).getIndexById(any(), any())).thenReturn(igniteIdxMock);
+ when(igniteTableMock.getRowType(any())).thenReturn(mock(RelDataType.class));
+ when(igniteTableMock.unwrap(InternalIgniteTable.class)).thenReturn(mock(InternalIgniteTable.class));
+
+ SqlSchemaManager schemaMock = mock(SqlSchemaManager.class);
+
+ when(schemaMock.tableById(tableId)).thenReturn(igniteTableMock);
+ when(schemaMock.indexById(any(UUID.class), any())).thenReturn(igniteIdxMock);
+
+ String json = ""
+ + "{\n"
+ + " \"rels\" : [ {\n"
+ + " \"id\" : \"0\",\n"
+ + " \"relOp\" : \"IgniteIndexScan\",\n"
+ + " \"index\" : \"idx0\",\n"
+ + " \"table\" : [\"PUBLIC\", \"TEST\"],\n"
+ + " \"indexId\" : \"" + indexId + "\",\n"
+ + " \"tableId\" : \"" + tableId + "\",\n"
+ + " \"inputs\" : [ ]\n"
+ + " } ]\n"
+ + "}";
+
+ RelNode node = RelJsonReader.fromJson(schemaMock, json);
+
+ assertThat(node, isA(IgniteIndexScan.class));
+ assertThat(node.getTable(), notNullValue());
+ assertThat(node.getTable().unwrap(IgniteTable.class), is(igniteTableMock));
+ assertThat(IgniteTestUtils.getFieldValue(node, AbstractIndexScan.class, "index"), notNullValue());
+
+ Mockito.verify(schemaMock).tableById(tableId);
+ Mockito.verify(schemaMock).indexById(any(UUID.class), any());
Review comment:
```suggestion
Mockito.verify(schemaMock).indexById(indexId, any());
```
--
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] zstan commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r840430680
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelInputEx.java
##########
@@ -38,8 +40,17 @@
/**
* Returns table by its id.
*
- * @param tag Tag under which id is serialised.
+ * @param tag Tag under which id is serialized.
* @return A table with given id.
*/
RelOptTable getTableById(String tag);
+
+ /**
+ * Check that appropriate index is available.
+ *
+ * @param tag Tag represents id serialization.
+ * @param idxName Index name.
Review comment:
What kind of information will obtain user in case of error in such a case ? I think that UUID is also not for user eyes but it`s discussable.
--
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] korlov42 commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r838339996
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/AbstractIndexScan.java
##########
@@ -51,6 +57,7 @@
protected AbstractIndexScan(RelInput input) {
super(input);
idxName = input.getString("index");
Review comment:
Looks like `idxName` is actually used to request the index from the table. Thus, I believe it would be better to keep `IgniteIndex` within this scan node.
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteIndex.java
##########
@@ -35,7 +37,9 @@
private final String idxName;
- private final InternalSortedIndex idx;
+ private final UUID id;
+
+ private @Nullable final InternalSortedIndex idx;
Review comment:
delegating index should never be null
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJsonReader.java
##########
@@ -75,8 +80,8 @@
* FromJson.
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
*/
- public static <T extends RelNode> T fromJson(SqlSchemaManager schemaManager, String json) {
- RelJsonReader reader = new RelJsonReader(schemaManager);
+ public static <T extends RelNode> T fromJson(SqlSchemaManager schemaManager, IndexManager idxManager, String json) {
+ RelJsonReader reader = new RelJsonReader(schemaManager, idxManager);
Review comment:
I believe that `schemaManager` should be enough to work with every schema object.
##########
File path: modules/index-api/src/main/java/org/apache/ignite/internal/idx/IndexManager.java
##########
@@ -102,4 +102,12 @@ InternalSortedIndex createIndex(
* @throws NodeStoppingException If an implementation stopped before the method was invoked.
*/
CompletableFuture<InternalSortedIndex> indexAsync(String idxCanonicalName);
+
+ /**
+ * Check index presence with current id.
+ *
+ * @param id Index id.
+ * @return {@code true} if found.
+ */
+ boolean getIndexById(UUID id);
Review comment:
```suggestion
InternalSortedIndex index(UUID id);
```
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteIndex.java
##########
@@ -44,26 +48,27 @@
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
*/
public IgniteIndex(RelCollation collation, String name, InternalIgniteTable tbl) {
Review comment:
It's better to drop this constructor and the next one because someone could accidentally use it in production code.
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelInputEx.java
##########
@@ -38,8 +39,17 @@
/**
* Returns table by its id.
*
- * @param tag Tag under which id is serialised.
+ * @param tag Tag under which id is serialized.
* @return A table with given id.
*/
RelOptTable getTableById(String tag);
+
+ /**
+ * Check that appropriate index is available.
+ *
+ * @param tag Tag represents id serialization.
+ * @param idxName Index name.
+ * @throws IndexNotFoundException If not found.
+ */
+ void checkIndexById(String tag, String idxName);
Review comment:
why do we need a `idxName` param here?
--
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] zstan commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r838451619
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteIndex.java
##########
@@ -44,26 +48,27 @@
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
*/
public IgniteIndex(RelCollation collation, String name, InternalIgniteTable tbl) {
Review comment:
i append @TestOnly annotation.
--
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] zstan commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r838464680
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelInputEx.java
##########
@@ -38,8 +39,17 @@
/**
* Returns table by its id.
*
- * @param tag Tag under which id is serialised.
+ * @param tag Tag under which id is serialized.
* @return A table with given id.
*/
RelOptTable getTableById(String tag);
+
+ /**
+ * Check that appropriate index is available.
+ *
+ * @param tag Tag represents id serialization.
+ * @param idxName Index name.
+ * @throws IndexNotFoundException If not found.
+ */
+ void checkIndexById(String tag, String idxName);
Review comment:
I fix RelInputImpl#checkIndexById method, my fail, name needed only for possible error message.
--
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] zstan commented on a change in pull request #672: IGNITE-16266 Add unique id for indexes
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #672:
URL: https://github.com/apache/ignite-3/pull/672#discussion_r838451619
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteIndex.java
##########
@@ -44,26 +48,27 @@
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
*/
public IgniteIndex(RelCollation collation, String name, InternalIgniteTable tbl) {
Review comment:
i append @TestOnly annotation.
--
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