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 618c2e37c72a40774cbfdb2222676b06210f4319
Author: Pengfei Zhan <pengfei.z...@kyligence.io>
AuthorDate: Fri Nov 11 22:57:51 2022 +0800
KYLIN-5402 fix performance of checking computed columns
1. promote the performance of checking computed columns;
2. promote the performance of saving model;
3. resolve the code complexity of ModelService#checkModelMeasures.
---
.../kylin/rest/service/ModelServiceBuildTest.java | 2 +-
.../kylin/rest/service/ModelSemanticHelper.java | 2 +-
.../apache/kylin/rest/service/ModelService.java | 79 ++++++++++++----------
.../kylin/rest/service/ModelServiceTest.java | 20 +++++-
.../kylin/rest/service/ModelServiceQueryTest.java | 2 +-
.../service/ModelServiceWithSecondStorageTest.java | 2 +-
.../apache/kylin/rest/util/AclPermissionUtil.java | 4 +-
7 files changed, 69 insertions(+), 42 deletions(-)
diff --git
a/src/data-loading-service/src/test/java/org/apache/kylin/rest/service/ModelServiceBuildTest.java
b/src/data-loading-service/src/test/java/org/apache/kylin/rest/service/ModelServiceBuildTest.java
index 72df60801a..de7cef0bf1 100644
---
a/src/data-loading-service/src/test/java/org/apache/kylin/rest/service/ModelServiceBuildTest.java
+++
b/src/data-loading-service/src/test/java/org/apache/kylin/rest/service/ModelServiceBuildTest.java
@@ -201,7 +201,7 @@ public class ModelServiceBuildTest extends SourceTestCase {
ReflectionTestUtils.setField(semanticService, "expandableMeasureUtil",
new ExpandableMeasureUtil((model, ccDesc) -> {
String ccExpression =
QueryUtil.massageComputedColumn(model, model.getProject(), ccDesc,
-
AclPermissionUtil.prepareQueryContextACLInfo(model.getProject(),
+ AclPermissionUtil.createAclInfo(model.getProject(),
semanticService.getCurrentUserGroups()));
ccDesc.setInnerExpression(ccExpression);
ComputedColumnEvalUtil.evaluateExprAndType(model, ccDesc);
diff --git
a/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelSemanticHelper.java
b/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelSemanticHelper.java
index f6866f2708..54190bcd77 100644
---
a/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelSemanticHelper.java
+++
b/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelSemanticHelper.java
@@ -131,7 +131,7 @@ public class ModelSemanticHelper extends BasicService {
private static final Logger logger =
LoggerFactory.getLogger(ModelSemanticHelper.class);
private final ExpandableMeasureUtil expandableMeasureUtil = new
ExpandableMeasureUtil((model, ccDesc) -> {
String ccExpression = QueryUtil.massageComputedColumn(model,
model.getProject(), ccDesc,
-
AclPermissionUtil.prepareQueryContextACLInfo(model.getProject(),
getCurrentUserGroups()));
+ AclPermissionUtil.createAclInfo(model.getProject(),
getCurrentUserGroups()));
ccDesc.setInnerExpression(ccExpression);
ComputedColumnEvalUtil.evaluateExprAndType(model, ccDesc);
});
diff --git
a/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelService.java
b/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelService.java
index 77fd3c6c5f..1f915c3dfa 100644
---
a/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelService.java
+++
b/src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelService.java
@@ -108,6 +108,7 @@ import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.exception.ExceptionUtils;
import org.apache.kylin.common.KapConfig;
import org.apache.kylin.common.KylinConfig;
+import org.apache.kylin.common.QueryContext;
import org.apache.kylin.common.event.ModelAddEvent;
import org.apache.kylin.common.event.ModelDropEvent;
import org.apache.kylin.common.exception.JobErrorCode;
@@ -303,6 +304,7 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
public static final Pattern VALID_NAME_FOR_MEASURE =
Pattern.compile("^[\\u4E00-\\u9FA5a-zA-Z0-9 _\\-()%?（）.]+$");
private static final List<String> MODEL_CONFIG_BLOCK_LIST =
Lists.newArrayList("kylin.index.rule-scheduler-data");
+ private static final Set<String> STRING_TYPE_SET =
Sets.newHashSet("STRING", "CHAR", "VARCHAR");
//The front-end supports only the following formats
private static final List<String> SUPPORTED_FORMATS =
ImmutableList.of("ZZ", "DD", "D", "Do", "dddd", "ddd", "dd", //
@@ -1714,8 +1716,9 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
@VisibleForTesting
public void checkFlatTableSql(NDataModel dataModel) {
- if (KylinConfig.getInstanceFromEnv().isUTEnv() ||
getManager(NProjectManager.class)
-
.getProject(dataModel.getProject()).getConfig().isSkipCheckFlatTable()) {
+ String project = dataModel.getProject();
+ ProjectInstance prjInstance =
getManager(NProjectManager.class).getProject(project);
+ if (KylinConfig.getInstanceFromEnv().isUTEnv() ||
prjInstance.getConfig().isSkipCheckFlatTable()) {
return;
}
if (getModelConfig(dataModel).skipCheckFlatTable()) {
@@ -1729,15 +1732,13 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
}
try {
- ProjectInstance projectInstance =
getManager(NProjectManager.class).getProject(dataModel.getProject());
- if (projectInstance.getSourceType() == ISourceAware.ID_SPARK
+ if (prjInstance.getSourceType() == ISourceAware.ID_SPARK
&& dataModel.getModelType() == NDataModel.ModelType.BATCH)
{
SparkSession ss = SparderEnv.getSparkSession();
String flatTableSql =
JoinedFlatTable.generateSelectDataStatement(dataModel, false);
- QueryParams queryParams = new
QueryParams(dataModel.getProject(), flatTableSql, "default", false);
- queryParams.setKylinConfig(projectInstance.getConfig());
- queryParams.setAclInfo(
-
AclPermissionUtil.prepareQueryContextACLInfo(dataModel.getProject(),
getCurrentUserGroups()));
+ QueryParams queryParams = new QueryParams(project,
flatTableSql, "default", false);
+ queryParams.setKylinConfig(prjInstance.getConfig());
+
queryParams.setAclInfo(AclPermissionUtil.createAclInfo(project,
getCurrentUserGroups()));
String pushdownSql = QueryUtil.massagePushDownSql(queryParams);
ss.sql(pushdownSql);
}
@@ -2163,27 +2164,38 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
}
}
- @VisibleForTesting
- public void checkModelMeasures(ModelRequest request) {
- Set<String> measureNames = new HashSet<>();
- Set<SimplifiedMeasure> measures = new HashSet<>();
- KylinConfig kylinConfig =
getManager(NProjectManager.class).getProject(request.getProject()).getConfig();
- int maxModelDimensionMeasureNameLength =
kylinConfig.getMaxModelDimensionMeasureNameLength();
-
+ private void checkMeasureNameValid(ModelRequest request) {
+ int maxLen =
NProjectManager.getProjectConfig(request.getProject()).getMaxModelDimensionMeasureNameLength();
+ String invalidPattern = MsgPicker.getMsg().getInvalidMeasureName();
for (SimplifiedMeasure measure : request.getSimplifiedMeasures()) {
- measure.setName(StringUtils.trim(measure.getName()));
- // check if the measure name is valid
- if (StringUtils.length(measure.getName()) >
maxModelDimensionMeasureNameLength
- || !checkIsValidMeasureName(measure.getName()))
- throw new KylinException(INVALID_NAME,
- String.format(Locale.ROOT,
MsgPicker.getMsg().getInvalidMeasureName(), measure.getName(),
- maxModelDimensionMeasureNameLength));
+ String name = measure.getName();
+ if (StringUtils.length(name) <= maxLen &&
checkIsValidMeasureName(name)) {
+ continue;
+ }
+ throw new KylinException(INVALID_NAME, String.format(Locale.ROOT,
invalidPattern, name, maxLen));
+ }
+ }
- // check duplicate measure names
- if (measureNames.contains(measure.getName()))
+ private void checkMeasureNameDuplicate(ModelRequest modelRequest) {
+ Set<String> measureNames = Sets.newHashSet();
+ for (SimplifiedMeasure measure : modelRequest.getSimplifiedMeasures())
{
+ if (measureNames.contains(measure.getName())) {
throw new KylinException(DUPLICATE_MEASURE_NAME,
String.format(Locale.ROOT,
MsgPicker.getMsg().getDuplicateMeasureName(), measure.getName()));
+ } else {
+ measureNames.add(measure.getName());
+ }
+ }
+ }
+ @VisibleForTesting
+ public void checkModelMeasures(ModelRequest request) {
+ Set<SimplifiedMeasure> measures = new HashSet<>();
+ request.getSimplifiedMeasures().forEach(measure ->
measure.setName(StringUtils.trim(measure.getName())));
+ checkMeasureNameValid(request);
+ checkMeasureNameDuplicate(request);
+
+ for (SimplifiedMeasure measure : request.getSimplifiedMeasures()) {
// check duplicate measure definitions
SimplifiedMeasure dupMeasure = null;
for (SimplifiedMeasure m : measures) {
@@ -2207,7 +2219,6 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
MsgPicker.getMsg().getDuplicateMeasureDefinition(),
measure.getName()));
}
- measureNames.add(measure.getName());
measures.add(measure);
}
}
@@ -2630,6 +2641,7 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
checkCCNameAmbiguity(model);
ComputedColumnDesc checkedCC = null;
+ QueryContext.AclInfo aclInfo =
AclPermissionUtil.createAclInfo(project, getCurrentUserGroups());
Set<String> excludedTables = getManager(FavoriteRuleManager.class,
project).getExcludedTables();
ExcludedLookupChecker checker = new
ExcludedLookupChecker(excludedTables, model.getJoinTables(), model);
for (ComputedColumnDesc cc : model.getComputedColumnDescs()) {
@@ -2645,8 +2657,7 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
MsgPicker.getMsg().getccOnAntiFlattenLookup(),
antiFlattenLookup));
}
ComputedColumnDesc.simpleParserCheck(cc.getExpression(),
model.getAliasMap().keySet());
- String innerExpression =
QueryUtil.massageComputedColumn(model, project, cc,
- AclPermissionUtil.prepareQueryContextACLInfo(project,
getCurrentUserGroups()));
+ String innerExpression =
QueryUtil.massageComputedColumn(model, project, cc, aclInfo);
cc.setInnerExpression(innerExpression);
//check by data source, this could be slow
@@ -2746,9 +2757,9 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
checkCCNameAmbiguity(model);
// Update CC expression from query transformers
+ QueryContext.AclInfo aclInfo =
AclPermissionUtil.createAclInfo(project, getCurrentUserGroups());
for (ComputedColumnDesc ccDesc : model.getComputedColumnDescs()) {
- String ccExpression = QueryUtil.massageComputedColumn(model,
project, ccDesc,
- AclPermissionUtil.prepareQueryContextACLInfo(project,
getCurrentUserGroups()));
+ String ccExpression = QueryUtil.massageComputedColumn(model,
project, ccDesc, aclInfo);
ccDesc.setInnerExpression(ccExpression);
TblColRef tblColRef = model.findColumn(ccDesc.getTableAlias(),
ccDesc.getColumnName());
tblColRef.getColumnDesc().setComputedColumn(ccExpression);
@@ -3599,7 +3610,7 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
}
String massagedFilterCond = QueryUtil.massageExpression(model,
model.getProject(), model.getFilterCondition(),
-
AclPermissionUtil.prepareQueryContextACLInfo(model.getProject(),
getCurrentUserGroups()), false);
+ AclPermissionUtil.createAclInfo(model.getProject(),
getCurrentUserGroups()), false);
String filterConditionWithTableName =
addTableNameIfNotExist(massagedFilterCond, model);
@@ -3853,7 +3864,7 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
}
public NDataModel updateResponseAcl(NDataModel model, String project) {
- List<NDataModel> models = updateResponseAcl(Arrays.asList(model),
project);
+ List<NDataModel> models =
updateResponseAcl(Collections.singletonList(model), project);
return models.get(0);
}
@@ -3905,8 +3916,6 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
}
}
- private static final Set<String> StringTypes = Sets.newHashSet("STRING",
"CHAR", "VARCHAR");
-
private boolean matchCCDataType(String actual, String expected) {
if (actual == null) {
return false;
@@ -3915,8 +3924,8 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
actual = actual.toUpperCase(Locale.ROOT);
expected = expected.toUpperCase(Locale.ROOT);
// char, varchar, string are of the same
- if (StringTypes.contains(actual)) {
- return StringTypes.contains(expected);
+ if (STRING_TYPE_SET.contains(actual)) {
+ return STRING_TYPE_SET.contains(expected);
}
// if actual type is of decimal type with no prec, scala
diff --git
a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ModelServiceTest.java
b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ModelServiceTest.java
index e8edf45716..26a2cf1eaf 100644
---
a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ModelServiceTest.java
+++
b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ModelServiceTest.java
@@ -18,6 +18,7 @@
package org.apache.kylin.rest.service;
+import static
org.apache.kylin.common.exception.ServerErrorCode.FAILED_EXECUTE_MODEL_SQL;
import static
org.apache.kylin.common.exception.ServerErrorCode.INVALID_PARTITION_COLUMN;
import static
org.apache.kylin.common.exception.ServerErrorCode.PERMISSION_DENIED;
import static
org.apache.kylin.common.exception.code.ErrorCodeServer.COMPUTED_COLUMN_CONFLICT;
@@ -289,7 +290,7 @@ public class ModelServiceTest extends SourceTestCase {
ReflectionTestUtils.setField(semanticService, "expandableMeasureUtil",
new ExpandableMeasureUtil((model, ccDesc) -> {
String ccExpression =
QueryUtil.massageComputedColumn(model, model.getProject(), ccDesc,
-
AclPermissionUtil.prepareQueryContextACLInfo(model.getProject(),
+ AclPermissionUtil.createAclInfo(model.getProject(),
semanticService.getCurrentUserGroups()));
ccDesc.setInnerExpression(ccExpression);
ComputedColumnEvalUtil.evaluateExprAndType(model, ccDesc);
@@ -4870,6 +4871,23 @@ public class ModelServiceTest extends SourceTestCase {
Assert.assertNull(model.getMeasures().get(1).getComment());
}
+ @Test
+ public void testCheckFlatTableSql() {
+ String project = "default";
+ String modelId = "a8ba3ff1-83bd-4066-ad54-d2fb3d1f0e94";
+ NDataModel model = NDataModelManager.getInstance(getTestConfig(),
project).getDataModelDesc(modelId);
+ modelService.checkFlatTableSql(model);
+
+ getTestConfig().setProperty("kylin.env", "PROD");
+ try {
+ modelService.checkFlatTableSql(model);
+ Assert.fail();
+ } catch (KylinException e) {
+
Assert.assertEquals(FAILED_EXECUTE_MODEL_SQL.toErrorCode().getCodeString(),
+ e.getErrorCode().getCodeString());
+ }
+ }
+
@Test
public void testUpdateModelWithDirtyMeasures() {
String project = "gc_test";
diff --git
a/src/query-service/src/test/java/org/apache/kylin/rest/service/ModelServiceQueryTest.java
b/src/query-service/src/test/java/org/apache/kylin/rest/service/ModelServiceQueryTest.java
index c112280a1b..b28c4e96b6 100644
---
a/src/query-service/src/test/java/org/apache/kylin/rest/service/ModelServiceQueryTest.java
+++
b/src/query-service/src/test/java/org/apache/kylin/rest/service/ModelServiceQueryTest.java
@@ -115,7 +115,7 @@ public class ModelServiceQueryTest extends SourceTestCase {
ReflectionTestUtils.setField(semanticService, "expandableMeasureUtil",
new ExpandableMeasureUtil((model, ccDesc) -> {
String ccExpression =
QueryUtil.massageComputedColumn(model, model.getProject(), ccDesc,
-
AclPermissionUtil.prepareQueryContextACLInfo(model.getProject(),
+ AclPermissionUtil.createAclInfo(model.getProject(),
semanticService.getCurrentUserGroups()));
ccDesc.setInnerExpression(ccExpression);
ComputedColumnEvalUtil.evaluateExprAndType(model, ccDesc);
diff --git
a/src/second-storage/core-ui/src/test/java/org/apache/kylin/rest/service/ModelServiceWithSecondStorageTest.java
b/src/second-storage/core-ui/src/test/java/org/apache/kylin/rest/service/ModelServiceWithSecondStorageTest.java
index 3df761f6c2..787b16f4c6 100644
---
a/src/second-storage/core-ui/src/test/java/org/apache/kylin/rest/service/ModelServiceWithSecondStorageTest.java
+++
b/src/second-storage/core-ui/src/test/java/org/apache/kylin/rest/service/ModelServiceWithSecondStorageTest.java
@@ -142,7 +142,7 @@ public class ModelServiceWithSecondStorageTest extends
NLocalFileMetadataTestCas
ReflectionTestUtils.setField(semanticService, "expandableMeasureUtil",
new ExpandableMeasureUtil((model, ccDesc) -> {
String ccExpression =
QueryUtil.massageComputedColumn(model, model.getProject(), ccDesc,
-
AclPermissionUtil.prepareQueryContextACLInfo(model.getProject(),
+ AclPermissionUtil.createAclInfo(model.getProject(),
semanticService.getCurrentUserGroups()));
ccDesc.setInnerExpression(ccExpression);
ComputedColumnEvalUtil.evaluateExprAndType(model, ccDesc);
diff --git
a/src/systools/src/main/java/org/apache/kylin/rest/util/AclPermissionUtil.java
b/src/systools/src/main/java/org/apache/kylin/rest/util/AclPermissionUtil.java
index 36b5b3723a..6a70456aba 100644
---
a/src/systools/src/main/java/org/apache/kylin/rest/util/AclPermissionUtil.java
+++
b/src/systools/src/main/java/org/apache/kylin/rest/util/AclPermissionUtil.java
@@ -33,6 +33,7 @@ import org.apache.kylin.common.QueryContext;
import org.apache.kylin.common.exception.KylinException;
import org.apache.kylin.common.msg.MsgPicker;
import org.apache.kylin.common.persistence.AclEntity;
+import org.apache.kylin.metadata.project.NProjectManager;
import org.apache.kylin.metadata.project.ProjectInstance;
import org.apache.kylin.rest.constant.Constant;
import org.apache.kylin.rest.security.AclEntityFactory;
@@ -43,7 +44,6 @@ import org.apache.kylin.rest.security.AclPermissionFactory;
import org.apache.kylin.rest.security.CompositeAclPermission;
import org.apache.kylin.rest.security.MutableAclRecord;
import org.apache.kylin.rest.security.ObjectIdentityImpl;
-import org.apache.kylin.metadata.project.NProjectManager;
import org.springframework.security.acls.domain.BasePermission;
import org.springframework.security.acls.domain.GrantedAuthoritySid;
import org.springframework.security.acls.domain.PrincipalSid;
@@ -231,7 +231,7 @@ public class AclPermissionUtil {
}
}
- public static QueryContext.AclInfo prepareQueryContextACLInfo(String
project, Set<String> groups) {
+ public static QueryContext.AclInfo createAclInfo(String project,
Set<String> groups) {
return new QueryContext.AclInfo(getCurrentUsername(), groups,
isAdminInProject(project, groups));
}
