From f6dc6ea13bad6112cf633ff93cff06551ec801ab Mon Sep 17 00:00:00 2001 From: zy-kkk Date: Mon, 15 Jan 2024 11:36:18 +0800 Subject: [PATCH] [improvement](catalog) Escape characters for columns in recovery predicate pushdown in SQL (#29854) In the previous logic, when we restored the Column in the predicate pushdown based on the logical syntax tree for JdbcScanNode, in order to avoid query errors caused by keywords such as `key`, we added escape characters for it, but before we only Binary predicates are processed, which is imperfect. We should add escape characters to all columns that appear in the predicate to avoid errors with keywords or illegal characters. --- .../org/apache/doris/analysis/CastExpr.java | 2 +- .../java/org/apache/doris/analysis/Expr.java | 23 +++++++---- .../org/apache/doris/analysis/SlotRef.java | 31 ++++++++++++--- .../apache/doris/planner/MysqlScanNode.java | 3 +- .../jdbc/JdbcFunctionPushDownRule.java | 4 +- .../planner/external/jdbc/JdbcScanNode.java | 39 ++++++------------- .../planner/external/odbc/OdbcScanNode.java | 2 +- .../jdbc/test_mysql_jdbc_catalog.out | 19 ++++++++- .../jdbc/test_clickhouse_jdbc_catalog.groovy | 4 +- .../jdbc/test_mysql_jdbc_catalog.groovy | 13 +++++-- 10 files changed, 88 insertions(+), 52 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/CastExpr.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/CastExpr.java index 68a6da391a..2a96609c27 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/CastExpr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/CastExpr.java @@ -204,7 +204,7 @@ public class CastExpr extends Expr { @Override public String toSqlImpl() { - if (needToMysql) { + if (needExternalSql) { return getChild(0).toSql(); } if (isAnalyzed) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java index c087107e5e..869ace0ef1 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java @@ -33,6 +33,8 @@ import org.apache.doris.catalog.MaterializedIndexMeta; import org.apache.doris.catalog.PrimitiveType; import org.apache.doris.catalog.ScalarFunction; import org.apache.doris.catalog.ScalarType; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.catalog.TableIf.TableType; import org.apache.doris.catalog.Type; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; @@ -88,7 +90,9 @@ public abstract class Expr extends TreeNode implements ParseNode, Cloneabl public static final String DEFAULT_EXPR_NAME = "expr"; protected boolean disableTableName = false; - protected boolean needToMysql = false; + protected boolean needExternalSql = false; + protected TableType tableType = null; + protected TableIf inputTable = null; // to be used where we can't come up with a better estimate public static final double DEFAULT_SELECTIVITY = 0.1; @@ -973,10 +977,13 @@ public abstract class Expr extends TreeNode implements ParseNode, Cloneabl } } - public void setNeedToMysql(boolean value) { - needToMysql = value; + public void setExternalContext(boolean needExternalSql, TableType tableType, TableIf inputTable) { + this.needExternalSql = needExternalSql; + this.tableType = tableType; + this.inputTable = inputTable; + for (Expr child : children) { - child.setNeedToMysql(value); + child.setExternalContext(needExternalSql, tableType, inputTable); } } @@ -1006,10 +1013,10 @@ public abstract class Expr extends TreeNode implements ParseNode, Cloneabl return toSqlImpl(); } - public String toMySql() { - setNeedToMysql(true); - String result = toSql(); - setNeedToMysql(false); + public String toExternalSql(TableType tableType, TableIf table) { + setExternalContext(true, tableType, table); + String result = toSql(); + setExternalContext(false, null, null); return result; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SlotRef.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SlotRef.java index a1175a5bff..c27eebeb66 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SlotRef.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SlotRef.java @@ -21,8 +21,11 @@ package org.apache.doris.analysis; import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.JdbcTable; import org.apache.doris.catalog.MaterializedIndexMeta; +import org.apache.doris.catalog.OdbcTable; import org.apache.doris.catalog.TableIf; +import org.apache.doris.catalog.TableIf.TableType; import org.apache.doris.catalog.Type; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.io.Text; @@ -246,12 +249,8 @@ public class SlotRef extends Expr { @Override public String toSqlImpl() { - if (needToMysql) { - if (col != null) { - return col; - } else { - return ""; - } + if (needExternalSql) { + return toExternalSqlImpl(); } if (disableTableName && label != null) { @@ -296,6 +295,26 @@ public class SlotRef extends Expr { } } + private String toExternalSqlImpl() { + if (col != null) { + if (tableType.equals(TableType.JDBC_EXTERNAL_TABLE) || tableType.equals(TableType.JDBC) || tableType + .equals(TableType.ODBC)) { + if (inputTable instanceof JdbcTable) { + return ((JdbcTable) inputTable).getProperRealColumnName( + ((JdbcTable) inputTable).getJdbcTableType(), col); + } else if (inputTable instanceof OdbcTable) { + return JdbcTable.databaseProperName(((OdbcTable) inputTable).getOdbcTableType(), col); + } else { + return col; + } + } else { + return col; + } + } else { + return ""; + } + } + public TableName getTableName() { if (tblName == null) { Preconditions.checkState(isAnalyzed); diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/MysqlScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/planner/MysqlScanNode.java index 2f3e49b8d9..98d09d4e6e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/MysqlScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/MysqlScanNode.java @@ -25,6 +25,7 @@ import org.apache.doris.analysis.SlotRef; import org.apache.doris.analysis.TupleDescriptor; import org.apache.doris.catalog.Column; import org.apache.doris.catalog.MysqlTable; +import org.apache.doris.catalog.TableIf.TableType; import org.apache.doris.common.UserException; import org.apache.doris.planner.external.ExternalScanNode; import org.apache.doris.statistics.StatisticalType; @@ -144,7 +145,7 @@ public class MysqlScanNode extends ExternalScanNode { } ArrayList mysqlConjuncts = Expr.cloneList(conjuncts, sMap); for (Expr p : mysqlConjuncts) { - filters.add(p.toMySql()); + filters.add(p.toExternalSql(TableType.MYSQL, null)); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcFunctionPushDownRule.java b/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcFunctionPushDownRule.java index 35d069c12c..27fd693ca4 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcFunctionPushDownRule.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcFunctionPushDownRule.java @@ -20,6 +20,7 @@ package org.apache.doris.planner.external.jdbc; import org.apache.doris.analysis.Expr; import org.apache.doris.analysis.FunctionCallExpr; import org.apache.doris.analysis.FunctionName; +import org.apache.doris.catalog.TableIf.TableType; import org.apache.doris.thrift.TOdbcTableType; import com.google.common.base.Preconditions; @@ -108,7 +109,8 @@ public class JdbcFunctionPushDownRule { Preconditions.checkArgument(!func.isEmpty(), "function can not be empty"); if (checkFunction.test(func)) { - String errMsg = "Unsupported function: " + func + " in expr: " + expr.toMySql() + String errMsg = "Unsupported function: " + func + " in expr: " + expr.toExternalSql( + TableType.JDBC_EXTERNAL_TABLE, null) + " in JDBC Table Type: " + tableType; LOG.warn(errMsg); errors.add(errMsg); diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcScanNode.java index 1ebff21a23..76dbd40aba 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/external/jdbc/JdbcScanNode.java @@ -33,6 +33,8 @@ import org.apache.doris.analysis.TupleDescriptor; import org.apache.doris.catalog.Column; import org.apache.doris.catalog.Env; import org.apache.doris.catalog.JdbcTable; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.catalog.TableIf.TableType; import org.apache.doris.catalog.external.JdbcExternalTable; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; @@ -325,7 +327,7 @@ public class JdbcScanNode extends ExternalScanNode { return !fnExprList.isEmpty(); } - public static String conjunctExprToString(TOdbcTableType tableType, Expr expr, JdbcTable tbl) { + public static String conjunctExprToString(TOdbcTableType tableType, Expr expr, TableIf tbl) { if (expr instanceof CompoundPredicate) { StringBuilder result = new StringBuilder(); CompoundPredicate compoundPredicate = (CompoundPredicate) expr; @@ -358,54 +360,37 @@ public class JdbcScanNode extends ExternalScanNode { if (expr.contains(DateLiteral.class) && expr instanceof BinaryPredicate) { ArrayList children = expr.getChildren(); - String filter = children.get(0).toMySql(); + String filter = children.get(0).toExternalSql(TableType.JDBC_EXTERNAL_TABLE, tbl); filter += " " + ((BinaryPredicate) expr).getOp().toString() + " "; if (tableType.equals(TOdbcTableType.ORACLE)) { - filter += handleOracleDateFormat(children.get(1)); + filter += handleOracleDateFormat(children.get(1), tbl); } else if (tableType.equals(TOdbcTableType.TRINO) || tableType.equals(TOdbcTableType.PRESTO)) { - filter += handleTrinoDateFormat(children.get(1)); + filter += handleTrinoDateFormat(children.get(1), tbl); } else { - filter += children.get(1).toMySql(); + filter += children.get(1).toExternalSql(TableType.JDBC_EXTERNAL_TABLE, tbl); } return filter; } - if (expr.contains(SlotRef.class) && expr instanceof BinaryPredicate) { - ArrayList children = expr.getChildren(); - String filter; - if (children.get(0) instanceof SlotRef) { - if (tbl != null) { - filter = tbl.getProperRealColumnName(tableType, children.get(0).toMySql()); - } else { - filter = JdbcTable.databaseProperName(tableType, children.get(0).toMySql()); - } - } else { - filter = children.get(0).toMySql(); - } - filter += " " + ((BinaryPredicate) expr).getOp().toString() + " "; - filter += children.get(1).toMySql(); - return filter; - } - // only for old planner if (expr.contains(BoolLiteral.class) && "1".equals(expr.getStringValue()) && expr.getChildren().isEmpty()) { return "1 = 1"; } - return expr.toMySql(); + return expr.toExternalSql(TableType.JDBC_EXTERNAL_TABLE, tbl); } - private static String handleOracleDateFormat(Expr expr) { + private static String handleOracleDateFormat(Expr expr, TableIf tbl) { if (expr.isConstant() && (expr.getType().isDatetime() || expr.getType().isDatetimeV2())) { return "to_date('" + expr.getStringValue() + "', 'yyyy-mm-dd hh24:mi:ss')"; } - return expr.toMySql(); + return expr.toExternalSql(TableType.JDBC_EXTERNAL_TABLE, tbl); } - private static String handleTrinoDateFormat(Expr expr) { + private static String handleTrinoDateFormat(Expr expr, TableIf tbl) { if (expr.isConstant()) { if (expr.getType().isDate() || expr.getType().isDateV2()) { return "date '" + expr.getStringValue() + "'"; @@ -413,6 +398,6 @@ public class JdbcScanNode extends ExternalScanNode { return "timestamp '" + expr.getStringValue() + "'"; } } - return expr.toMySql(); + return expr.toExternalSql(TableType.JDBC_EXTERNAL_TABLE, tbl); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/external/odbc/OdbcScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/planner/external/odbc/OdbcScanNode.java index d827ec4a4e..f3ba2a5add 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/external/odbc/OdbcScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/external/odbc/OdbcScanNode.java @@ -216,7 +216,7 @@ public class OdbcScanNode extends ExternalScanNode { ArrayList odbcConjuncts = Expr.cloneList(conjuncts, sMap); for (Expr p : odbcConjuncts) { if (shouldPushDownConjunct(odbcType, p)) { - String filter = JdbcScanNode.conjunctExprToString(odbcType, p, null); + String filter = JdbcScanNode.conjunctExprToString(odbcType, p, tbl); filters.add(filter); conjuncts.remove(p); } diff --git a/regression-test/data/external_table_p0/jdbc/test_mysql_jdbc_catalog.out b/regression-test/data/external_table_p0/jdbc/test_mysql_jdbc_catalog.out index 70957b94e2..4a7c9a9035 100644 --- a/regression-test/data/external_table_p0/jdbc/test_mysql_jdbc_catalog.out +++ b/regression-test/data/external_table_p0/jdbc/test_mysql_jdbc_catalog.out @@ -165,9 +165,26 @@ bca 2022-11-02 2022-11-02 8012 vivo 1.12345 1.12345 1.12345 1.12345 1.12345 1.12345 123456789012345678901234567890123.12345 12345678901234567890123456789012.12345 1234567890123456789012345678901234.12345 123456789012345678901234567890123.12345 123456789012345678901234567890123456789012345678901234567890.12345 123456789012345678901234567890123456789012345678901234567890.12345 --- !ex_tb21 -- +-- !ex_tb21_1 -- 2 2 +-- !ex_tb21_2 -- +2 2 + +-- !ex_tb21_3 -- +1 1 +2 2 + +-- !ex_tb21_4 -- +2 2 + +-- !ex_tb21_5 -- +1 1 +2 2 + +-- !ex_tb21_6 -- +1 1 + -- !information_schema -- character_sets collations diff --git a/regression-test/suites/external_table_p0/jdbc/test_clickhouse_jdbc_catalog.groovy b/regression-test/suites/external_table_p0/jdbc/test_clickhouse_jdbc_catalog.groovy index 56c9d097cd..9948c49d24 100644 --- a/regression-test/suites/external_table_p0/jdbc/test_clickhouse_jdbc_catalog.groovy +++ b/regression-test/suites/external_table_p0/jdbc/test_clickhouse_jdbc_catalog.groovy @@ -87,7 +87,7 @@ suite("test_clickhouse_jdbc_catalog", "p0,external,clickhouse,external_docker,ex order_qt_func_push """select * from ts where from_unixtime(ts,'yyyyMMdd') >= '2022-01-01';""" explain { sql("select * from ts where from_unixtime(ts,'yyyyMMdd') >= '2022-01-01';") - contains """QUERY: SELECT "id", "ts" FROM "doris_test"."ts" WHERE (FROM_UNIXTIME(ts, '%Y%m%d') >= '2022-01-01')""" + contains """QUERY: SELECT "id", "ts" FROM "doris_test"."ts" WHERE (FROM_UNIXTIME("ts", '%Y%m%d') >= '2022-01-01')""" } explain { sql("select * from ts where nvl(ts,null) >= '2022-01-01';") @@ -96,7 +96,7 @@ suite("test_clickhouse_jdbc_catalog", "p0,external,clickhouse,external_docker,ex order_qt_func_push2 """select * from ts where ts <= unix_timestamp(from_unixtime(ts,'yyyyMMdd'));""" explain { sql("select * from ts where ts <= unix_timestamp(from_unixtime(ts,'yyyy-MM-dd'));") - contains """QUERY: SELECT "id", "ts" FROM "doris_test"."ts" WHERE (ts <= toUnixTimestamp(FROM_UNIXTIME(ts, '%Y-%m-%d')))""" + contains """QUERY: SELECT "id", "ts" FROM "doris_test"."ts" WHERE ("ts" <= toUnixTimestamp(FROM_UNIXTIME("ts", '%Y-%m-%d')))""" } order_qt_dt_with_tz """ select * from dt_with_tz order by id; """ diff --git a/regression-test/suites/external_table_p0/jdbc/test_mysql_jdbc_catalog.groovy b/regression-test/suites/external_table_p0/jdbc/test_mysql_jdbc_catalog.groovy index 2b343a8e01..ec09d2a319 100644 --- a/regression-test/suites/external_table_p0/jdbc/test_mysql_jdbc_catalog.groovy +++ b/regression-test/suites/external_table_p0/jdbc/test_mysql_jdbc_catalog.groovy @@ -166,7 +166,12 @@ suite("test_mysql_jdbc_catalog", "p0,external,mysql,external_docker,external_doc order_qt_ex_tb18 """ select * from ${ex_tb18} order by num_tinyint; """ order_qt_ex_tb19 """ select * from ${ex_tb19} order by date_value; """ order_qt_ex_tb20 """ select * from ${ex_tb20} order by decimal_normal; """ - order_qt_ex_tb21 """ select `key`, `id` from ${ex_tb21} where `key` = 2 order by id;""" + order_qt_ex_tb21_1 """ select `key`, `id` from ${ex_tb21} where `key` = 2 order by id;""" + order_qt_ex_tb21_2 """ select `key`, `id` from ${ex_tb21} where `key` like 2 order by id;""" + order_qt_ex_tb21_3 """ select `key`, `id` from ${ex_tb21} where `key` in (1,2) order by id;""" + order_qt_ex_tb21_4 """ select `key`, `id` from ${ex_tb21} where abs(`key`) = 2 order by id;""" + order_qt_ex_tb21_5 """ select `key`, `id` from ${ex_tb21} where `key` between 1 and 2 order by id;""" + order_qt_ex_tb21_6 """ select `key`, `id` from ${ex_tb21} where `key` = case when id = 1 then 1 else 0 end order by id;""" order_qt_information_schema """ show tables from information_schema; """ order_qt_auto_default_t """insert into ${auto_default_t}(name) values('a'); """ order_qt_dt """select * from ${dt}; """ @@ -386,17 +391,17 @@ suite("test_mysql_jdbc_catalog", "p0,external,mysql,external_docker,external_doc explain { sql ("SELECT timestamp0 from dt where DATE_TRUNC(date_sub(timestamp0,INTERVAL 9 HOUR),'hour') > '2011-03-03 17:39:05' and timestamp0 > '2022-01-01';") - contains "QUERY: SELECT `timestamp0` FROM `doris_test`.`dt` WHERE (timestamp0 > '2022-01-01 00:00:00')" + contains "QUERY: SELECT `timestamp0` FROM `doris_test`.`dt` WHERE (`timestamp0` > '2022-01-01 00:00:00')" } explain { sql ("select k6, k8 from test1 where nvl(k6, null) = 1;") - contains "QUERY: SELECT `k6`, `k8` FROM `doris_test`.`test1` WHERE (ifnull(k6, NULL) = 1)" + contains "QUERY: SELECT `k6`, `k8` FROM `doris_test`.`test1` WHERE (ifnull(`k6`, NULL) = 1)" } explain { sql ("select k6, k8 from test1 where nvl(nvl(k6, null),null) = 1;") - contains "QUERY: SELECT `k6`, `k8` FROM `doris_test`.`test1` WHERE (ifnull(ifnull(k6, NULL), NULL) = 1)" + contains "QUERY: SELECT `k6`, `k8` FROM `doris_test`.`test1` WHERE (ifnull(ifnull(`k6`, NULL), NULL) = 1)" } sql """ admin set frontend config ("enable_func_pushdown" = "false"); """ explain {