kgyrtkirk commented on code in PR #16312: URL: https://github.com/apache/druid/pull/16312#discussion_r1579741955
########## sql/src/main/java/org/apache/druid/sql/calcite/expression/ExprEvalWrapper.java: ########## @@ -0,0 +1,54 @@ +/* + * 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.druid.sql.calcite.expression; + +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.math.expr.ExprEval; + +/** + * Wraps an {@link ExprEval} and provides an additional {@link #trueValue()} method to allow callers in + * {@link NullHandling#replaceWithDefault()} mode to determine whether the {@link ExprEval} represents a true + * null or an empty string. + */ +public class ExprEvalWrapper +{ + private final ExprEval<?> eval; + private final boolean isEmptyString; + + public ExprEvalWrapper(ExprEval<?> eval, boolean isEmptyString) + { + this.eval = eval; + this.isEmptyString = isEmptyString; + } + + public ExprEval<?> exprEval() + { + return eval; + } + + /** + * Like calling {@link ExprEval#value()} on {@link #exprEval()}, but in {@link NullHandling#replaceWithDefault()} + * mode, returns empty string rather than null if eval should truly be an empty string. + */ + public Object trueValue() Review Comment: I wonder if this means that in `replaceWithDefault` mode this may return `null` ; but only for "strings" ? I feel like the method name`trueValue` is a bit misleading - as it suggests to have some connection with logic functions. Don't have any good suggestion... `realValue` also sounds pretty bad... ########## sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java: ########## @@ -306,61 +309,79 @@ private static DruidExpression rexCallToDruidExpression( } } + /** + * Create an {@link ExprEval} from a literal {@link RexNode}. + */ @Nullable - static DruidExpression literalToDruidExpression( + public static ExprEvalWrapper literalToExprEval( final PlannerContext plannerContext, final RexNode rexNode ) { - final SqlTypeName sqlTypeName = rexNode.getType().getSqlTypeName(); + if (rexNode.isA(SqlKind.CAST)) { + if (SqlTypeFamily.DATE.contains(rexNode.getType())) { + // Cast to DATE suggests some timestamp flooring. We don't deal with that here, so return null. + return null; + } + + final ExprEvalWrapper innerEval = literalToExprEval(plannerContext, ((RexCall) rexNode).getOperands().get(0)); + final ColumnType castToColumnType = Calcites.getColumnTypeForRelDataType(rexNode.getType()); + if (castToColumnType == null) { + return null; + } + + final ExpressionType castToExprType = ExpressionType.fromColumnType(castToColumnType); + if (castToExprType == null) { + return null; + } + + return new ExprEvalWrapper( + innerEval.exprEval().castTo(castToExprType), + false + ); + } // Translate literal. - final ColumnType columnType = Calcites.getColumnTypeForRelDataType(rexNode.getType()); + final SqlTypeName sqlTypeName = rexNode.getType().getSqlTypeName(); + final ExprEval<?> retVal; + boolean isEmptyString = false; + if (RexLiteral.isNullLiteral(rexNode)) { - return DruidExpression.ofLiteral(columnType, DruidExpression.nullLiteral()); + final ColumnType columnType = Calcites.getColumnTypeForRelDataType(rexNode.getType()); + final ExpressionType expressionType = columnType == null ? null : ExpressionType.fromColumnTypeStrict(columnType); + retVal = ExprEval.ofType(expressionType, null); } else if (SqlTypeName.INT_TYPES.contains(sqlTypeName)) { final Number number = (Number) RexLiteral.value(rexNode); - return DruidExpression.ofLiteral( - columnType, - number == null ? DruidExpression.nullLiteral() : DruidExpression.longLiteral(number.longValue()) - ); + retVal = ExprEval.ofType(ExpressionType.LONG, number == null ? null : number.longValue()); } else if (SqlTypeName.NUMERIC_TYPES.contains(sqlTypeName)) { // Numeric, non-INT, means we represent it as a double. final Number number = (Number) RexLiteral.value(rexNode); - return DruidExpression.ofLiteral( - columnType, - number == null ? DruidExpression.nullLiteral() : DruidExpression.doubleLiteral(number.doubleValue()) - ); + retVal = ExprEval.ofType(ExpressionType.DOUBLE, number == null ? null : number.doubleValue()); } else if (SqlTypeFamily.INTERVAL_DAY_TIME == sqlTypeName.getFamily()) { // Calcite represents DAY-TIME intervals in milliseconds. final long milliseconds = ((Number) RexLiteral.value(rexNode)).longValue(); - return DruidExpression.ofLiteral(columnType, DruidExpression.longLiteral(milliseconds)); + retVal = ExprEval.ofType(ExpressionType.LONG, milliseconds); } else if (SqlTypeFamily.INTERVAL_YEAR_MONTH == sqlTypeName.getFamily()) { // Calcite represents YEAR-MONTH intervals in months. final long months = ((Number) RexLiteral.value(rexNode)).longValue(); - return DruidExpression.ofLiteral(columnType, DruidExpression.longLiteral(months)); + retVal = ExprEval.ofType(ExpressionType.LONG, months); } else if (SqlTypeName.STRING_TYPES.contains(sqlTypeName)) { - return DruidExpression.ofStringLiteral(RexLiteral.stringValue(rexNode)); + final String s = RexLiteral.stringValue(rexNode); + retVal = ExprEval.ofType(ExpressionType.STRING, s); + isEmptyString = s != null && s.isEmpty(); } else if (SqlTypeName.TIMESTAMP == sqlTypeName || SqlTypeName.DATE == sqlTypeName) { - if (RexLiteral.isNullLiteral(rexNode)) { - return DruidExpression.ofLiteral(columnType, DruidExpression.nullLiteral()); - } else { - return DruidExpression.ofLiteral( - columnType, - DruidExpression.longLiteral( - Calcites.calciteDateTimeLiteralToJoda(rexNode, plannerContext.getTimeZone()).getMillis() - ) - ); - } - } else if (SqlTypeName.BOOLEAN == sqlTypeName) { - return DruidExpression.ofLiteral( - columnType, - DruidExpression.longLiteral(RexLiteral.booleanValue(rexNode) ? 1 : 0) + retVal = ExprEval.ofType( + ExpressionType.LONG, + Calcites.calciteDateTimeLiteralToJoda(rexNode, plannerContext.getTimeZone()).getMillis() ); + } else if (SqlTypeName.BOOLEAN == sqlTypeName) { + retVal = ExprEval.ofType(ExpressionType.LONG, RexLiteral.booleanValue(rexNode) ? 1 : 0); } else { // Can't translate other literals. return null; } + + return new ExprEvalWrapper(retVal, isEmptyString); Review Comment: I wonder if there is a real need to repackage everything into these `ExprEvalWrapper` objects; I think because `isEmptyString` is a construction time parameter a static factory method could decide to wrap the expr or not - and possibly keep the original `Expr` if it doesn't need it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
