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