IGNITE-10645: SQL: Avoid key/val ownership resolution of a field in r…

…untime. This closes apache#5657.

modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryUtils.java

@@ -563,10 +563,6 @@ public static void processBinaryMeta(GridKernalContext ctx, QueryEntity qryEntit
         Map<String, Integer> precision  = qryEntity.getFieldsPrecision();
         Map<String, Integer> scale = qryEntity.getFieldsScale();
 
-        // We have to distinguish between empty and null keyFields when the key is not of SQL type -
-        // when a key is not of SQL type, absence of a field in nonnull keyFields tell us that this field
-        // is a value field, and null keyFields tells us that current configuration
-        // does not tell us anything about this field's ownership.
         boolean hasKeyFields = (keyFields != null);
 
         boolean isKeyClsSqlType = isSqlType(d.keyClass());
@@ -580,23 +576,31 @@ public static void processBinaryMeta(GridKernalContext ctx, QueryEntity qryEntit
             }
         }
 
+        // We are creating binary properties for all the fields, even if field is of sql type (keyFieldName or
+        // valueFieldName). In that case we rely on the fact, that binary property's methods value() and
+        // setValue() will never get called, because there is no value to extract, key/val object itself is a
+        // value.
         for (Map.Entry<String, String> entry : fields.entrySet()) {
-            Boolean isKeyField;
+            String fieldName = entry.getKey();
+            String fieldType = entry.getValue();
 
-            if (isKeyClsSqlType) // We don't care about keyFields in this case - it might be null, or empty, or anything
+            boolean isKeyField;
+
+            if (isKeyClsSqlType)
+                // Entire key is not field of itself, even if it is set in "keyFields".
                 isKeyField = false;
             else
-                isKeyField = (hasKeyFields ? keyFields.contains(entry.getKey()) : null);
+                isKeyField = hasKeyFields && keyFields.contains(fieldName);
 
-            boolean notNull = notNulls != null && notNulls.contains(entry.getKey());
+            boolean notNull = notNulls != null && notNulls.contains(fieldName);
 
-            Object dfltVal = dlftVals != null ? dlftVals.get(entry.getKey()) : null;
+            Object dfltVal = dlftVals != null ? dlftVals.get(fieldName) : null;
 
-            QueryBinaryProperty prop = buildBinaryProperty(ctx, entry.getKey(),
-                U.classForName(entry.getValue(), Object.class, true),
+            QueryBinaryProperty prop = buildBinaryProperty(ctx, fieldName,
+                U.classForName(fieldType, Object.class, true),
                 d.aliases(), isKeyField, notNull, dfltVal,
-                precision == null ? -1 : precision.getOrDefault(entry.getKey(), -1),
-                scale == null ? -1 : scale.getOrDefault(entry.getKey(), -1));
+                precision == null ? -1 : precision.getOrDefault(fieldName, -1),
+                scale == null ? -1 : scale.getOrDefault(fieldName, -1));
 
             d.addProperty(prop, false);
         }
@@ -799,7 +803,7 @@ else if (idxTyp != null)
      * @return Binary property.
      */
     public static QueryBinaryProperty buildBinaryProperty(GridKernalContext ctx, String pathStr,
-        Class<?> resType, Map<String, String> aliases, @Nullable Boolean isKeyField, boolean notNull, Object dlftVal,
+        Class<?> resType, Map<String, String> aliases, boolean isKeyField, boolean notNull, Object dlftVal,
         int precision, int scale) {
         String[] path = pathStr.split("\\.");
 

modules/core/src/main/java/org/apache/ignite/internal/processors/query/property/QueryBinaryProperty.java

@@ -18,7 +18,6 @@
 package org.apache.ignite.internal.processors.query.property;
 
 import org.apache.ignite.IgniteCheckedException;
-import org.apache.ignite.IgniteLogger;
 import org.apache.ignite.binary.BinaryField;
 import org.apache.ignite.binary.BinaryObject;
 import org.apache.ignite.binary.BinaryObjectBuilder;
@@ -28,8 +27,6 @@
 import org.apache.ignite.internal.binary.BinaryObjectExImpl;
 import org.apache.ignite.internal.processors.query.GridQueryProperty;
 import org.apache.ignite.internal.util.typedef.F;
-import org.apache.ignite.internal.util.typedef.internal.U;
-import org.jetbrains.annotations.Nullable;
 
 /**
  * Binary property.
@@ -38,9 +35,6 @@ public class QueryBinaryProperty implements GridQueryProperty {
     /** Kernal context. */
     private final GridKernalContext ctx;
 
-    /** Logger. */
-    private final IgniteLogger log;
-
     /** Property name. */
     private String propName;
 
@@ -53,18 +47,15 @@ public class QueryBinaryProperty implements GridQueryProperty {
     /** Result class. */
     private Class<?> type;
 
-    /** */
-    private volatile int isKeyProp;
+    /** Defines where value should be extracted from : cache entry's key or value. */
+    private final boolean isKeyProp;
 
     /** Binary field to speed-up deserialization. */
     private volatile BinaryField field;
 
     /** Flag indicating that we already tried to take a field. */
     private volatile boolean fieldTaken;
 
-    /** Whether user was warned about missing property. */
-    private volatile boolean warned;
-
     /** */
     private final boolean notNull;
 
@@ -84,29 +75,23 @@ public class QueryBinaryProperty implements GridQueryProperty {
      * @param propName Property name.
      * @param parent Parent property.
      * @param type Result type.
-     * @param key {@code true} if key property, {@code false} otherwise, {@code null}  if unknown.
+     * @param key {@code true} if key property, {@code false} otherwise.
      * @param alias Field alias.
      * @param notNull {@code true} if null value is not allowed.
      * @param defaultValue Default value.
      * @param precision Precision.
      * @param scale Scale.
      */
     public QueryBinaryProperty(GridKernalContext ctx, String propName, QueryBinaryProperty parent,
-        Class<?> type, @Nullable Boolean key, String alias, boolean notNull, Object defaultValue,
+        Class<?> type, boolean key, String alias, boolean notNull, Object defaultValue,
         int precision, int scale) {
         this.ctx = ctx;
-
-        log = ctx.log(QueryBinaryProperty.class);
-
         this.propName = propName;
         this.alias = F.isEmpty(alias) ? propName : alias;
         this.parent = parent;
         this.type = type;
         this.notNull = notNull;
-
-        if (key != null)
-            this.isKeyProp = key ? 1 : -1;
-
+        this.isKeyProp = key;
         this.defaultValue = defaultValue;
         this.precision = precision;
         this.scale = scale;
@@ -126,33 +111,12 @@ public QueryBinaryProperty(GridKernalContext ctx, String propName, QueryBinaryPr
                 throw new IgniteCheckedException("Non-binary object received as a result of property extraction " +
                     "[parent=" + parent + ", propName=" + propName + ", obj=" + obj + ']');
         }
-        else {
-            int isKeyProp0 = isKeyProp;
-
-            if (isKeyProp0 == 0) {
-                // Key is allowed to be a non-binary object here.
-                // We check value before key consistently with ClassProperty.
-                if (val instanceof BinaryObject && ((BinaryObject)val).hasField(propName))
-                    isKeyProp = isKeyProp0 = -1;
-                else if (key instanceof BinaryObject && ((BinaryObject)key).hasField(propName))
-                    isKeyProp = isKeyProp0 = 1;
-                else {
-                    if (!warned) {
-                        U.warn(log, "Neither key nor value have property \"" + propName + "\" " +
-                            "(is cache indexing configured correctly?)");
-
-                        warned = true;
-                    }
-
-                    return null;
-                }
-            }
-
-            obj = isKeyProp0 == 1 ? key : val;
-        }
+        else
+            obj = isKeyProp ? key : val;
 
         if (obj instanceof BinaryObject) {
             BinaryObject obj0 = (BinaryObject) obj;
+
             return fieldValue(obj0);
         }
         else if (obj instanceof BinaryObjectBuilder) {
@@ -273,13 +237,7 @@ private Object fieldValue(BinaryObject obj) {
 
     /** {@inheritDoc} */
     @Override public boolean key() {
-        int isKeyProp0 = isKeyProp;
-
-        if (isKeyProp0 == 0)
-            throw new IllegalStateException("Ownership flag not set for binary property. Have you set 'keyFields'" +
-                " property of QueryEntity in programmatic or XML configuration?");
-
-        return isKeyProp0 == 1;
+        return isKeyProp;
     }
 
     /** {@inheritDoc} */

modules/core/src/test/java/org/apache/ignite/client/FunctionalTest.java

@@ -169,7 +169,10 @@ public void testCacheConfiguration() throws Exception {
                             SimpleEntry::getKey, SimpleEntry::getValue, (a, b) -> a, LinkedHashMap::new
                         ))
                     )
-                    .setKeyFields(Collections.singleton("id"))
+                    // During query normalization null keyFields become empty set.
+                    // Set empty collection for comparator.
+                    .setKeyFields(Collections.emptySet())
+                    .setKeyFieldName("id")
                     .setNotNullFields(Collections.singleton("id"))
                     .setDefaultFieldValues(Collections.singletonMap("id", 0))
                     .setIndexes(Collections.singletonList(new QueryIndex("id", true, "IDX_EMPLOYEE_ID")))

modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/CacheBinaryKeyConcurrentQueryTest.java

@@ -17,6 +17,7 @@
 
 package org.apache.ignite.internal.processors.cache;
 
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.concurrent.Callable;
@@ -229,6 +230,8 @@ private CacheConfiguration cacheConfiguration(String name, CacheAtomicityMode at
         qryEntity.addQueryField("id", Integer.class.getName(), null);
         qryEntity.addQueryField("val", Integer.class.getName(), null);
 
+        qryEntity.setKeyFields(Collections.singleton("id"));
+
         qryEntity.setIndexes(F.asList(new QueryIndex("id"), new QueryIndex("val")));
 
         ccfg.setQueryEntities(F.asList(qryEntity));

modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCacheDistributedJoinCollocatedAndNotTest.java

@@ -19,7 +19,8 @@
 
 import java.io.Serializable;
 import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.ignite.Ignite;
@@ -80,7 +81,8 @@ public class IgniteCacheDistributedJoinCollocatedAndNotTest extends GridCommonAb
             entity.addQueryField("id", Integer.class.getName(), null);
             entity.addQueryField("affKey", Integer.class.getName(), null);
             entity.addQueryField("name", String.class.getName(), null);
-            entity.setKeyFields(Collections.singleton("affKey"));
+
+            entity.setKeyFields(new HashSet<>(Arrays.asList("id", "affKey")));
 
             ccfg.setQueryEntities(F.asList(entity));
 

modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCacheJoinQueryWithAffinityKeyTest.java

@@ -18,6 +18,7 @@
 package org.apache.ignite.internal.processors.cache;
 
 import java.io.Serializable;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -360,6 +361,7 @@ private CacheConfiguration cacheConfiguration(CacheMode cacheMode,
         QueryEntity person = new QueryEntity();
         person.setKeyType(personKeyType);
         person.setValueType(Person.class.getName());
+        person.setKeyFields(Collections.singleton("id"));
         person.addQueryField("orgId", Integer.class.getName(), null);
         person.addQueryField("id", Integer.class.getName(), null);
         person.addQueryField("name", String.class.getName(), null);