clintropolis commented on a change in pull request #9893:
URL: https://github.com/apache/druid/pull/9893#discussion_r427752016
##########
File path: docs/querying/sql.md
##########
@@ -322,17 +322,18 @@ String functions accept strings, and return a type
appropriate to the function.
|`LOWER(expr)`|Returns expr in all lowercase.|
|`PARSE_LONG(string[, radix])`|Parses a string into a long (BIGINT) with the
given radix, or 10 (decimal) if a radix is not provided.|
|`POSITION(needle IN haystack [FROM fromIndex])`|Returns the index of needle
within haystack, with indexes starting from 1. The search will begin at
fromIndex, or 1 if fromIndex is not specified. If the needle is not found,
returns 0.|
-|`REGEXP_EXTRACT(expr, pattern, [index])`|Apply regular expression pattern and
extract a capture group, or null if there is no match. If index is unspecified
or zero, returns the substring that matched the pattern.|
+|`REGEXP_EXTRACT(expr, pattern, [index])`|Apply regular expression `pattern`
to `expr` and extract a capture group, or `NULL` if there is no match. If index
is unspecified or zero, returns the substring that matched the pattern. The
pattern must match starting at the beginning of `expr`, but does not need to
match the entire string. Note: when `druid.generic.useDefaultValueForNull =
true`, it is not possible to differentiate an empty-string match from a
non-match (both will return `NULL`).|
+|`REGEXP_LIKE(expr, pattern)`|Returns whether `expr` matches regular
expression `pattern`. The pattern must match starting at the beginning of
`expr`, but does not need to match the entire string. Especially useful in
WHERE clauses.|
Review comment:
Should this mention that it is like `LIKE` but with 100% more regex?
Maybe it's obvious enough since it is in the function name. Regardless, we
should add docs to `math-expr.md` as well?
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/expression/OperatorConversions.java
##########
@@ -291,15 +294,15 @@ public OperatorBuilder operandTypes(final
SqlTypeFamily... operandTypes)
return this;
}
- public OperatorBuilder operandTypeInference(final SqlOperandTypeInference
operandTypeInference)
Review comment:
hmm, I forget why I added this function, but it appears at least now
that nothing is calling it.. It was added with #6974, specifically [to be able
to set this as
non-null](https://github.com/apache/druid/pull/6974/files#diff-251ecf0956707731dab37c9469e83a70R330).
I guess with this change that block will always be null, so the if can
probably be removed, though I wish I could remember why I added it. However,
that PR was open for over a year so .. maybe over that lifetime of fixing it up
for conflicts it's purpose was lost to time...
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/expression/OperatorConversions.java
##########
@@ -291,15 +294,15 @@ public OperatorBuilder operandTypes(final
SqlTypeFamily... operandTypes)
return this;
}
- public OperatorBuilder operandTypeInference(final SqlOperandTypeInference
operandTypeInference)
+ public OperatorBuilder requiredOperands(final int requiredOperands)
{
- this.operandTypeInference = operandTypeInference;
+ this.requiredOperands = requiredOperands;
return this;
}
- public OperatorBuilder requiredOperands(final int requiredOperands)
+ public OperatorBuilder literalOperands(final int... literalOperands)
Review comment:
this seems handy to have :metal:, but (nit) could you add javadoc
indicating that these are the positions of the operands that are required to be
literals? bonus points if you add javadocs to other methods, but also, since
nothing else has them don't feel obligated to add them even for this unless
there are other changes to make
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/expression/OperatorConversions.java
##########
@@ -430,36 +434,64 @@ public void inferOperandTypes(
/**
* Operand type checker that is used in 'simple' situations: there are a
particular number of operands, with
- * particular types, some of which may be optional or nullable.
+ * particular types, some of which may be optional or nullable, and some of
which may be required to be literals.
*/
private static class DefaultOperandTypeChecker implements
SqlOperandTypeChecker
{
private final List<SqlTypeFamily> operandTypes;
private final int requiredOperands;
private final IntSet nullableOperands;
+ private final IntSet literalOperands;
DefaultOperandTypeChecker(
final List<SqlTypeFamily> operandTypes,
final int requiredOperands,
- final IntSet nullableOperands
+ final IntSet nullableOperands,
+ @Nullable final int[] literalOperands
)
{
Preconditions.checkArgument(requiredOperands <= operandTypes.size() &&
requiredOperands >= 0);
this.operandTypes = Preconditions.checkNotNull(operandTypes,
"operandTypes");
this.requiredOperands = requiredOperands;
this.nullableOperands = Preconditions.checkNotNull(nullableOperands,
"nullableOperands");
+
+ if (literalOperands == null) {
+ this.literalOperands = IntSets.EMPTY_SET;
+ } else {
+ this.literalOperands = new IntArraySet();
+ Arrays.stream(literalOperands).forEach(this.literalOperands::add);
+ }
}
@Override
public boolean checkOperandTypes(SqlCallBinding callBinding, boolean
throwOnFailure)
{
- if (operandTypes.size() != callBinding.getOperandCount()) {
- // Just like FamilyOperandTypeChecker: assume this is an inapplicable
sub-rule of a composite rule; don't throw
- return false;
- }
-
for (int i = 0; i < callBinding.operands().size(); i++) {
final SqlNode operand = callBinding.operands().get(i);
+
+ if (literalOperands.contains(i)) {
+ // Verify that 'operand' is a literal.
+ if (!SqlUtil.isLiteral(operand)) {
+ return throwOrReturn(
+ throwOnFailure,
+ callBinding,
+ cb -> cb.getValidator()
+ .newValidationError(
+ operand,
+
Static.RESOURCE.argumentMustBeLiteral(callBinding.getOperator().getName())
+ )
+ );
+ }
+
+ if (!nullableOperands.contains(i) && SqlUtil.isNullLiteral(operand,
true)) {
Review comment:
Would it make sense for this null part of the check be consolidated with
the `else if (operandType.getSqlTypeName() == SqlTypeName.NULL)` branch by
putting the can be null check first in the if/else chain? I guess
`SqlUtil.isNullLiteral`, is slightly more expensive of a check than just
checking that the type name is null, but is also maybe more correct since
casting a null is still probably null?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
