kirklund commented on a change in pull request #6096:
URL: https://github.com/apache/geode/pull/6096#discussion_r589690197
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/query/functional/INOperatorJUnitTest.java
##########
@@ -225,11 +225,11 @@ public void testUNDEFINED() throws Exception {
q = CacheUtils.getQueryService().newQuery(" UNDEFINED IN SET(UNDEFINED)");
result = q.execute();
- assertThat(result).isEqualTo(QueryService.UNDEFINED);
+ assertThat(result).isEqualTo(TRUE);
Review comment:
Another syntax to consider: `assertThat(result).isTrue();`
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/IndexUseJUnitTest.java
##########
@@ -353,7 +353,7 @@ public void testCompactMapIndexUsageWithIndexOnSingleKey()
throws Exception {
Index i2 =
qs.createIndex("Index2", IndexType.FUNCTIONAL, "objs.liist[0]",
SEPARATOR + "testRgn objs");
- assertTrue(i2 instanceof CompactRangeIndex);
+ assertTrue(i2 instanceof CompactMapRangeIndex);
Review comment:
I highly recommend updating any test that you touch to use AssertJ
instead of JUnit Assert. You can even install an IntelliJ plugin that'll
automate most of the work for you.
JUnit assertions like this will produce a terrible failure message:
```
ava.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at
org.apache.geode.IndexStatisticsJUnitTest.testCompactMapIndexUsageWithIndexOnSingleKey(IndexStatisticsJUnitTest.java:356)
```
Changing to AssertJ:
```
assertThat(i2).isInstanceOf(CompactMapRangeIndex.class);
```
The failure message will then be:
```
java.lang.AssertionError:
Expecting:
java.lang.Object@735b478
to be an instance of:
java.util.Collection
but was instance of:
java.lang.Object
at
org.apache.geode.IndexStatisticsJUnitTest.testCompactMapIndexUsageWithIndexOnSingleKey(IndexStatisticsJUnitTest.java:356)
```
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/IndexUseJUnitTest.java
##########
@@ -1196,7 +1196,7 @@ public void
testCompactMapIndexUsageManyKeysWithVariousValueTypes() throws Excep
Index i5 = qs.createIndex("Index5", IndexType.FUNCTIONAL,
"itr1.testFields['complex']",
SEPARATOR + "testRgn itr1");
- assertTrue(i1 instanceof CompactRangeIndex);
+ assertTrue(i1 instanceof CompactMapRangeIndex);
Review comment:
AssertJ will work much better here.
##########
File path:
geode-junit/src/main/java/org/apache/geode/cache/query/data/Portfolio.java
##########
@@ -184,12 +184,14 @@ public int hashCode() {
public String toString() {
String out =
"Portfolio [ID=" + ID + " status=" + status + " type=" + type + "
pkid=" + pkid + "\n ";
- Iterator iter = positions.entrySet().iterator();
- while (iter.hasNext()) {
- Map.Entry entry = (Map.Entry) iter.next();
- out += entry.getKey() + ":" + entry.getValue() + ", ";
+ if (positions != null) {
+ Iterator iter = positions.entrySet().iterator();
+ while (iter.hasNext()) {
+ Map.Entry entry = (Map.Entry) iter.next();
+ out += entry.getKey() + ":" + entry.getValue() + ", ";
+ }
+ out += "\n P1:" + position1 + ", P2:" + position2;
Review comment:
Please change all instances of `\n` to `System.lineSeparator()`.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/query/internal/ResultsCollectionPdxDeserializerWrapper.java
##########
@@ -218,4 +218,12 @@ public void setElementType(ObjectType elementType) {
results.setElementType(elementType);
}
+ public String toString() {
+ String out = "size: " + size() + "\n";
+ Iterator iter = iterator();
+ while (iter.hasNext()) {
+ out += iter.next() + "\n";
+ }
+ return out;
+ }
Review comment:
All instances of `\n` that you find should be changed to
`System.lineSeparator()`.
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +341,155 @@ public void
testNullMapValuesInIndexOnLocalRegionForCompactMap() throws Exceptio
assertEquals(1, result.size());
}
+ @Test
+ public void testQueriesForValueInMapFieldWithoutIndex() throws Exception {
+ region =
+
CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+ testQueriesForValueInMapField(region, qs);
+ }
+
+ @Test
+ public void testQueriesForValueInMapFieldWithMapIndexWithOneKey() throws
Exception {
+ region =
+
CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 = qs.createIndex(INDEX_NAME, "positions['SUN']", SEPARATOR +
"portfolio ");
+ assertTrue(keyIndex1 instanceof CompactMapRangeIndex);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfValues();
+ assertEquals(3, keys);
+ assertEquals(1, mapIndexKeys);
+ assertEquals(3, values);
+ }
+
+ @Test
+ public void testQueriesForValueInMapFieldWithMapIndexWithSeveralKeys()
throws Exception {
+ region =
+
CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 =
+ qs.createIndex(INDEX_NAME, "positions['SUN', 'ERICSSON']", SEPARATOR +
"portfolio ");
+ assertTrue(keyIndex1 instanceof CompactMapRangeIndex);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfValues();
+ assertEquals(3, keys);
+ assertEquals(1, mapIndexKeys);
+ assertEquals(3, values);
+ }
+
+ @Test
+ public void testQueriesForValueInMapFieldWithMapIndexWithStar() throws
Exception {
+ region =
+
CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 = qs.createIndex(INDEX_NAME, "positions[*]", SEPARATOR +
"portfolio ");
+ assertTrue(keyIndex1 instanceof CompactMapRangeIndex);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex)
keyIndex1).internalIndexStats.getNumberOfValues();
+ assertEquals(5, keys);
+ assertEquals(4, mapIndexKeys);
+ assertEquals(5, values);
+ }
+
+ public void testQueriesForValueInMapField(Region region, QueryService qs)
throws Exception {
+ // Empty map
+ Portfolio p = new Portfolio(1, 1);
+ p.positions = new HashMap();
+ region.put(1, p);
+
+ // Map is null
+ Portfolio p2 = new Portfolio(2, 2);
+ p2.positions = null;
+ region.put(2, p2);
+
+ // Map with null value for "SUN" key
+ Portfolio p3 = new Portfolio(3, 3);
+ p3.positions = new HashMap();
+ p3.positions.put("IBM", "something");
+ p3.positions.put("SUN", null);
+ region.put(3, p3);
+
+ // Map with not null value for "SUN" key
+ Portfolio p4 = new Portfolio(4, 4);
+ p4.positions = new HashMap();
+ p4.positions.put("SUN", "nothing");
+ region.put(4, p4);
+
+ // Map with null key
+ Portfolio p5 = new Portfolio(5, 5);
+ p5.positions = new HashMap();
+ p5.positions.put("SUN", "more");
+ // The next one causes trouble with gfsh as json cannot show maps with
null keys
+ p5.positions.put(null, "empty");
+ region.put(5, p5);
+
+ // One more with map without the "SUN" key
+ Portfolio p6 = new Portfolio(6, 6);
+ p6.positions = new HashMap();
+ p6.positions.put("ERIC", "hey");
+ region.put(6, p6);
+
+ // One more with null map
+ Portfolio p7 = new Portfolio(7, 7);
+ p7.positions = null;
+ region.put(7, p7);
+
+ String query;
+ query = "select * from " + SEPARATOR + "portfolio p where
p.positions['SUN'] = null";
+ SelectResults result = (SelectResults) qs
+ .newQuery(query)
+ .execute();
+ System.out.println("Query: " + query + ", result: " + result);
+ assertEquals(1, result.size());
+
+ query = "select * from " + SEPARATOR + "portfolio p where
p.positions['SUN'] != null";
+ result = (SelectResults) qs
+ .newQuery(query)
+ .execute();
+ System.out.println("Query: " + query + ", result: " + result);
+ assertEquals(6, result.size());
+
+ query = "select * from " + SEPARATOR + "portfolio p where
p.positions['SUN'] = 'nothing'";
+ result = (SelectResults) qs
+ .newQuery(query)
+ .execute();
+ System.out.println("Query: " + query + ", result: " + result);
+ assertEquals(1, result.size());
+
+ query = "select * from " + SEPARATOR + "portfolio p where
p.positions['SUN'] != 'nothing'";
+ result = (SelectResults) qs
+ .newQuery(query)
+ .execute();
+ System.out.println("Query: " + query + ", result: " + result);
+ assertEquals(6, result.size());
+
+ query = "select * from " + SEPARATOR + "portfolio p";
+ result = (SelectResults) qs
+ .newQuery(query)
+ .execute();
+ System.out.println("Query: " + query + ", result: " + result);
+ assertEquals(7, result.size());
Review comment:
AssertJ will print out the entire result and why the assertion failed if
you change to:
```
assertThat(result).hasSize(7);
```
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/query/functional/IndexOperatorJUnitTest.java
##########
@@ -171,7 +171,7 @@ public void testWithUNDEFINED() throws Exception {
map.put("0", new Integer(11));
map.put("1", new Integer(12));
Object result = runQuery(map, QueryService.UNDEFINED);
- if (result != null)
+ if (result != QueryService.UNDEFINED)
Review comment:
`assertThat(result).isNotEqualTo(QueryService.UNDEFINED);`
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/query/functional/IndexOperatorJUnitTest.java
##########
@@ -145,7 +145,7 @@ public void testWithNULL() throws Exception {
map.put("0", new Integer(11));
map.put("1", new Integer(12));
Object result = runQuery(map, null);
- if (result != null)
+ if (result != QueryService.UNDEFINED)
Review comment:
I realize that you're editing code that isn't great but `fail();` is a
terrible anti-pattern in testing.
Try this:
```
assertThat(result).isNotEqualTo(QueryService.UNDEFINED);
```
----------------------------------------------------------------
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:
[email protected]
