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 2021/05/31 15:33:31 UTC
[GitHub] [ignite] javaronok opened a new pull request #9139: IGNITE-13723 Fixed ComplexSecondaryKeyUnwrapSelfTest test
javaronok opened a new pull request #9139:
URL: https://github.com/apache/ignite/pull/9139
Test ComplexSecondaryKeyUnwrapSelfTest was broken after a two-step query optimization commit (IGNITE-10305)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on pull request #9139: IGNITE-13723 Fixed ComplexSecondaryKeyUnwrapSelfTest test
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on pull request #9139:
URL: https://github.com/apache/ignite/pull/9139#issuecomment-853647041
Hi @javaronok ! Thanks for the effort. I've put some minor comments, could you please have a look?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9139: IGNITE-13723 Fixed ComplexSecondaryKeyUnwrapSelfTest test
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9139:
URL: https://github.com/apache/ignite/pull/9139#discussion_r644551636
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -55,15 +52,15 @@ public void testSecondaryIndexWithIntersectColumnsComplexPk() {
executeSql("CREATE INDEX ON " + tblName + "(id, name, city)");
- checkUsingIndexes(tblName, "'1'");
+ checkUsingIndexes(tblName, "'1'", 2, "Query should be splitted");
}
/**
* Test using secondary index with simple PK.
*/
@Test
public void testSecondaryIndexSimplePk() {
- HashMap<String, String> types = new HashMap() {
+ Map<String, String> types = new LinkedHashMap<String, String>() {
Review comment:
What is a purpose of this change?
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -97,7 +94,7 @@ public void testSecondaryIndexSimplePk() {
executeSql("CREATE INDEX ON " + tblName + "(id, name, city)");
- checkUsingIndexes(tblName, val);
+ checkUsingIndexes(tblName, val, 1, "Query with type column: " + type);
Review comment:
Can we use the same label as for other test, like "Query shouldn't be splitted"?
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
String explainSQL = "explain SELECT * FROM " + tblName + " WHERE ";
List<List<?>> results = executeSql(explainSQL + "id=1");
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, 2, assertLabel); // always merge_scan for non key (or affinity) fields condition
Review comment:
Please check rules how comments should be formatted there: https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-JavadocComments
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
Review comment:
assertLabel has to be replaced with abbreviation. Please check [1]. It can be configured for IDE.
[1] https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
String explainSQL = "explain SELECT * FROM " + tblName + " WHERE ";
List<List<?>> results = executeSql(explainSQL + "id=1");
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, 2, assertLabel); // always merge_scan for non key (or affinity) fields condition
results = executeSql(explainSQL + "id=1 and name=" + nameVal);
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, expResCnt, assertLabel);
results = executeSql(explainSQL + "id=1 and name=" + nameVal + " and age=0");
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, expResCnt, assertLabel);
}
/**
* Check that explain plan result shown using Secondary index and don't use scan.
*
* @param results result of execut explain plan query.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void assertUsingSecondaryIndex(List<List<?>> results) {
- assertEquals(2, results.size());
+ private void assertUsingSecondaryIndex(List<List<?>> results, int expResCnt, String assertLabel) {
Review comment:
USe abbr for assertLabel.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] javaronok commented on a change in pull request #9139: IGNITE-13723 Fixed ComplexSecondaryKeyUnwrapSelfTest test
Posted by GitBox <gi...@apache.org>.
javaronok commented on a change in pull request #9139:
URL: https://github.com/apache/ignite/pull/9139#discussion_r645472470
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -55,15 +52,15 @@ public void testSecondaryIndexWithIntersectColumnsComplexPk() {
executeSql("CREATE INDEX ON " + tblName + "(id, name, city)");
- checkUsingIndexes(tblName, "'1'");
+ checkUsingIndexes(tblName, "'1'", 2, "Query should be splitted");
}
/**
* Test using secondary index with simple PK.
*/
@Test
public void testSecondaryIndexSimplePk() {
- HashMap<String, String> types = new HashMap() {
+ Map<String, String> types = new LinkedHashMap<String, String>() {
Review comment:
The order of types from the declaration is cosily for debugging. Just for this. But I can revert it back.
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -97,7 +94,7 @@ public void testSecondaryIndexSimplePk() {
executeSql("CREATE INDEX ON " + tblName + "(id, name, city)");
- checkUsingIndexes(tblName, val);
+ checkUsingIndexes(tblName, val, 1, "Query with type column: " + type);
Review comment:
The parameter expResCnt in method should determine number items in the plan.
I'll separate method for the two-phase plan only.
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
Review comment:
Thanks for the link, I'll fix it
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
String explainSQL = "explain SELECT * FROM " + tblName + " WHERE ";
List<List<?>> results = executeSql(explainSQL + "id=1");
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, 2, assertLabel); // always merge_scan for non key (or affinity) fields condition
Review comment:
Thanks for the link, I'll fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9139: IGNITE-13723 Fixed ComplexSecondaryKeyUnwrapSelfTest test
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9139:
URL: https://github.com/apache/ignite/pull/9139#discussion_r644551636
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -55,15 +52,15 @@ public void testSecondaryIndexWithIntersectColumnsComplexPk() {
executeSql("CREATE INDEX ON " + tblName + "(id, name, city)");
- checkUsingIndexes(tblName, "'1'");
+ checkUsingIndexes(tblName, "'1'", 2, "Query should be splitted");
}
/**
* Test using secondary index with simple PK.
*/
@Test
public void testSecondaryIndexSimplePk() {
- HashMap<String, String> types = new HashMap() {
+ Map<String, String> types = new LinkedHashMap<String, String>() {
Review comment:
What is a purpose of this change?
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -97,7 +94,7 @@ public void testSecondaryIndexSimplePk() {
executeSql("CREATE INDEX ON " + tblName + "(id, name, city)");
- checkUsingIndexes(tblName, val);
+ checkUsingIndexes(tblName, val, 1, "Query with type column: " + type);
Review comment:
Can we use the same label as for other test, like "Query shouldn't be splitted"?
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
String explainSQL = "explain SELECT * FROM " + tblName + " WHERE ";
List<List<?>> results = executeSql(explainSQL + "id=1");
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, 2, assertLabel); // always merge_scan for non key (or affinity) fields condition
Review comment:
Please check rules how comments should be formatted there: https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-JavadocComments
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
Review comment:
assertLabel has to be replaced with abbreviation. Please check [1]. It can be configured for IDE.
[1] https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -106,30 +103,34 @@ public void testSecondaryIndexSimplePk() {
*
* @param tblName name of table which should be checked to using secondary indexes.
* @param nameVal Value for name param.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void checkUsingIndexes(String tblName, String nameVal) {
+ private void checkUsingIndexes(String tblName, String nameVal, int expResCnt, String assertLabel) {
String explainSQL = "explain SELECT * FROM " + tblName + " WHERE ";
List<List<?>> results = executeSql(explainSQL + "id=1");
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, 2, assertLabel); // always merge_scan for non key (or affinity) fields condition
results = executeSql(explainSQL + "id=1 and name=" + nameVal);
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, expResCnt, assertLabel);
results = executeSql(explainSQL + "id=1 and name=" + nameVal + " and age=0");
- assertUsingSecondaryIndex(results);
+ assertUsingSecondaryIndex(results, expResCnt, assertLabel);
}
/**
* Check that explain plan result shown using Secondary index and don't use scan.
*
* @param results result of execut explain plan query.
+ * @param expResCnt Expceted result count.
+ * @param assertLabel Assert label.
*/
- private void assertUsingSecondaryIndex(List<List<?>> results) {
- assertEquals(2, results.size());
+ private void assertUsingSecondaryIndex(List<List<?>> results, int expResCnt, String assertLabel) {
Review comment:
USe abbr for assertLabel.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9139: IGNITE-13723 Fixed ComplexSecondaryKeyUnwrapSelfTest test
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9139:
URL: https://github.com/apache/ignite/pull/9139#discussion_r645503973
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/ComplexSecondaryKeyUnwrapSelfTest.java
##########
@@ -55,15 +52,15 @@ public void testSecondaryIndexWithIntersectColumnsComplexPk() {
executeSql("CREATE INDEX ON " + tblName + "(id, name, city)");
- checkUsingIndexes(tblName, "'1'");
+ checkUsingIndexes(tblName, "'1'", 2, "Query should be splitted");
}
/**
* Test using secondary index with simple PK.
*/
@Test
public void testSecondaryIndexSimplePk() {
- HashMap<String, String> types = new HashMap() {
+ Map<String, String> types = new LinkedHashMap<String, String>() {
Review comment:
OK, let's use LinkedHashMap then.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on pull request #9139: IGNITE-13723 Fixed ComplexSecondaryKeyUnwrapSelfTest test
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on pull request #9139:
URL: https://github.com/apache/ignite/pull/9139#issuecomment-853647041
Hi @javaronok ! Thanks for the effort. I've put some minor comments, could you please have a look?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org