You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/05 09:13:18 UTC
[GitHub] [geode] albertogpz opened a new pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
albertogpz opened a new pull request #6096:
URL: https://github.com/apache/geode/pull/6096
Several changes have been made to solve issues
found when running queries targeting a Map field
with and without indexes in the Map field.
- For queries without an index in the Map field
the behavior has been changed so that
for queries like "positions['SUN'] = null" instead
of returning those entries fulfiling
that condition and also those entries
where the Map does not contain the 'SUN' key
or the Map is null, it now returns
only entries where the Map contains the
'SUN' key and the value is equal to null.
- As a consequence of the above, queries
without an index in the Map field like
"positions['SUN'] != null" will now return
any entry that does not contain
the positions field Map with the 'SUN' key
and with value equal to null.
- For queries with an index in the Map field,
either specifying several keys or '*'
the behavior has been modified (fixed) so that
"!=" conditions targeting the indexed
Map fields return the same entries returned
when the index is not present. Prior to this
change, only entries containing the Map
were returned but if the Map was not
present in the entry then that entry was
not returned.
- Indexes for a field Map
specifying just one key, e.g.
positions['SUN'] have been changed
internally from Range indexes
to Map indexes in order to align
the behavior with Map indexes
(those that specified more than
one key or '*' for a Map field)
so that the space taken by this type
of index is equivalent to the
one taken by one specifying
more than one key for the Map.
This has also solved some wrong
behavior of these indexes that
sometimes did not return the expected
number of entries.
The drawback of this change is that
"!=" queries for the map field
will not make use of the index.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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] [geode] DonalEvans commented on pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #6096:
URL: https://github.com/apache/geode/pull/6096#issuecomment-794554193
The StressNewTest failures you're seeing in pre-checkin might be fixed if you rebase your branch on develop. They look very similar to the failures that were being caused by GEODE-8905, which has since been reverted.
----------------------------------------------------------------
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] [geode] albertogpz commented on pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6096:
URL: https://github.com/apache/geode/pull/6096#issuecomment-808285124
Opened a new pull request with some changes to the original solution but taking into account all the comments already received.
--
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] [geode] albertogpz commented on pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6096:
URL: https://github.com/apache/geode/pull/6096#issuecomment-794446665
> Overall, switching from JUnit Assert to AssertJ will provide much better failure messages that will really help anyone dealing with test failures.
>
> `\n` should always be replaced with `System.lineSeparator()`.
Thanks for your comments, Kirk. I thought of changing completely the files I edited to AssertJ but then I realized it would be better just to change to AssertJ my changes and leave the cleaning to another PR so that the core of this PR is not hidden by the other changes.
----------------------------------------------------------------
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] [geode] DonalEvans commented on a change in pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6096:
URL: https://github.com/apache/geode/pull/6096#discussion_r590790779
##########
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();
Review comment:
It looks like this use of the raw class instead of the parameterized one got missed in the last set of changes.
----------------------------------------------------------------
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] [geode] kirklund commented on a change in pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6096:
URL: https://github.com/apache/geode/pull/6096#discussion_r589867114
##########
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 {
Review comment:
The compiler warning here can be cleaned up by using `Region<Object, Object>` here.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/partitioned/PRIndexStatisticsJUnitTest.java
##########
@@ -274,7 +274,7 @@ public void testStatsForCompactMapRangeIndex() throws Exception {
}
// Both RangeIndex should be used
Review comment:
This comment seems incorrect now.
##########
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
Review comment:
Compiler warnings here and elsewhere in this method can be fixed by using `SelectResults<Object>` and `UncheckedUtils.uncheckedCast(qs.newQuery(query).execute());`
##########
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();
Review comment:
Compiler warnings can be cleaned up by using `HashMap<>()` throughout this method.
##########
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);
Review comment:
If output is strictly necessary in this method, it should be via a logger, not `System.out`. Instead, however, it might be better to assert on the query result to confirm that it matches what is expected beyond just having the correct size, so that any failure will be self explanatory and not require this debug logging.
----------------------------------------------------------------
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] [geode] albertogpz closed pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz closed pull request #6096:
URL: https://github.com/apache/geode/pull/6096
--
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] [geode] mhansonp commented on a change in pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6096:
URL: https://github.com/apache/geode/pull/6096#discussion_r601717922
##########
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:
+1 for consistency.
--
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] [geode] mhansonp commented on a change in pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6096:
URL: https://github.com/apache/geode/pull/6096#discussion_r601720307
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/IndexStatisticsJUnitTest.java
##########
@@ -370,7 +370,7 @@ public void testStatsForCompactMapRangeIndex() throws Exception {
assertEquals(0, keyIndexStats.getReadLockCount());
- assertEquals(100, keyIndexStats.getTotalUses());
Review comment:
Why did this change with no other code changes in the test method?
--
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] [geode] mhansonp commented on a change in pull request #6096: GEODE-9004: Fix issues with queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6096:
URL: https://github.com/apache/geode/pull/6096#discussion_r601720307
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/IndexStatisticsJUnitTest.java
##########
@@ -370,7 +370,7 @@ public void testStatsForCompactMapRangeIndex() throws Exception {
assertEquals(0, keyIndexStats.getReadLockCount());
- assertEquals(100, keyIndexStats.getTotalUses());
Review comment:
Why did this change with no other code changes in the test method?
--
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