xiangfu0 commented on code in PR #20001:
URL: https://github.com/apache/pinot/pull/20001#discussion_r4344441868
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/validation/ArrayToMvValidationVisitor.java:
##########
@@ -172,6 +174,42 @@ public Void visitUnnest(UnnestNode node, Boolean
isIntermediateStage) {
return null;
}
+ /// Comparing a multi-value column to a value (e.g. `mvCol = 'a'`) is
planned as a comparison on `ARRAY_TO_MV`. As
+ /// the query does not contain `ARRAY_TO_MV` in this case, the error
explains the comparison instead.
+ private QueryException arrayToMvNotSupported(String location, List<? extends
RexExpression> expressions,
+ String hint) {
+ if (containsMultiValueComparison(expressions)) {
+ return new QueryException(QueryErrorCode.QUERY_PLANNING,
+ "Comparing a multi-value column to a value (e.g. mvCol = 'a' or
mvCol IN ('a', 'b')) is not supported in "
+ + location + ". It is only supported in a WHERE clause filter
that can be pushed down to the table "
+ + "scan. Elsewhere, use arrayContainsString(mvCol, 'a') or
arrayContainsInt(mvCol, 1) instead");
Review Comment:
The hint hardcodes `arrayContainsString`/`arrayContainsInt`, but an MV
column could be e.g. LONG/DOUBLE where neither applies verbatim. It's
illustrative so this is fine, just flagging in case you want to soften it to
"e.g. arrayContains...". Non-blocking.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/validate/PinotTypeCoercion.java:
##########
@@ -108,7 +134,64 @@ public boolean binaryComparisonCoercion(SqlCallBinding
binding) {
return super.binaryComparisonCoercion(binding);
}
+ @Override
+ public boolean inOperationCoercion(SqlCallBinding binding) {
+ SqlKind kind = binding.getOperator().getKind();
+ if ((kind == SqlKind.IN || kind == SqlKind.NOT_IN) && binding.operand(1)
instanceof SqlNodeList
+ && isMultiValueColumn(binding, 0) && isScalarValueList(binding,
(SqlNodeList) binding.operand(1))) {
+ return coerceMultiValueComparison(binding, 0, () ->
inOperationCoercion(binding));
+ }
+ return super.inOperationCoercion(binding);
+ }
+
+ /// Returns whether the operand at the given index is a reference to a
multi-value (ARRAY type) column.
+ private static boolean isMultiValueColumn(SqlCallBinding binding, int index)
{
+ return SqlTypeUtil.isArray(binding.getOperandType(index)) &&
isColumnReference(binding.operand(index));
+ }
+
+ /// Returns whether all values of the `IN` list are non-array values. A
`NULL` value is rejected: Calcite types it
+ /// as the array type of the column, and the leaf stage cannot evaluate it
in a multi-value predicate.
+ private static boolean isScalarValueList(SqlCallBinding binding, SqlNodeList
values) {
Review Comment:
Tiny naming nit: `isScalarValueList` reads like a pure predicate but it also
throws on a NULL in the list. Could be worth renaming to something like
`validateScalarValueList` (or splitting the NULL check out) so the side effect
isn't surprising to a future reader. Non-blocking.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/validate/PinotTypeCoercion.java:
##########
@@ -108,7 +134,64 @@ public boolean binaryComparisonCoercion(SqlCallBinding
binding) {
return super.binaryComparisonCoercion(binding);
}
+ @Override
+ public boolean inOperationCoercion(SqlCallBinding binding) {
+ SqlKind kind = binding.getOperator().getKind();
+ if ((kind == SqlKind.IN || kind == SqlKind.NOT_IN) && binding.operand(1)
instanceof SqlNodeList
+ && isMultiValueColumn(binding, 0) && isScalarValueList(binding,
(SqlNodeList) binding.operand(1))) {
+ return coerceMultiValueComparison(binding, 0, () ->
inOperationCoercion(binding));
+ }
+ return super.inOperationCoercion(binding);
+ }
+
+ /// Returns whether the operand at the given index is a reference to a
multi-value (ARRAY type) column.
+ private static boolean isMultiValueColumn(SqlCallBinding binding, int index)
{
+ return SqlTypeUtil.isArray(binding.getOperandType(index)) &&
isColumnReference(binding.operand(index));
+ }
+
+ /// Returns whether all values of the `IN` list are non-array values. A
`NULL` value is rejected: Calcite types it
+ /// as the array type of the column, and the leaf stage cannot evaluate it
in a multi-value predicate.
+ private static boolean isScalarValueList(SqlCallBinding binding, SqlNodeList
values) {
Review Comment:
Tiny naming nit: `isScalarValueList` reads like a pure predicate but it also
throws on a NULL in the list. Could be worth renaming to something like
`validateScalarValueList` (or splitting the NULL check out) so the side effect
isn't surprising to a future reader. Non-blocking.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/validation/ArrayToMvValidationVisitor.java:
##########
@@ -172,6 +174,42 @@ public Void visitUnnest(UnnestNode node, Boolean
isIntermediateStage) {
return null;
}
+ /// Comparing a multi-value column to a value (e.g. `mvCol = 'a'`) is
planned as a comparison on `ARRAY_TO_MV`. As
+ /// the query does not contain `ARRAY_TO_MV` in this case, the error
explains the comparison instead.
+ private QueryException arrayToMvNotSupported(String location, List<? extends
RexExpression> expressions,
+ String hint) {
+ if (containsMultiValueComparison(expressions)) {
+ return new QueryException(QueryErrorCode.QUERY_PLANNING,
+ "Comparing a multi-value column to a value (e.g. mvCol = 'a' or
mvCol IN ('a', 'b')) is not supported in "
+ + location + ". It is only supported in a WHERE clause filter
that can be pushed down to the table "
+ + "scan. Elsewhere, use arrayContainsString(mvCol, 'a') or
arrayContainsInt(mvCol, 1) instead");
Review Comment:
The hint hardcodes `arrayContainsString`/`arrayContainsInt`, but an MV
column could be e.g. LONG/DOUBLE where neither applies verbatim. It's
illustrative so this is fine, just flagging in case you want to soften it to
"e.g. arrayContains...". Non-blocking.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
