[GitHub] [ignite] korlov42 commented on a change in pull request #7982: IGNITE-13200 SQL create index on invalid data type

[GitBox <gi...@apache.org>]

korlov42 commented on a change in pull request #7982:
URL: https://github.com/apache/ignite/pull/7982#discussion_r452017691



##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/query/CreateIndexOnInvalidDataTypeTest.java
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.List;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.binary.BinaryObjectBuilder;
+import org.apache.ignite.cache.QueryEntity;
+import org.apache.ignite.cache.affinity.rendezvous.RendezvousAffinityFunction;
+import org.apache.ignite.cache.query.FieldsQueryCursor;
+import org.apache.ignite.cache.query.SqlFieldsQuery;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.failure.StopNodeFailureHandler;
+import org.apache.ignite.internal.processors.cache.CachePartialUpdateCheckedException;
+import org.apache.ignite.internal.processors.cache.index.AbstractIndexingCommonTest;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Tests for local query execution in lazy mode.

Review comment:
       Wrong description

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/IgniteQueryErrorCode.java
##########
@@ -135,6 +135,9 @@
     /** Transaction serialization error. */
     public static final int TRANSACTION_SERIALIZATION_ERROR = 5005;
 
+    /** Field type mismatch. e.g.: cause is ClassCastException. */

Review comment:
       It's better to format as a link: `{@link ClassCastException}`

##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
##########
@@ -839,13 +846,38 @@ public boolean remove(CacheDataRow row) throws IgniteCheckedException {
      * @param idx Index to add row to.
      * @param row Row to add to index.
      * @param prevRow Previous row state, if any.
+     * @param prevErr Error on index add

Review comment:
       missing dot

##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/query/CreateIndexOnInvalidDataTypeTest.java
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.List;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.binary.BinaryObjectBuilder;
+import org.apache.ignite.cache.QueryEntity;
+import org.apache.ignite.cache.affinity.rendezvous.RendezvousAffinityFunction;
+import org.apache.ignite.cache.query.FieldsQueryCursor;
+import org.apache.ignite.cache.query.SqlFieldsQuery;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.failure.StopNodeFailureHandler;
+import org.apache.ignite.internal.processors.cache.CachePartialUpdateCheckedException;
+import org.apache.ignite.internal.processors.cache.index.AbstractIndexingCommonTest;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Tests for local query execution in lazy mode.
+ */
+public class CreateIndexOnInvalidDataTypeTest extends AbstractIndexingCommonTest {
+    /** Keys count. */
+    private static final int KEY_CNT = 10;
+
+    /** {@inheritDoc} */
+    @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+        return super.getConfiguration(igniteInstanceName)
+            .setFailureHandler(new StopNodeFailureHandler())
+            .setDataStorageConfiguration(new DataStorageConfiguration()
+                .setDefaultDataRegionConfiguration(new DataRegionConfiguration()
+                    .setPersistenceEnabled(true)
+                )
+            );
+    }
+
+    /** {@inheritDoc} */
+    @Override protected void beforeTest() throws Exception {
+        super.beforeTest();
+
+        cleanPersistenceDir();
+    }
+
+    /** {@inheritDoc} */
+    @Override protected void afterTest() throws Exception {
+        stopAllGrids();
+
+        super.afterTest();
+    }
+
+    /**
+     * Check case when index is created on the field with invalid data type.
+     * Test steps:
+     * - create cache with query entity describes a table;
+     * - fill data (real data contains the fields that was not described by query entity);
+     * - execute alter table (ADD COLUMN with invalid type for exists field);
+     * - try to create index for the new field - exception must be throw;
+     * - checks that index isn't created.
+     */
+    @Test
+    public void testCreateIndexOnInvalidData() throws Exception {
+        startGrid();
+
+        grid().cluster().state(ClusterState.ACTIVE);
+
+        IgniteCache<Integer, Value> c = grid().createCache(new CacheConfiguration<Integer, Value>()
+            .setName("test")
+            .setSqlSchema("PUBLIC")
+            .setQueryEntities(Collections.singleton(new QueryEntity(Integer.class, Value.class)
+                    .setTableName("TEST")
+                )
+            )
+            .setBackups(1)
+            .setAffinity(new RendezvousAffinityFunction(false, 10)));
+
+        for (int i = 0; i < KEY_CNT; ++i)
+            c.put(i, new Value(i));
+
+        sql("ALTER TABLE TEST ADD COLUMN (VAL_DATE DATE)");
+
+        sql("CREATE INDEX TEST_VAL_INT_IDX ON TEST(VAL_INT)");
+
+        GridTestUtils.assertThrowsAnyCause(log, () -> {
+                sql("CREATE INDEX TEST_VAL_DATE_IDX ON TEST(VAL_DATE)");
+
+                return null;
+            },
+            IgniteSQLException.class, "java.util.Date cannot be cast to java.sql.Date");
+
+        // Wait for node stop if it is initiated by FailureHandler
+        U.sleep(1000);
+
+        List<List<?>> res = sql("SELECT val_int FROM TEST where val_int > -1").getAll();
+
+        assertEquals(KEY_CNT, res.size());
+
+        GridTestUtils.assertThrowsAnyCause(log, () -> {
+                sql("DROP INDEX TEST_VAL_DATE_IDX");
+
+                return null;
+            },
+            IgniteSQLException.class, "Index doesn't exist: TEST_VAL_DATE_IDX");
+    }
+
+    /**
+     * Check case when row with invalid field is added.
+     * Test steps:
+     * - create table;
+     * - create two index;
+     * - try add entry - exception must be thrown;
+     * - remove the index for field with invalid type;
+     * - check that select query that uses the index for valid field is successful.
+     */
+    @Test
+    public void testAddInvalidDataToIndex() throws Exception {
+        startGrid();
+
+        grid().cluster().state(ClusterState.ACTIVE);
+
+        sql("CREATE TABLE TEST (ID INT PRIMARY KEY, VAL_INT INT, VAL_DATE DATE) " +
+            "WITH \"CACHE_NAME=test,VALUE_TYPE=ValueType0\"");
+
+        sql("CREATE INDEX TEST_0_VAL_DATE_IDX ON TEST(VAL_DATE)");
+        sql("CREATE INDEX TEST_1_VAL_INT_IDX ON TEST(VAL_INT)");
+
+        BinaryObjectBuilder bob = grid().binary().builder("ValueType0");
+
+        bob.setField("VAL_INT", 10);
+        bob.setField("VAL_DATE", new java.util.Date());
+
+        GridTestUtils.assertThrowsAnyCause(log, () -> {
+                grid().cache("test").put(0, bob.build());
+
+                return null;
+            },
+            CachePartialUpdateCheckedException.class, "Failed to update keys");
+
+        sql("DROP INDEX TEST_0_VAL_DATE_IDX");

Review comment:
       let's verify that after index was dropped you could insert values without any error 

##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/query/CreateIndexOnInvalidDataTypeTest.java
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.List;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.binary.BinaryObjectBuilder;
+import org.apache.ignite.cache.QueryEntity;
+import org.apache.ignite.cache.affinity.rendezvous.RendezvousAffinityFunction;
+import org.apache.ignite.cache.query.FieldsQueryCursor;
+import org.apache.ignite.cache.query.SqlFieldsQuery;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.failure.StopNodeFailureHandler;
+import org.apache.ignite.internal.processors.cache.CachePartialUpdateCheckedException;
+import org.apache.ignite.internal.processors.cache.index.AbstractIndexingCommonTest;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Tests for local query execution in lazy mode.
+ */
+public class CreateIndexOnInvalidDataTypeTest extends AbstractIndexingCommonTest {
+    /** Keys count. */
+    private static final int KEY_CNT = 10;
+
+    /** {@inheritDoc} */
+    @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+        return super.getConfiguration(igniteInstanceName)
+            .setFailureHandler(new StopNodeFailureHandler())
+            .setDataStorageConfiguration(new DataStorageConfiguration()
+                .setDefaultDataRegionConfiguration(new DataRegionConfiguration()
+                    .setPersistenceEnabled(true)
+                )
+            );
+    }
+
+    /** {@inheritDoc} */
+    @Override protected void beforeTest() throws Exception {
+        super.beforeTest();
+
+        cleanPersistenceDir();
+    }
+
+    /** {@inheritDoc} */
+    @Override protected void afterTest() throws Exception {
+        stopAllGrids();
+
+        super.afterTest();
+    }
+
+    /**
+     * Check case when index is created on the field with invalid data type.
+     * Test steps:
+     * - create cache with query entity describes a table;
+     * - fill data (real data contains the fields that was not described by query entity);
+     * - execute alter table (ADD COLUMN with invalid type for exists field);
+     * - try to create index for the new field - exception must be throw;
+     * - checks that index isn't created.
+     */
+    @Test
+    public void testCreateIndexOnInvalidData() throws Exception {
+        startGrid();
+
+        grid().cluster().state(ClusterState.ACTIVE);
+
+        IgniteCache<Integer, Value> c = grid().createCache(new CacheConfiguration<Integer, Value>()
+            .setName("test")
+            .setSqlSchema("PUBLIC")
+            .setQueryEntities(Collections.singleton(new QueryEntity(Integer.class, Value.class)
+                    .setTableName("TEST")
+                )
+            )

Review comment:
       ```
   .setQueryEntities(
       Collections.singleton(
           new QueryEntity(Integer.class, Value.class).setTableName("TEST")
       )
   )
   ```
   or
   ```
   .setQueryEntities(Collections.singleton(new QueryEntity(Integer.class, Value.class)
       .setTableName("TEST")))
   ```
   or
   ```
   .setQueryEntities(Collections.singleton(
       new QueryEntity(Integer.class, Value.class).setTableName("TEST")
   ))
   ```
   but not mix of them




----------------------------------------------------------------
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