tanclary commented on code in PR #3732:
URL: https://github.com/apache/calcite/pull/3732#discussion_r1523999817
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -642,12 +643,14 @@ Builder populate() {
defineMethod(EXP, BuiltInMethod.EXP.method, NullPolicy.STRICT);
defineMethod(POWER, BuiltInMethod.POWER.method, NullPolicy.STRICT);
defineMethod(ABS, BuiltInMethod.ABS.method, NullPolicy.STRICT);
- defineMethod(LOG2, BuiltInMethod.LOG2.method, NullPolicy.STRICT);
map.put(LN, new LogImplementor());
map.put(LOG, new LogImplementor());
map.put(LOG10, new LogImplementor());
Review Comment:
Can you just create an instance of a LogMsImplementor that we can reuse? I
think that's more efficient.
Would you mind doing the same for LogImplementor above?
Thanks.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4146,9 +4149,52 @@ private static List<Expression> args(RexCall call,
if (argValueList.size() == 2) {
return list.append(argValueList.get(1));
}
+ if (argValueList.size() == 1) {
+ return list.append(Expressions.constant(Math.exp(1)));
+ }
+ // fall through
+ case "LN":
+ return list.append(Expressions.constant(Math.exp(1)));
+ case "LOG10":
+ return list.append(Expressions.constant(BigDecimal.TEN));
+ default:
+ throw new AssertionError("Operator not found: " + call.getOperator());
+ }
+ }
+ }
+
+ /** Implementor for the {@code LN}, {@code LOG}, and {@code LOG10} operators.
+ *
+ * <p>Handles all logarithm functions using log rules to determine the
+ * appropriate base (i.e. base e for LN).
+ */
+ private static class LogMSImplementor extends AbstractRexCallImplementor {
+ LogMSImplementor() {
+ super("logMS", NullPolicy.STRICT, true);
+ }
+
+ @Override Expression implementSafe(final RexToLixTranslator translator,
+ final RexCall call, final List<Expression> argValueList) {
+ return Expressions.call(BuiltInMethod.LOGMS.method, args(call,
argValueList));
+ }
+
+ private static List<Expression> args(RexCall call,
Review Comment:
this looks duplicated can you point out what's different here?
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6332,6 +6346,66 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.SPARK), consumer);
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-6325";>[CALCITE-6325]
+ * Add LOG function (enabled in MYSQL, Spark library)</a>. */
Review Comment:
nit: MySQL*, and can you make that consistent across this PR (commit
message, jira, etc.)
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4146,9 +4149,52 @@ private static List<Expression> args(RexCall call,
if (argValueList.size() == 2) {
return list.append(argValueList.get(1));
}
+ if (argValueList.size() == 1) {
+ return list.append(Expressions.constant(Math.exp(1)));
+ }
+ // fall through
+ case "LN":
+ return list.append(Expressions.constant(Math.exp(1)));
+ case "LOG10":
+ return list.append(Expressions.constant(BigDecimal.TEN));
+ default:
+ throw new AssertionError("Operator not found: " + call.getOperator());
+ }
+ }
+ }
+
+ /** Implementor for the {@code LN}, {@code LOG}, and {@code LOG10} operators.
Review Comment:
This javadoc looks identical to the one above. If the comment can be the
same there's a chance we probably don't need two different implementors.
Would you mind either: a) making the comment more clear so we know why this
implementor is needed or b) putting it all in the original LogImplementor?
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4146,9 +4149,52 @@ private static List<Expression> args(RexCall call,
if (argValueList.size() == 2) {
Review Comment:
can the function only have 1 or 2 arguments? If so, let's rewrite this as an
`if (size == 1) ... else ...`
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -2788,15 +2788,26 @@ public static double log(BigDecimal d0, BigDecimal d1) {
return Math.log(d0.doubleValue()) / Math.log(d1.doubleValue());
}
- /** SQL {@code LOG2(number)} function applied to double values. */
- public static @Nullable Double log2(double number) {
- return (number <= 0) ? null : log(number, 2);
+ /** SQL {@code LOG(number, number2)} function applied to double values. */
+ public static @Nullable Double logMS(double number, double number2) {
+ return (number <= 0) ? null : log(number, number2);
+ }
+
+ /** SQL {@code LOGMS(number, number2)} function applied to
+ * double and BigDecimal values. */
+ public static @Nullable Double logMS(double number, BigDecimal number2) {
+ return (number <= 0) ? null : log(number, number2);
+ }
+
+ /** SQL {@code LOGMS(number, number2)} function applied to
+ * BigDecimal and double values. */
+ public static @Nullable Double logMS(BigDecimal number, double number2) {
+ return (number.doubleValue() <= 0) ? null : log(number, number2);
Review Comment:
just put this conditional logic in one of the methods, and then have the
others cast to that type and use it.
example
public Double foo(int num) {
return num * 2;
}
public Double foo (double num) {
return foo((int) num);
}
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -211,6 +211,7 @@
import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOG2;
import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOGICAL_AND;
import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOGICAL_OR;
+import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOG_MS;
Review Comment:
We really should look into some other mechanism for operators implemented
differently across different dialects. I feel that just modifying the name is
confusing and not very sustainable (what if 2 dialects support 1 version and 2
support another? what do you name it then?) I know it's not in the scope of
this change but just putting it out there.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -211,6 +211,7 @@
import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOG2;
import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOGICAL_AND;
import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOGICAL_OR;
+import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOG_MS;
Review Comment:
I think we could rethink how we use the @LibraryAnnotations because it seems
like we have a straightforward way of referencing what dialects a given
operator supports.
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -2205,6 +2205,14 @@ private static RelDataType
deriveTypeMapFromEntries(SqlOperatorBinding opBinding
OperandTypes.NUMERIC_OPTIONAL_NUMERIC,
SqlFunctionCategory.NUMERIC);
+ /** The "LOG(numeric, numeric1)" function. Returns the base numeric1
logarithm of numeric. */
+ @LibraryOperator(libraries = {MYSQL, SPARK})
+ public static final SqlFunction LOG_MS =
Review Comment:
This name is confusing to me, when I see it i think `MsSQL`
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6295,6 +6295,20 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
f.checkNull("log(10, cast(null as real))");
}
+ @Test void testLogFuncByOneParameter() {
Review Comment:
can you javadoc this method? Why not add these to `testLogFunc`?
--
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]
