SQL: Add RAND() function. #8814
Conversation
|
Does it fully fix #8661? |
I believe the other functions already exist (check the docs) so yes. |
| throw new IAE("Function[%s] needs zero or one arguments", name()); | ||
| } | ||
|
|
||
| class RandExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr |
There was a problem hiding this comment.
👀 I've never seen this pattern before.
dumb guy question: Is there any advantage of using this vs a private static inner class. Do we do this just so we don't have to pass randomGenerator to the constructor of this class?
There was a problem hiding this comment.
It's sort of the same thing as an anonymous class, except it's not anonymous. I use them somewhat interchangeably. I'm not aware of any major advantage except that the class name is nicer in stack traces, debuggers, etc.
| final DoubleSupplier randomGenerator; | ||
|
|
||
| if (args.isEmpty()) { | ||
| randomGenerator = () -> ThreadLocalRandom.current().nextDouble(); |
There was a problem hiding this comment.
If you wanted to make the test in ExpressionsTest.java easier, you could inject an annotated Supplier<DoubleSupplier> into this class something like
@Inject
RandExprMacro(@ThreadRandom Supplier<DoubleSupplier> localThreadRandomSupplier) {
...
}
As I'm typing this, it feels like overkill, so feel free to ignore
|
This pull request introduces 2 alerts when merging 97a0780 into 3b602da - view on LGTM.com new alerts:
|
|
Actually, I just realized this is a bad idea, for two reasons.
These issues can be overcome, but, it doesn't seem worth it at this time. So I will close this PR. Thanks @suneet-amp for taking a look anyway! |
Fixes #8789, #8661.