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 cd08e98868fe9b60a27a6550322f30d655b290f2
Author: fengguangyuan <qq272101...@gmail.com>
AuthorDate: Thu May 25 17:44:13 2023 +0800
KYLIN-5695 Refine code to avoid potential issues on formatting model filter
condition
---------
Co-authored-by: Guangyuan Feng <guangyuan.f...@kyligence.io>
---
.../kylin/common/util/AddTableNameSqlVisitor.java | 85 ----------------
.../common/util/SqlIdentifierFormatterVisitor.java | 107 +++++++++++++++++++++
.../apache/kylin/rest/service/ModelService.java | 24 +----
.../org/apache/kylin/common/util/VisitorsTest.java | 93 ++++++++++++++++++
.../kylin/rest/service/ModelServiceTest.java | 14 ++-
5 files changed, 212 insertions(+), 111 deletions(-)
diff --git
a/src/modeling-service/src/main/java/org/apache/kylin/common/util/AddTableNameSqlVisitor.java
b/src/modeling-service/src/main/java/org/apache/kylin/common/util/AddTableNameSqlVisitor.java
deleted file mode 100644
index 775f391fb4..0000000000
---
a/src/modeling-service/src/main/java/org/apache/kylin/common/util/AddTableNameSqlVisitor.java
+++ /dev/null
@@ -1,85 +0,0 @@
-/*
- * 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.kylin.common.util;
-
-import java.util.Locale;
-import java.util.Map;
-import java.util.Set;
-
-import org.apache.calcite.sql.SqlAggFunction;
-import org.apache.calcite.sql.SqlAsOperator;
-import org.apache.calcite.sql.SqlBasicCall;
-import org.apache.calcite.sql.SqlCall;
-import org.apache.calcite.sql.SqlIdentifier;
-import org.apache.calcite.sql.util.SqlBasicVisitor;
-
-import com.google.common.collect.ImmutableList;
-
-public class AddTableNameSqlVisitor extends SqlBasicVisitor<Object> {
- private static final String DEFAULT_REASON = "Something went wrong. %s";
- private String expr;
- private Map<String, String> colToTable;
- private Set<String> ambiguityCol;
- private Set<String> allColumn;
-
- public AddTableNameSqlVisitor(String expr, Map<String, String> colToTable,
Set<String> ambiguityCol,
- Set<String> allColumn) {
- this.expr = expr;
- this.colToTable = colToTable;
- this.ambiguityCol = ambiguityCol;
- this.allColumn = allColumn;
- }
-
- @Override
- public Object visit(SqlIdentifier id) {
- boolean ok = true;
- if (id.names.size() == 1) {
- String column = id.names.get(0).toUpperCase(Locale.ROOT).trim();
- if (!colToTable.containsKey(column) ||
ambiguityCol.contains(column)) {
- ok = false;
- } else {
- id.names = ImmutableList.of(colToTable.get(column), column);
- }
- } else if (id.names.size() == 2) {
- String table = id.names.get(0).toUpperCase(Locale.ROOT).trim();
- String column = id.names.get(1).toUpperCase(Locale.ROOT).trim();
- ok = allColumn.contains(table + "." + column);
- } else {
- ok = false;
- }
- if (!ok) {
- throw new IllegalArgumentException(
- "Unrecognized column: " + id.toString() + " in expression
'" + expr + "'.");
- }
- return null;
- }
-
- @Override
- public Object visit(SqlCall call) {
- if (call instanceof SqlBasicCall) {
- if (call.getOperator() instanceof SqlAsOperator) {
- throw new IllegalArgumentException(String.format(Locale.ROOT,
DEFAULT_REASON, "null"));
- }
-
- if (call.getOperator() instanceof SqlAggFunction) {
- throw new IllegalArgumentException(String.format(Locale.ROOT,
DEFAULT_REASON, "null"));
- }
- }
- return call.getOperator().acceptCall(this, call);
- }
-}
diff --git
a/src/modeling-service/src/main/java/org/apache/kylin/common/util/SqlIdentifierFormatterVisitor.java
b/src/modeling-service/src/main/java/org/apache/kylin/common/util/SqlIdentifierFormatterVisitor.java
new file mode 100644
index 0000000000..098810dc3a
--- /dev/null
+++
b/src/modeling-service/src/main/java/org/apache/kylin/common/util/SqlIdentifierFormatterVisitor.java
@@ -0,0 +1,107 @@
+/*
+ * 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.kylin.common.util;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlBasicCall;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.util.SqlBasicVisitor;
+import org.apache.kylin.common.exception.KylinException;
+import org.apache.kylin.common.exception.ServerErrorCode;
+import org.apache.kylin.guava30.shaded.common.collect.Maps;
+import org.apache.kylin.guava30.shaded.common.collect.Sets;
+import org.apache.kylin.metadata.model.NDataModel;
+
+import com.google.common.collect.ImmutableList;
+
+import lombok.val;
+
+public class SqlIdentifierFormatterVisitor extends SqlBasicVisitor<Void> {
+ private final String expr;
+ private final Map<String, Set<String>> table2cols = Maps.newHashMap();
+ private final Map<String, Set<String>> col2tables = Maps.newHashMap();
+
+ public SqlIdentifierFormatterVisitor(String expr,
List<NDataModel.NamedColumn> fullQualifiedNamedColumns) {
+ this.expr = expr;
+ for (val col : fullQualifiedNamedColumns) {
+ if (col.getStatus() == NDataModel.ColumnStatus.TOMB) {
+ continue;
+ }
+ String aliasDotColumn = col.getAliasDotColumn();
+ String[] nameParts = aliasDotColumn.split("\\.");
+ if (nameParts.length < 2) {
+ throw new KylinException(ServerErrorCode.INVALID_MODEL_TYPE,
+ "Found invalid stored full qualified column name for "
+ nameParts[nameParts.length - 1]);
+ }
+ String table = nameParts[nameParts.length - 2];
+ table2cols.putIfAbsent(table, Sets.newHashSet());
+ String column = nameParts[nameParts.length - 1];
+ Set<String> cols = table2cols.get(table);
+ if (cols.contains(column)) {
+ throw new
KylinException(ServerErrorCode.DUPLICATED_COLUMN_NAME,
+ String.format("Found duplicate stored column %s for
table %s!", column, table));
+ }
+ cols.add(column);
+ col2tables.putIfAbsent(column, Sets.newHashSet());
+ col2tables.get(column).add(table);
+ }
+ }
+
+ @Override
+ public Void visit(SqlIdentifier id) {
+ if (id.names.size() == 1) {
+ String column = id.names.get(0).toUpperCase(Locale.ROOT).trim();
+ Set<String> targetTbls = col2tables.getOrDefault(column,
Sets.newHashSet());
+ if (targetTbls.size() != 1) {
+ throw new KylinException(ServerErrorCode.COLUMN_NOT_EXIST,
+ String.format(
+ "Found unrecognized or ambiguous column: %s in
candidate tables [%s] in expression '%s'.",
+ id, targetTbls.stream().reduce(", ", String::join),
expr));
+ }
+ id.names = ImmutableList.of(targetTbls.iterator().next(), column);
+ } else if (id.names.size() >= 2) {
+ String table = id.names.get(0).toUpperCase(Locale.ROOT).trim();
+ String column = id.names.get(1).toUpperCase(Locale.ROOT).trim();
+ Set<String> cols = table2cols.get(table);
+ // TODO: Support 3 or more name parts, like: database.table.name?
+ if (id.names.size() > 2 || cols == null || !cols.contains(column))
{
+ throw new KylinException(ServerErrorCode.COLUMN_NOT_EXIST,
+ String.format("Found unrecognized column: %s in
expression '%s'.", id, expr));
+ }
+ id.names = ImmutableList.of(table, column);
+ }
+ return null;
+ }
+
+ @Override
+ public Void visit(SqlCall call) {
+ if (call instanceof SqlBasicCall
+ && (call.getOperator() instanceof SqlAsOperator ||
call.getOperator() instanceof SqlAggFunction)) {
+ throw new KylinException(ServerErrorCode.INVALID_PARAMETER,
+ String.format("Unsupported SqlNode %s in expression
%s", call, expr));
+ }
+ return super.visit(call);
+ }
+}
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 9f46c116be..b4b1721dee 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
@@ -124,7 +124,7 @@ import
org.apache.kylin.common.persistence.transaction.TransactionException;
import org.apache.kylin.common.persistence.transaction.UnitOfWork;
import org.apache.kylin.common.persistence.transaction.UnitOfWorkContext;
import org.apache.kylin.common.scheduler.EventBusFactory;
-import org.apache.kylin.common.util.AddTableNameSqlVisitor;
+import org.apache.kylin.common.util.SqlIdentifierFormatterVisitor;
import org.apache.kylin.common.util.DateFormat;
import org.apache.kylin.common.util.JsonUtil;
import org.apache.kylin.common.util.Pair;
@@ -3659,28 +3659,8 @@ public class ModelService extends AbstractModelService
implements TableModelSupp
@VisibleForTesting
String addTableNameIfNotExist(final String expr, final NDataModel model) {
- Map<String, String> colToTable = Maps.newHashMap();
- Set<String> ambiguityCol = Sets.newHashSet();
- Set<String> allColumn = Sets.newHashSet();
- for (val col : model.getAllNamedColumns()) {
- if (col.getStatus() == NDataModel.ColumnStatus.TOMB) {
- continue;
- }
- String aliasDotColumn = col.getAliasDotColumn();
- allColumn.add(aliasDotColumn);
- String table = aliasDotColumn.split("\\.")[0];
- String column = aliasDotColumn.split("\\.")[1];
- if (colToTable.containsKey(column)) {
- ambiguityCol.add(column);
- } else {
- colToTable.put(column, table);
- }
- }
-
SqlNode sqlNode = CalciteParser.getExpNode(expr);
-
- SqlVisitor<Object> sqlVisitor = new AddTableNameSqlVisitor(expr,
colToTable, ambiguityCol, allColumn);
-
+ SqlVisitor<Void> sqlVisitor = new SqlIdentifierFormatterVisitor(expr,
model.getAllNamedColumns());
sqlNode.accept(sqlVisitor);
if
(!NProjectManager.getProjectConfig(model.getProject()).isBuildExcludedTableEnabled())
{
diff --git
a/src/modeling-service/src/test/java/org/apache/kylin/common/util/VisitorsTest.java
b/src/modeling-service/src/test/java/org/apache/kylin/common/util/VisitorsTest.java
new file mode 100644
index 0000000000..110dc8dc3b
--- /dev/null
+++
b/src/modeling-service/src/test/java/org/apache/kylin/common/util/VisitorsTest.java
@@ -0,0 +1,93 @@
+/*
+ * 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.kylin.common.util;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlBasicCall;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.kylin.common.exception.KylinException;
+import org.apache.kylin.common.exception.ServerErrorCode;
+import org.apache.kylin.guava30.shaded.common.collect.ImmutableList;
+import org.apache.kylin.metadata.model.NDataModel;
+import org.junit.Test;
+
+public class VisitorsTest {
+
+ @Test
+ public void testSqlIdentifierFormatterVisitor() {
+ NDataModel.NamedColumn col1 = new NDataModel.NamedColumn();
+ col1.setAliasDotColumn("TBL.COL1");
+ NDataModel.NamedColumn col2 = new NDataModel.NamedColumn();
+ col2.setAliasDotColumn("TBL.COL1");
+ NDataModel.NamedColumn tombCol = new NDataModel.NamedColumn();
+ tombCol.setAliasDotColumn("TOMB");
+ tombCol.setStatus(NDataModel.ColumnStatus.TOMB);
+
+ SqlIdentifier validIdentifier = new
SqlIdentifier(ImmutableList.of("col1"), SqlParserPos.ZERO);
+ new SqlIdentifierFormatterVisitor("",
ImmutableList.of(col1)).visit(validIdentifier);
+ assertEquals("TBL.COL1", validIdentifier.toString());
+
+ ImmutableList<NDataModel.NamedColumn> columns = ImmutableList.of(col1,
col2, tombCol);
+ KylinException ke = assertThrows(KylinException.class, () -> new
SqlIdentifierFormatterVisitor("", columns));
+
assertEquals(ServerErrorCode.DUPLICATED_COLUMN_NAME.toErrorCode().getCodeString(),
+ ke.getErrorCode().getCodeString());
+
+ NDataModel.NamedColumn col3 = new NDataModel.NamedColumn();
+ col3.setAliasDotColumn("COL3");
+ ke = assertThrows(KylinException.class,
+ () -> new SqlIdentifierFormatterVisitor("",
ImmutableList.of(col3, tombCol)));
+
assertEquals(ServerErrorCode.INVALID_MODEL_TYPE.toErrorCode().getCodeString(),
+ ke.getErrorCode().getCodeString());
+
+ SqlIdentifierFormatterVisitor tombVisitor = new
SqlIdentifierFormatterVisitor("", ImmutableList.of(tombCol));
+ final SqlIdentifier identifier = new
SqlIdentifier(ImmutableList.of("TBL", "COL4"), SqlParserPos.ZERO);
+ ke = assertThrows(KylinException.class, () ->
tombVisitor.visit(identifier));
+
assertEquals(ServerErrorCode.COLUMN_NOT_EXIST.toErrorCode().getCodeString(),
ke.getErrorCode().getCodeString());
+ assertTrue(ke.getMessage().contains("unrecognized column"));
+
+ SqlIdentifierFormatterVisitor visitor = new
SqlIdentifierFormatterVisitor("", ImmutableList.of(col1));
+ ke = assertThrows(KylinException.class, () ->
visitor.visit(identifier));
+
assertEquals(ServerErrorCode.COLUMN_NOT_EXIST.toErrorCode().getCodeString(),
ke.getErrorCode().getCodeString());
+ assertTrue(ke.getMessage().contains("unrecognized column"));
+
+ final SqlIdentifier ambiguousIdentifier = new SqlIdentifier("COL3",
SqlParserPos.ZERO);
+ ke = assertThrows(KylinException.class, () ->
visitor.visit(ambiguousIdentifier));
+
assertEquals(ServerErrorCode.COLUMN_NOT_EXIST.toErrorCode().getCodeString(),
ke.getErrorCode().getCodeString());
+ assertTrue(ke.getMessage().contains("ambiguous column"));
+
+ final SqlBasicCall sqlBasicCall =
+ new SqlBasicCall(new SqlAsOperator(), new SqlNode[]
{identifier, identifier}, SqlParserPos.ZERO);
+ ke = assertThrows(KylinException.class, () ->
visitor.visit(sqlBasicCall));
+
assertEquals(ServerErrorCode.INVALID_PARAMETER.toErrorCode().getCodeString(),
+ ke.getErrorCode().getCodeString());
+
+ final SqlIdentifier unsupportedIdentifier =
+ new SqlIdentifier(ImmutableList.of("catalog", "tbl", "col"),
SqlParserPos.ZERO);
+ ke = assertThrows(KylinException.class, () ->
visitor.visit(unsupportedIdentifier));
+
assertEquals(ServerErrorCode.COLUMN_NOT_EXIST.toErrorCode().getCodeString(),
+ ke.getErrorCode().getCodeString());
+ }
+
+}
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 9b039b040a..a12d08674e 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
@@ -3300,7 +3300,7 @@ public class ModelServiceTest extends SourceTestCase {
modelRequest.setUuid(null);
String filterCond = "\"day\" = 0 and \"123TABLE\".\"day#\" = 1 and
\"中文列\" = 1";
- String expectedFilterCond = "(((`123TABLE`.`DAY` = 0) AND
(`123TABLE`.`day#` = 1)) AND (`123TABLE`.`中文列` = 1))";
+ String expectedFilterCond = "(((`123TABLE`.`DAY` = 0) AND
(`123TABLE`.`DAY#` = 1)) AND (`123TABLE`.`中文列` = 1))";
modelRequest.setFilterCondition(filterCond);
val newModel = modelService.createModel(modelRequest.getProject(),
modelRequest);
@@ -3546,12 +3546,18 @@ public class ModelServiceTest extends SourceTestCase {
NDataModelManager modelManager =
NDataModelManager.getInstance(KylinConfig.getInstanceFromEnv(), project);
NDataModel model = modelManager
.copyForWrite(modelManager.getDataModelDesc("89af4ee2-2cdb-4b07-b39e-4c29856309aa"));
- String originSql = "trans_id = 0 and TEST_KYLIN_FACT.order_id < 100
and DEAL_AMOUNT > 123";
+ String originSql = "`trans_id` = 0 and test_kylin_fact.TRANS_ID > 0
and `test_kylin_fact`.trans_id > 0 "
+ + "and TEST_KYLIN_FACT.order_id < 100 and DEAL_AMOUNT > 123";
model.setFilterCondition(originSql);
modelService.massageModelFilterCondition(model);
- Assert.assertEquals("(((`TEST_KYLIN_FACT`.`TRANS_ID` = 0) "
- + "AND (`TEST_KYLIN_FACT`.`ORDER_ID` < 100)) AND
((`TEST_KYLIN_FACT`.`PRICE` * `TEST_KYLIN_FACT`.`ITEM_COUNT`) > 123))",
+ Assert.assertEquals("(((((`TEST_KYLIN_FACT`.`TRANS_ID` = 0) AND
(`TEST_KYLIN_FACT`.`TRANS_ID` > 0)) "
+ + "AND (`TEST_KYLIN_FACT`.`TRANS_ID` > 0)) AND
(`TEST_KYLIN_FACT`.`ORDER_ID` < 100)) "
+ + "AND ((`TEST_KYLIN_FACT`.`PRICE` *
`TEST_KYLIN_FACT`.`ITEM_COUNT`) > 123))",
model.getFilterCondition());
+
+ originSql = "badColumn is null";
+ model.setFilterCondition(originSql);
+ Assert.assertThrows(KylinException.class, () ->
modelService.massageModelFilterCondition(model));
}
@Test
