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/26 14:56:26 UTC
[GitHub] [geode] albertogpz opened a new pull request #6200: GEODE-9004: Fix issues in queries targeting a Map field
albertogpz opened a new pull request #6200:
URL: https://github.com/apache/geode/pull/6200
Queries in which map fields are involved
in the condition, sometimes do not return the same entries
if map indexes are used, compared to when map indexes
are not used.
Differences were found when the condition on the
map field was of type '!=' and also when the condition
on the map field was comparing the value with null.
Example1:
Running a query with the following condition:
positions['SUN'] = null
in a region with the following entries:
entry1.positions=null
entry2.positions={'a'=>'b'}
entry3.positions={'SUN'=>null}
- will return entry1 and entry3 if there are no
indexes defined.
- will return no entries if the following index is defined:
positions['SUN','c']
- will return entry3 if the following index is defined:
positions[*]
Example2:
Running a query with the following condition:
positions['SUN'] != '3'
in a region with the following entries:
entry1.positions=null
entry2.positions={'a'=>'b'}
entry3.positions={'SUN'=>'4'}
entry4.positions={'SUN'=>'3'}
entry5.positions={'SUN'=>null}
- will return all entries except for entry4 if:
there are no indexes defined.
- will return entry3 if the following index is defined:
positions['SUN','c']
- will return entry3 and entry5 if the following index is defined:
positions[*]
In order to have the same results for these
queries no matter if indexes are used,
the following changes have been made:
- When running queries, the result of getting the
value from a Map that does not contain the key used
to access the value will be
considered UNDEFINED instead of null.
This will imply that queries where the condition is
positions['SUN'] = null will not return entries
that do not have the 'SUN' key in the positions Map.
- Queries with map indexes will not be able to use
the indexes if the query is of type '!=' against
any of the indexed map fields.
Example:
for index positions['SUN', 'ERIC'] or
positions[*]
the following query will not use indexes:
positions['SUN'] != '44'
These changes must be documented accordingly
in the corresponding indexes section.
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 a change in pull request #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6200:
URL: https://github.com/apache/geode/pull/6200#discussion_r604441480
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,306 @@ 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 testQueriesForValueInMapFieldWithCompactMapIndexWithOneKey() throws Exception {
+ region =
+ CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 = qs.createIndex(INDEX_NAME, "positions['SUN']", SEPARATOR + "portfolio ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ // The number of keys must be equal to the number of different values the
+ // positions map takes in region entries for the 'SUN' key:
+ // ("nothing", "more", null) + 1 for any entry that does not have
+ // a value for the "SUN" key (UNDEFINED)
+ assertThat(keys).isEqualTo(4);
+ // mapIndexKeys must be zero because the index used is a range index and not a map index
+ assertThat(mapIndexKeys).isEqualTo(0);
+ // The number of values must be equal to the number of region entries
+ assertThat(values).isEqualTo(7);
+ // The index must be used in every query
+ assertThat(uses).isEqualTo(4);
+ }
+
+ @Test
+ public void testQueriesForValueInMapFieldWithCompactMapIndexWithSeveralKeys() throws Exception {
+ region =
+ CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 =
+ qs.createIndex(INDEX_NAME, "positions['SUN', 'ERICSSON']", SEPARATOR + "portfolio ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ // The number of keys must be equal to the number of different values the
+ // positions map takes for the 'SUN' key (null, "nothing", "more") plus
+ // the number of different values the positions map takes for the 'ERICSSON' key
+ // ("hey") for each entry in the region.
+ assertThat(keys).isEqualTo(4);
+ // The number of mapIndexKeys must be equal to the number of keys
+ // in the index that appear in region entries:
+ // 'SUN', 'ERICSSON'
+ assertThat(mapIndexKeys).isEqualTo(2);
+ // The number of values must be equal to the number of values the
+ // positions map takes for each key indexed in the map
+ // for each entry in the region:
+ // null, "nothing", "more", "hey", "more"
+ assertThat(values).isEqualTo(5);
+ // The index must not be used in queries with "!="
+ assertThat(uses).isEqualTo(2);
+ }
+
+ @Test
+ public void testQueriesForValueInMapFieldWithCompactMapIndexWithStar() throws Exception {
+ region =
+ CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 = qs.createIndex(INDEX_NAME, "positions[*]", SEPARATOR + "portfolio ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ // The number of keys must be equal to the number of different values the
+ // positions map takes for each key (null not included)
+ // for each entry in the region:
+ // "something", null, "nothing", "more", "hey", "tip"
+ assertThat(keys).isEqualTo(7);
Review comment:
This value is now in disagreement with the comment above, as the list of values includes only 6 values. Either the explanation in the comment needs to be changed, or the value being returned here is incorrect and there is a bug somewhere. Also, it is a little confusing in terms of what "null not included" is referring to, since the list of values includes null.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,306 @@ 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 testQueriesForValueInMapFieldWithCompactMapIndexWithOneKey() throws Exception {
+ region =
+ CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 = qs.createIndex(INDEX_NAME, "positions['SUN']", SEPARATOR + "portfolio ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ // The number of keys must be equal to the number of different values the
+ // positions map takes in region entries for the 'SUN' key:
+ // ("nothing", "more", null) + 1 for any entry that does not have
+ // a value for the "SUN" key (UNDEFINED)
+ assertThat(keys).isEqualTo(4);
+ // mapIndexKeys must be zero because the index used is a range index and not a map index
+ assertThat(mapIndexKeys).isEqualTo(0);
+ // The number of values must be equal to the number of region entries
+ assertThat(values).isEqualTo(7);
+ // The index must be used in every query
+ assertThat(uses).isEqualTo(4);
+ }
+
+ @Test
+ public void testQueriesForValueInMapFieldWithCompactMapIndexWithSeveralKeys() throws Exception {
+ region =
+ CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 =
+ qs.createIndex(INDEX_NAME, "positions['SUN', 'ERICSSON']", SEPARATOR + "portfolio ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ // The number of keys must be equal to the number of different values the
+ // positions map takes for the 'SUN' key (null, "nothing", "more") plus
+ // the number of different values the positions map takes for the 'ERICSSON' key
+ // ("hey") for each entry in the region.
+ assertThat(keys).isEqualTo(4);
+ // The number of mapIndexKeys must be equal to the number of keys
+ // in the index that appear in region entries:
+ // 'SUN', 'ERICSSON'
+ assertThat(mapIndexKeys).isEqualTo(2);
+ // The number of values must be equal to the number of values the
+ // positions map takes for each key indexed in the map
+ // for each entry in the region:
+ // null, "nothing", "more", "hey", "more"
+ assertThat(values).isEqualTo(5);
+ // The index must not be used in queries with "!="
+ assertThat(uses).isEqualTo(2);
+ }
+
+ @Test
+ public void testQueriesForValueInMapFieldWithCompactMapIndexWithStar() throws Exception {
+ region =
+ CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE).create("portfolio");
+ qs = CacheUtils.getQueryService();
+
+ keyIndex1 = qs.createIndex(INDEX_NAME, "positions[*]", SEPARATOR + "portfolio ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ // The number of keys must be equal to the number of different values the
+ // positions map takes for each key (null not included)
+ // for each entry in the region:
+ // "something", null, "nothing", "more", "hey", "tip"
+ assertThat(keys).isEqualTo(7);
+ // The number of mapIndexKeys must be equal to the number of different keys
+ // that appear in entries of the region:
+ // "IBM", "ERICSSON", "HP", "SUN", null
+ assertThat(mapIndexKeys).isEqualTo(5);
+ // The number of values must be equal to the number of values the
+ // positions map takes for each key (null not included)
+ // for each entry in the region:
+ // "something", null, "nothing", "more", "empty", "hey", "more", "tip"
+ assertThat(values).isEqualTo(8);
Review comment:
This comment is a little confusing in terms of what "null not included" is referring to, since the list of values includes null.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/partitioned/PRIndexStatisticsJUnitTest.java
##########
@@ -273,8 +273,8 @@ public void testStatsForCompactMapRangeIndex() throws Exception {
query.execute();
}
- // Both RangeIndex should be used
- assertEquals(100 /* Execution time */, keyIndexStats.getTotalUses());
+ // Index should not be used
Review comment:
Could this comment (and others in this class) be expanded slightly to explain why the index should not be used here? I am assuming it's because the query uses `!=`, but it would be good to make it explicit.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6200:
URL: https://github.com/apache/geode/pull/6200#discussion_r602562611
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,167 @@ 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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(4);
+ assertThat(mapIndexKeys).isEqualTo(0);
+ assertThat(values).isEqualTo(7);
+ assertThat(uses).isEqualTo(4);
+ }
+
+ @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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(3);
+ assertThat(mapIndexKeys).isEqualTo(1);
+ assertThat(values).isEqualTo(3);
+ assertThat(uses).isEqualTo(2);
+ }
+
+ @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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+ assertThat(keys).isEqualTo(5);
+ assertThat(mapIndexKeys).isEqualTo(4);
+ assertThat(values).isEqualTo(5);
+ assertThat(uses).isEqualTo(2);
+ }
+
+ public void testQueriesForValueInMapField(Region<Object, Object> 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
Review comment:
Is it necessary to have two region entries with null maps? p2 also has the same null `positions` field.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,167 @@ 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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(4);
+ assertThat(mapIndexKeys).isEqualTo(0);
+ assertThat(values).isEqualTo(7);
+ assertThat(uses).isEqualTo(4);
+ }
+
+ @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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(3);
+ assertThat(mapIndexKeys).isEqualTo(1);
+ assertThat(values).isEqualTo(3);
+ assertThat(uses).isEqualTo(2);
+ }
+
+ @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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+ assertThat(keys).isEqualTo(5);
+ assertThat(mapIndexKeys).isEqualTo(4);
+ assertThat(values).isEqualTo(5);
+ assertThat(uses).isEqualTo(2);
Review comment:
See above comment about it being unclear where these values are coming from.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,167 @@ 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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(4);
+ assertThat(mapIndexKeys).isEqualTo(0);
+ assertThat(values).isEqualTo(7);
+ assertThat(uses).isEqualTo(4);
Review comment:
It's not immediately obvious where these values are coming from. Woudl it be possible to add a comment explaining why they are what they are?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,167 @@ public void testNullMapValuesInIndexOnLocalRegionForCompactMap() throws Exceptio
assertEquals(1, result.size());
}
+ @Test
+ public void testQueriesForValueInMapFieldWithoutIndex() throws Exception {
+ region =
Review comment:
Multiple warnings in this file can be cleaned up by making `region` a `Region<Object, Object>` where it's declared.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,167 @@ 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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(4);
+ assertThat(mapIndexKeys).isEqualTo(0);
+ assertThat(values).isEqualTo(7);
+ assertThat(uses).isEqualTo(4);
+ }
+
+ @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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(3);
+ assertThat(mapIndexKeys).isEqualTo(1);
+ assertThat(values).isEqualTo(3);
+ assertThat(uses).isEqualTo(2);
Review comment:
See above comment about it being unclear where these values are coming from.
--
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] agingade commented on pull request #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-811539256
@albertogpz
Thanks for your contributions to make the query engine robust. Sorry for delay in responding...
Here is what I believe should be happening.
- The result for a query should be consistent in both using index or non-index case.
- The query engine returns UNDEFINED when it is unable to find the next level field.
E.g: if address.city and address is null (This is documented).
This is not same when you are looking for a non existing "key" in the map; UNDEFINED needs to be returned when positions is null and query is trying to access field from it.
E.g.: positions['*'] should be returning UNDEFINED.
- Query engine supports heterogenous objects stored in a region.
E.g: Employee or Customer.
Inline with supporting this, its designed/architected such that if a field is not found in the object it will be ignored.
E.g query with employeeID is not going to return customer objects unless it has that field.
- To be inline with the above design (query expectation), when a map field is not present available it should ignore that entry/object from adding to the result.
E.g. if positions['SUN'] if SUN key is not present query should ignore that object.
This is also different from null check.
If there is a SUN key with null value it should be returned for queries looking for null value. And non null check will return if the key is there and its value is not null.
Try the query with non map field and the behavior should be same.
Please let me know if you have any questions on the expected behavior.
The overall behavior of the query on map should be in-line with non-map fields.
The usage of index should not be avoided; unless the results are inconsistent with non-index query results. Instead of avoiding/blocking the use of index, it will be good to address the issues with indexed queries and make the behavior consistent.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-814834421
> > We had a small impromtu discussion with the query engine team and some research on how NoSQL treats null (or nil in some worlds)
> > ```
> > * in MongoDB we founnd that
> > `The { item : null } query matches documents that either contain the item field whose value is null or that do not contain the item field.`
> >
> > * in redis nil is returned if the field does not exist.
> > ```
> >
> >
> > Hence we think that the current behavior i.e. querying without indexes is the right result and we should try to make the indexes return the same result.
>
> @nabarunnag Thanks for your comments. If @agingade agrees (I will check his answers after having provided mine to his comments) I will look into this possibility.
@nabarunnag and @agingade : I have created a new pull request with a solution in which I keep the current behavior of the queries without indexes: https://github.com/apache/geode/pull/6279
Please, dismiss this PR and check the new one.
I have not been able to find a complete solution for the "allkeys" type of index (e.g. positions[*]) and with the solution, queries with this type of index and queries of type "!=" or comparing to null will not use the index even if configured. This is a limitation that could be tackled in a future PR.
--
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 a change in pull request #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #6200:
URL: https://github.com/apache/geode/pull/6200#discussion_r604235309
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,167 @@ 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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(4);
+ assertThat(mapIndexKeys).isEqualTo(0);
+ assertThat(values).isEqualTo(7);
+ assertThat(uses).isEqualTo(4);
Review comment:
I have added comments about these and the other. Nevertheless, there is no documentation on them so I am not 100% sure that they are working correctly. Anyhow, it is not something changed by this PR.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-808429795
I'm not a query expert, so I don't understand what the reasoning is behind map indexes no longer being used for queries using `!=`. Would it be possible to get an explanation of the motivation behind this change?
--
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 a change in pull request #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #6200:
URL: https://github.com/apache/geode/pull/6200#discussion_r604236057
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/index/MapRangeIndexMaintenanceJUnitTest.java
##########
@@ -341,6 +344,167 @@ 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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(4);
+ assertThat(mapIndexKeys).isEqualTo(0);
+ assertThat(values).isEqualTo(7);
+ assertThat(uses).isEqualTo(4);
+ }
+
+ @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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+
+ assertThat(keys).isEqualTo(3);
+ assertThat(mapIndexKeys).isEqualTo(1);
+ assertThat(values).isEqualTo(3);
+ assertThat(uses).isEqualTo(2);
+ }
+
+ @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 ");
+ assertThat(keyIndex1).isInstanceOf(CompactMapRangeIndex.class);
+ testQueriesForValueInMapField(region, qs);
+
+ long keys = ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfKeys();
+ long mapIndexKeys =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfMapIndexKeys();
+ long values =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getNumberOfValues();
+ long uses =
+ ((CompactMapRangeIndex) keyIndex1).internalIndexStats.getTotalUses();
+ assertThat(keys).isEqualTo(5);
+ assertThat(mapIndexKeys).isEqualTo(4);
+ assertThat(values).isEqualTo(5);
+ assertThat(uses).isEqualTo(2);
+ }
+
+ public void testQueriesForValueInMapField(Region<Object, Object> 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
Review comment:
Not really. I have changed a bit some of the entries to cover more cases and removed having two region entries with null maps.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-814286023
@agingade
Thanks for your comments. Please, let me answer inline to your questions.
> @albertogpz
> Thanks for your contributions to make the query engine robust. Sorry for delay in responding...
>
> Here is what I believe should be happening.
>
> * The result for a query should be consistent in both using index or non-index case.
>
This is what I tried to fix with this PR and the test new cases now show that results are the same with and without indexes while prior to the PR, they returned different results.
> * The query engine returns UNDEFINED when it is unable to find the next level field.
> E.g: if address.city and address is null (This is documented).
> This is not same when you are looking for a non existing "key" in the map; UNDEFINED needs to be returned when positions is null and query is trying to access field from it.
> E.g.: positions['*'] should be returning UNDEFINED.
So, you mean if positions is not null but it only contains a mapping for the 'SUN" key, positions["ERIC"] should return null instead of UNDEFINED?
>
> * Query engine supports heterogenous objects stored in a region.
> E.g: Employee or Customer.
> Inline with supporting this, its designed/architected such that if a field is not found in the object it will be ignored.
> E.g query with employeeID is not going to return customer objects unless it has that field.
>
ok. That should not have been changed.
> * To be inline with the above design (query expectation), when a map field is not present available it should ignore that entry/object from adding to the result.
> E.g. if positions['SUN'] if SUN key is not present query should ignore that object.
> This is also different from null check.
> If there is a SUN key with null value it should be returned for queries looking for null value. And non null check will return if the key is there and its value is not null.
Currently if you have an entry for which positions has the following mappings: {"SUN" => null} and another entry for which positions has the following mapping: {"ERICSSON" => "3"} querying for positions["SUN"] = null will return both entries.
With my PR, only the first one would be returned. What should be the right behavior?
> Try the query with non map field and the behavior should be same.
>
>
> Please let me know if you have any questions on the expected behavior.
>
> The overall behavior of the query on map should be in-line with non-map fields.
>
> The usage of index should not be avoided; unless the results are inconsistent with non-index query results. Instead of avoiding/blocking the use of index, it will be good to address the issues with indexed queries and make the behavior consistent.
>
I tried to support the use of indexes in all queries with my PR but I found no way to do it with != queries using an index of type: positions[*] (index on all keys) or positions["SUN", "ERICSSON"].
For the second type of index, I have an alternative solution in a draft PR that will allow the use of the index although it is more costly in memory (see https://github.com/apache/geode/pull/6238)
> Can you confirm above requirements are met in this PR.
--
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] agingade edited a comment on pull request #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
agingade edited a comment on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-811539256
@albertogpz
Thanks for your contributions to make the query engine robust. Sorry for delay in responding...
Here is what I believe should be happening.
- The result for a query should be consistent in both using index or non-index case.
- The query engine returns UNDEFINED when it is unable to find the next level field.
E.g: if address.city and address is null (This is documented).
This is not same when you are looking for a non existing "key" in the map; UNDEFINED needs to be returned when positions is null and query is trying to access field from it.
E.g.: positions['*'] should be returning UNDEFINED.
- Query engine supports heterogenous objects stored in a region.
E.g: Employee or Customer.
Inline with supporting this, its designed/architected such that if a field is not found in the object it will be ignored.
E.g query with employeeID is not going to return customer objects unless it has that field.
- To be inline with the above design (query expectation), when a map field is not present available it should ignore that entry/object from adding to the result.
E.g. if positions['SUN'] if SUN key is not present query should ignore that object.
This is also different from null check.
If there is a SUN key with null value it should be returned for queries looking for null value. And non null check will return if the key is there and its value is not null.
Try the query with non map field and the behavior should be same.
Please let me know if you have any questions on the expected behavior.
The overall behavior of the query on map should be in-line with non-map fields.
The usage of index should not be avoided; unless the results are inconsistent with non-index query results. Instead of avoiding/blocking the use of index, it will be good to address the issues with indexed queries and make the behavior consistent.
Can you confirm above requirements are met in this PR.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-814287858
> We had a small impromtu discussion with the query engine team and some research on how NoSQL treats null (or nil in some worlds)
>
> * in MongoDB we founnd that
> `The { item : null } query matches documents that either contain the item field whose value is null or that do not contain the item field.`
>
> * in redis nil is returned if the field does not exist.
>
>
> Hence we think that the current behavior i.e. querying without indexes is the right result and we should try to make the indexes return the same result.
@nabarunnag Thanks for your comments. If @agingade agrees (I will check his answers after having provided mine to his comments) I will look into this possibility.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz closed pull request #6200:
URL: https://github.com/apache/geode/pull/6200
--
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] nabarunnag commented on pull request #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-811341260
We had a small impromtu discussion with the query engine team and some research on how NoSQL treats null (or nil in some worlds)
- in MongoDB we founnd that
`The { item : null } query matches documents that either contain the item field whose value is null or that do not contain the item field.`
- in redis nil is returned if the field does not exist.
Hence we think that the current behavior i.e. querying without indexes is the right result and we should try to make the indexes return the same result.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-808420030
> The PR description says that "These changes must be documented accordingly in the corresponding indexes section." Is there a GEODE ticket for this docs work? If not, one should be created, or the docs changes included as part of this PR.
I agree, Donal. My plan is to update the documentation as part of this PR if the solution is accepted. Given that the outcome of some queries without indexes change and that indexes will not be used with queries using "!=" with Map indexes, I want to have first the blessing from the community and only then update the documentation.
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-808435119
> I'm not a query expert, so I don't understand what the reasoning is behind map indexes no longer being used for queries using `!=`. Would it be possible to get an explanation of the motivation behind this change?
With the current implementation, map indexes are used (if defined) in queries that have a condition using "!=" against a value in the map field indexed. The problem is that these queries offer different outcome if the index is defined vs when the index is not defined.
In this PR I have made the queries offer the same output no matter if the index is defined or not but at the expense of not using the index if the condition is "!=" when the index is defined. I have not been able to come up with a solution without losing this "feature". That's the reason behind "map indexes no longer being used for queries using `!=`."
--
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 #6200: GEODE-9004: Fix issues in queries targeting a Map field
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #6200:
URL: https://github.com/apache/geode/pull/6200#issuecomment-808411165
The PR description says that "These changes must be documented accordingly in the corresponding indexes section." Is there a GEODE ticket for this docs work? If not, one should be created, or the docs changes included as part of this PR.
--
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