tanclary commented on code in PR #3732:
URL: https://github.com/apache/calcite/pull/3732#discussion_r1528851053
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4127,14 +4130,14 @@ private static class LogicalNotImplementor extends
AbstractRexCallImplementor {
* <p>Handles all logarithm functions using log rules to determine the
* appropriate base (i.e. base e for LN).
*/
- private static class LogImplementor extends AbstractRexCallImplementor {
- LogImplementor() {
- super("log", NullPolicy.STRICT, true);
+ private static class LogImplementor extends MethodImplementor {
+ LogImplementor(Method method) {
+ super(method, NullPolicy.STRICT, true);
Review Comment:
shouldn't we be passing a String here? Why change the class we're extending?
##########
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. */
Review Comment:
call them `numeric1` and `numeric2` it's more consistent
##########
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 LOGMySqlSpark(number, number2)} function applied to double
values. */
Review Comment:
Just call it LOG and mention it's for those libraries. Your comment it makes
it seem like `LOGMySqlSpark` is a function.
##########
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:
It still doesn't look like you addressed the comment though so would you
mind doing that? Thanks
##########
site/_docs/reference.md:
##########
@@ -2785,6 +2785,7 @@ In the following:
| b f s | LENGTH(string) | Equivalent to
`CHAR_LENGTH(string)`
| h s | LEVENSHTEIN(string1, string2) | Returns the Levenshtein
distance between *string1* and *string2*
| b | LOG(numeric1 [, numeric2 ]) | Returns the logarithm
of *numeric1* to base *numeric2*, or base e if *numeric2* is not present
+| m s | LOG(numeric1 [, numeric2 ]) | Returns the logarithm
of *numeric1* to base *numeric2*, or base e if *numeric2* is not present
Review Comment:
This description is the same as the one above. If it's different from the BQ
function, the description should explain how.
##########
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:
Please write it as "MySQL", that's how they style 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]
