New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CALCITE-6325] Add LOG function (enabled in Mysql and Spark library) #3789
base: main
Are you sure you want to change the base?
Conversation
@@ -512,8 +512,7 @@ public enum BuiltInMethod { | |||
SAFE_DIVIDE(SqlFunctions.class, "safeDivide", double.class, double.class), | |||
SAFE_MULTIPLY(SqlFunctions.class, "safeMultiply", double.class, double.class), | |||
SAFE_SUBTRACT(SqlFunctions.class, "safeSubtract", double.class, double.class), | |||
LOG(SqlFunctions.class, "log", long.class, long.class), | |||
LOG2(SqlFunctions.class, "log2", long.class), | |||
LOG(SqlFunctions.class, "log", long.class, long.class, int.class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this requires an enum instead of a numeric flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tanclary I'm sorry that it took so long to improve it. If you have time, please review this PR.
Co-authored-by: Tanner Clary <tannerclary@google.com>
Quality Gate passedIssues Measures |
|
||
map.put(LN, new LogImplementor()); | ||
map.put(LOG, new LogImplementor()); | ||
map.put(LOG10, new LogImplementor()); | ||
|
||
map.put(LOG_MYSQLSPARK, new LogMysqlSparkImplementor()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL function name that you are registering is LOG
, which is the same as what is added on line 651. The equals
and hashCode
methods used for the key is from the class SqlOperator
and will only use the name+kind.
The result is that you overwrite the map entry from line 651. One way around this is set LOG_MYSQLSPARK
to a new anonymous inner subclass of SqlBasicFunction
. You would need to increase visibility of the SqlBasicFunction
constructor.
LOG_MYSQLSPARK = new SqlBasicFunction(...) {};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanj-bitquill Yes, you are right, so I use the name LOGMYSQLSPARK. The method you mentioned is something I didn't expect. Let me try it.
Thank you for your reply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue isn't with the static variable name, it is with the value of name in the SqlBasicFunction
object. Both LOG
and LOG_MYSQLSPARK
use the same value for the name (and for kind).
If you debug at this point, you can see if it is replacing or adding a new entry.
https://issues.apache.org/jira/browse/CALCITE-6325