This is an automated email from the ASF dual-hosted git repository. xxyu pushed a commit to branch kylin5 in repository https://gitbox.apache.org/repos/asf/kylin.git
commit 7fe52b87411451debd40f2b92eee6111f764de6c Author: Ruixuan Zhang <ruixuan.zh...@kyligence.io> AuthorDate: Wed Jan 11 15:21:26 2023 +0800 KYLIN-5450 fix NPE while col_order is not required --- .../rest/controller/NIndexPlanController.java | 10 --- .../rest/controller/IndexPlanControllerTest.java | 20 ++--- .../kylin/rest/service/IndexPlanService.java | 4 + .../kylin/rest/service/IndexPlanServiceTest.java | 90 ++++++++++++++++++++++ 4 files changed, 104 insertions(+), 20 deletions(-) diff --git a/src/metadata-server/src/main/java/org/apache/kylin/rest/controller/NIndexPlanController.java b/src/metadata-server/src/main/java/org/apache/kylin/rest/controller/NIndexPlanController.java index 3bca9f0ecf..24124071a5 100644 --- a/src/metadata-server/src/main/java/org/apache/kylin/rest/controller/NIndexPlanController.java +++ b/src/metadata-server/src/main/java/org/apache/kylin/rest/controller/NIndexPlanController.java @@ -21,7 +21,6 @@ package org.apache.kylin.rest.controller; import static org.apache.kylin.common.constant.HttpConstant.HTTP_VND_APACHE_KYLIN_JSON; import static org.apache.kylin.common.constant.HttpConstant.HTTP_VND_APACHE_KYLIN_V4_PUBLIC_JSON; import static org.apache.kylin.common.exception.code.ErrorCodeServer.LAYOUT_LIST_EMPTY; -import static org.apache.kylin.common.exception.code.ErrorCodeServer.SHARD_BY_COLUMN_NOT_IN_INDEX; import java.util.List; import java.util.Set; @@ -144,19 +143,10 @@ public class NIndexPlanController extends NBasicController { checkRequiredArg(MODEL_ID, request.getModelId()); checkRequiredArg("id", request.getId()); modelService.validateCCType(request.getModelId(), request.getProject()); - List<String> shardByColumns = request.getShardByColumns(); - List<String> colOrder = request.getColOrder(); - checkShardbyCol(shardByColumns, colOrder); val response = fusionIndexService.updateTableIndex(request.getProject(), request); return new EnvelopeResponse<>(KylinException.CODE_SUCCESS, response, ""); } - private void checkShardbyCol(List<String> shardByColumns, List<String> colOrder) { - if (!colOrder.containsAll(shardByColumns)) { - throw new KylinException(SHARD_BY_COLUMN_NOT_IN_INDEX); - } - } - @Deprecated @ApiOperation(value = "deleteTableIndex", tags = { "AI" }, notes = "Update URL: {project}, Update Param: project") @DeleteMapping(value = "/table_index/{id:.+}") diff --git a/src/metadata-server/src/test/java/org/apache/kylin/rest/controller/IndexPlanControllerTest.java b/src/metadata-server/src/test/java/org/apache/kylin/rest/controller/IndexPlanControllerTest.java index bcb9ef7ec1..7a1de9708c 100644 --- a/src/metadata-server/src/test/java/org/apache/kylin/rest/controller/IndexPlanControllerTest.java +++ b/src/metadata-server/src/test/java/org/apache/kylin/rest/controller/IndexPlanControllerTest.java @@ -18,9 +18,7 @@ package org.apache.kylin.rest.controller; import static org.apache.kylin.common.constant.HttpConstant.HTTP_VND_APACHE_KYLIN_JSON; -import static org.apache.kylin.common.exception.code.ErrorCodeServer.SHARD_BY_COLUMN_NOT_IN_INDEX; -import org.apache.kylin.common.exception.KylinException; import org.apache.kylin.common.util.JsonUtil; import org.apache.kylin.common.util.NLocalFileMetadataTestCase; import org.apache.kylin.common.util.Pair; @@ -36,7 +34,6 @@ import org.apache.kylin.rest.service.FusionIndexService; import org.apache.kylin.rest.service.IndexPlanService; import org.apache.kylin.rest.service.ModelService; import org.junit.After; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.mockito.InjectMocks; @@ -162,13 +159,16 @@ public class IndexPlanControllerTest extends NLocalFileMetadataTestCase { } @Test - public void testUpdateTableIndex() { + public void testUpdateTableIndex() throws Exception { CreateTableIndexRequest tableIndexRequest = CreateTableIndexRequest.builder().project("default") - .modelId("89af4ee2-2cdb-4b07-b39e-4c29856309aa").id(20000010000L) - .colOrder(Lists.newArrayList("1", "0", "2")).shardByColumns(Lists.newArrayList("4")) - .sortByColumns(Lists.newArrayList("0", "2")).build(); - Assert.assertThrows(SHARD_BY_COLUMN_NOT_IN_INDEX.getMsg(), KylinException.class, () -> { - indexPlanController.updateTableIndex(tableIndexRequest); - }); + .modelId("89af4ee2-2cdb-4b07-b39e-4c29856309aa").id(20000010001L).colOrder(Lists + .newArrayList("TEST_KYLIN_FACT.TRANS_ID", "TEST_SITES.SITE_NAME", "TEST_KYLIN_FACT.CAL_DT")) + .sortByColumns(Lists.newArrayList()).build(); + Mockito.when(indexPlanService.updateTableIndex(Mockito.anyString(), Mockito.any(CreateTableIndexRequest.class))) + .thenReturn(new BuildIndexResponse()); + mockMvc.perform(MockMvcRequestBuilders.put("/api/index_plans/table_index") + .contentType(MediaType.APPLICATION_JSON).content(JsonUtil.writeValueAsString(tableIndexRequest)) + .accept(MediaType.parseMediaType(HTTP_VND_APACHE_KYLIN_JSON))) + .andExpect(MockMvcResultMatchers.status().isOk()); } } diff --git a/src/modeling-service/src/main/java/org/apache/kylin/rest/service/IndexPlanService.java b/src/modeling-service/src/main/java/org/apache/kylin/rest/service/IndexPlanService.java index e45016aeeb..62d597daf5 100644 --- a/src/modeling-service/src/main/java/org/apache/kylin/rest/service/IndexPlanService.java +++ b/src/modeling-service/src/main/java/org/apache/kylin/rest/service/IndexPlanService.java @@ -21,6 +21,7 @@ import static org.apache.kylin.common.exception.ServerErrorCode.PERMISSION_DENIE import static org.apache.kylin.common.exception.code.ErrorCodeServer.INDEX_DUPLICATE; import static org.apache.kylin.common.exception.code.ErrorCodeServer.LAYOUT_LIST_EMPTY; import static org.apache.kylin.common.exception.code.ErrorCodeServer.LAYOUT_NOT_EXISTS; +import static org.apache.kylin.common.exception.code.ErrorCodeServer.SHARD_BY_COLUMN_NOT_IN_INDEX; import java.util.ArrayList; import java.util.Arrays; @@ -235,6 +236,9 @@ public class IndexPlanService extends BasicService implements TableIndexPlanSupp newLayout.setOwner(BasicService.getUsername()); newLayout.setManual(true); newLayout.setIndexRange(request.getIndexRange()); + if (!newLayout.getColOrder().containsAll(newLayout.getShardByColumns())) { + throw new KylinException(SHARD_BY_COLUMN_NOT_IN_INDEX); + } Map<Integer, String> layoutOverride = Maps.newHashMap(); if (request.getLayoutOverrideIndexes() != null) { diff --git a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/IndexPlanServiceTest.java b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/IndexPlanServiceTest.java index 2a8db33ac3..c5f257e3fb 100644 --- a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/IndexPlanServiceTest.java +++ b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/IndexPlanServiceTest.java @@ -19,6 +19,7 @@ package org.apache.kylin.rest.service; import static org.apache.kylin.common.exception.code.ErrorCodeServer.INDEX_DUPLICATE; import static org.apache.kylin.common.exception.code.ErrorCodeServer.LAYOUT_NOT_EXISTS; +import static org.apache.kylin.common.exception.code.ErrorCodeServer.SHARD_BY_COLUMN_NOT_IN_INDEX; import static org.apache.kylin.metadata.cube.model.IndexEntity.Source.CUSTOM_TABLE_INDEX; import static org.apache.kylin.metadata.cube.model.IndexEntity.Source.RECOMMENDED_TABLE_INDEX; import static org.apache.kylin.metadata.model.SegmentStatusEnum.READY; @@ -1447,4 +1448,93 @@ public class IndexPlanServiceTest extends SourceTestCase { val response2 = indexPlanService.getShardByColumns("default", modelId); Assert.assertFalse(response2.isShowLoadData()); } + + @Test + public void testCheckShardByColumns() { + CreateTableIndexRequest tableIndexRequest = CreateTableIndexRequest.builder().project("default") + .modelId("89af4ee2-2cdb-4b07-b39e-4c29856309aa").id(20000010000L) + .colOrder(Lists.newArrayList("TEST_KYLIN_FACT.TRANS_ID", "TEST_SITES.SITE_NAME", + "TEST_KYLIN_FACT.CAL_DT")) + .shardByColumns(Lists.newArrayList("TEST_KYLIN_FACT.LSTG_SITE_ID")).sortByColumns(Lists.newArrayList()) + .build(); + + Assert.assertThrows(SHARD_BY_COLUMN_NOT_IN_INDEX.getMsg(), KylinException.class, () -> { + indexPlanService.updateTableIndex("default", tableIndexRequest); + }); + } + + @Test + public void testUpdateTableIndexWithNullColOrder() { + String project = "default"; + String modelId = "89af4ee2-2cdb-4b07-b39e-4c29856309aa"; + long layoutId = 20000010001L; + CreateTableIndexRequest tableIndexRequest = CreateTableIndexRequest.builder().project(project).modelId(modelId) + .id(layoutId).shardByColumns(Lists.newArrayList()).sortByColumns(Lists.newArrayList()).build(); + BuildIndexResponse response = indexPlanService.updateTableIndex(project, tableIndexRequest); + Assert.assertEquals(BuildIndexResponse.BuildIndexType.NORM_BUILD, response.getType()); + LayoutEntity layoutEntity = NIndexPlanManager.getInstance(KylinConfig.getInstanceFromEnv(), project) + .getIndexPlan(modelId).getLayoutEntity(layoutId); + Assert.assertEquals(Lists.newArrayList(1, 0, 2), layoutEntity.getColOrder()); + } + + @Test + public void testUpdateTableIndexWithNullColOrderThrowsException() { + String project = "default"; + String modelId = "89af4ee2-2cdb-4b07-b39e-4c29856309aa"; + long layoutId = 20000010001L; + CreateTableIndexRequest tableIndexRequest = CreateTableIndexRequest.builder().project(project).modelId(modelId) + .id(layoutId).shardByColumns(Lists.newArrayList("TEST_KYLIN_FACT.TRANS_ID")) + .sortByColumns(Lists.newArrayList()).build(); + Assert.assertThrows(SHARD_BY_COLUMN_NOT_IN_INDEX.getMsg(), KylinException.class, () -> { + indexPlanService.updateTableIndex("default", tableIndexRequest); + }); + } + + @Test + public void testUpdateTableIndexWithNullShardByCols() { + String project = "default"; + String modelId = "89af4ee2-2cdb-4b07-b39e-4c29856309aa"; + long layoutId = 20000010001L; + CreateTableIndexRequest tableIndexRequest = CreateTableIndexRequest + .builder().project(project).modelId(modelId).id(layoutId).colOrder(Lists + .newArrayList("TEST_KYLIN_FACT.TRANS_ID", "TEST_SITES.SITE_NAME", "TEST_KYLIN_FACT.CAL_DT")) + .sortByColumns(Lists.newArrayList()).build(); + BuildIndexResponse response = indexPlanService.updateTableIndex(project, tableIndexRequest); + Assert.assertEquals(BuildIndexResponse.BuildIndexType.NORM_BUILD, response.getType()); + LayoutEntity layoutEntity = NIndexPlanManager.getInstance(KylinConfig.getInstanceFromEnv(), project) + .getIndexPlan(modelId).getLayoutEntity(layoutId); + Assert.assertEquals(Lists.newArrayList(1, 0, 2), layoutEntity.getColOrder()); + } + + @Test + public void testUpdateTableIndexWithCreateEmptyIndex() { + String project = "default"; + String modelId = "89af4ee2-2cdb-4b07-b39e-4c29856309aa"; + long layoutId = 20000180001L; + CreateTableIndexRequest tableIndexRequest = CreateTableIndexRequest.builder().project(project).modelId(modelId) + .id(layoutId).colOrder(Lists.newArrayList()).sortByColumns(Lists.newArrayList()).build(); + BuildIndexResponse response = indexPlanService.updateTableIndex(project, tableIndexRequest); + Assert.assertEquals(BuildIndexResponse.BuildIndexType.NORM_BUILD, response.getType()); + IndexPlan indexPlan = NIndexPlanManager.getInstance(KylinConfig.getInstanceFromEnv(), project) + .getIndexPlan(modelId); + Assert.assertTrue( + indexPlan.getAllLayouts().stream().anyMatch(layoutEntity -> layoutEntity.getColOrder().size() == 0)); + } + + @Test + public void testUpdateTableIndexWithUpdateEmptyIndex() { + String project = "default"; + String modelId = "89af4ee2-2cdb-4b07-b39e-4c29856309aa"; + long layoutId = 20000010001L; + CreateTableIndexRequest tableIndexRequest = CreateTableIndexRequest.builder().project("default") + .modelId(modelId).id(layoutId).colOrder(Lists.newArrayList()).sortByColumns(Lists.newArrayList()) + .build(); + BuildIndexResponse response = indexPlanService.updateTableIndex("default", tableIndexRequest); + Assert.assertEquals(BuildIndexResponse.BuildIndexType.NORM_BUILD, response.getType()); + IndexPlan indexPlan = NIndexPlanManager.getInstance(KylinConfig.getInstanceFromEnv(), project) + .getIndexPlan(modelId); + Assert.assertTrue(indexPlan.getLayoutEntity(layoutId).isToBeDeleted()); + Assert.assertTrue( + indexPlan.getAllLayouts().stream().anyMatch(layoutEntity -> layoutEntity.getColOrder().size() == 0)); + } }