xiangfu0 commented on code in PR #20001:
URL: https://github.com/apache/pinot/pull/20001#discussion_r4352451191
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/validate/PinotTypeCoercion.java:
##########
@@ -108,7 +134,65 @@ 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)
+ && isScalarValueListRejectingNull(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, and
throws on a `NULL` value: 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 isScalarValueListRejectingNull(SqlCallBinding
binding, SqlNodeList values) {
+ for (SqlNode value : values) {
+ if (SqlUtil.isNullLiteral(value, false)) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException(
+ "NULL is not supported in an IN list compared to multi-value
column: " + binding.operand(0));
Review Comment:
Small asymmetry worth a note: a NULL in an `IN` list errors here, but `mvCol
= NULL` on the binary path is silently wrapped and pushed down as `col = NULL`
(always no-match). Both are defensible, but one form erroring while the sibling
form is accepted may surprise users. Consider a one-line comment explaining the
difference, and maybe a `mvCol = NULL` test so the accepted behavior is pinned.
##########
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 an arrayContains function instead, e.g.
arrayContainsString(mvCol, 'a')");
Review Comment:
Nit: for an INT MV column the only suggested example is
`arrayContainsString`, which won't type-check. Since `arrayContainsInt` exists,
a user on an INT column has to guess the right variant. Fine to leave as-is
given you intentionally softened this to one example, but mentioning both (or
picking by element type) would be friendlier.
##########
pinot-query-runtime/src/test/resources/queries/MultiValueFilters.json:
##########
@@ -0,0 +1,370 @@
+{
+ "mv_filter": {
+ "comment": "Comparisons of a multi-value column to a value. As in the
single-stage engine, = and IN match a row if any value matches, <> and NOT IN
if no value matches. They are only supported in filters pushed down to the leaf
stage.",
+ "tables": {
+ "orders": {
+ "schema": [
+ {
+ "name": "order_id",
+ "type": "STRING"
+ },
+ {
+ "name": "customer_id",
+ "type": "STRING"
+ },
+ {
+ "name": "tags",
+ "type": "STRING",
+ "isSingleValue": false
+ },
+ {
+ "name": "codes",
+ "type": "INT",
+ "isSingleValue": false
+ }
+ ],
+ "inputs": [
+ [
+ "o1",
+ "c1",
+ [
+ "urgent",
+ "vip"
+ ],
+ [
+ 1,
+ 2
+ ]
+ ],
+ [
+ "o2",
+ "c1",
+ [
+ "urgent"
+ ],
+ [
+ 2
+ ]
+ ],
+ [
+ "o3",
+ "c2",
+ [
+ "urgent"
+ ],
+ [
+ 3
+ ]
+ ],
+ [
+ "o4",
+ "c2",
+ [
+ "vip"
+ ],
+ [
+ 4,
+ 5
+ ]
+ ],
+ [
+ "o5",
+ "c3",
+ [
+ "normal"
+ ],
+ [
+ 5
+ ]
+ ]
+ ]
+ },
+ "customers": {
+ "schema": [
+ {
+ "name": "customer_id",
+ "type": "STRING"
+ },
+ {
+ "name": "name",
+ "type": "STRING"
+ }
+ ],
+ "inputs": [
+ [
+ "c1",
+ "Alice"
+ ],
+ [
+ "c2",
+ "Bob"
+ ],
+ [
+ "c3",
+ "Charlie"
+ ]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "Equality on a STRING MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE tags = 'urgent' ORDER BY
order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o2"
+ ],
+ [
+ "o3"
+ ]
+ ]
+ },
+ {
+ "description": "Equality with the value on the left side",
+ "sql": "SELECT order_id FROM {orders} WHERE 'vip' = tags ORDER BY
order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o4"
+ ]
+ ]
+ },
+ {
+ "description": "IN on a STRING MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE tags IN ('vip', 'normal')
ORDER BY order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o4"
+ ],
+ [
+ "o5"
+ ]
+ ]
+ },
+ {
+ "description": "IN with a large value list on a STRING MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE tags IN ('v0', 'v1', 'v2',
'v3', 'v4', 'v5', 'v6', 'v7', 'v8', 'v9', 'v10', 'v11', 'v12', 'v13', 'v14',
'v15', 'v16', 'v17', 'v18', 'v19', 'v20', 'v21', 'v22', 'v23', 'v24', 'normal')
ORDER BY order_id",
+ "outputs": [
+ [
+ "o5"
+ ]
+ ]
+ },
+ {
+ "description": "Two equalities on the same MV column are not folded to
false",
+ "sql": "SELECT order_id FROM {orders} WHERE tags = 'urgent' AND tags =
'vip' ORDER BY order_id",
+ "outputs": [
+ [
+ "o1"
+ ]
+ ]
+ },
+ {
+ "description": "Equality and IN on an INT MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE codes = 5 OR codes IN (1,
3) ORDER BY order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o3"
+ ],
+ [
+ "o4"
+ ],
+ [
+ "o5"
+ ]
+ ]
+ },
+ {
+ "description": "MV equality pushed below a join",
+ "sql": "SELECT o.order_id, c.name FROM {orders} o JOIN {customers} c
ON o.customer_id = c.customer_id WHERE o.tags = 'urgent' ORDER BY o.order_id",
+ "outputs": [
+ [
+ "o1",
+ "Alice"
+ ],
+ [
+ "o2",
+ "Alice"
+ ],
+ [
+ "o3",
+ "Bob"
+ ]
+ ]
+ },
+ {
+ "description": "MV IN pushed below a join with an aggregate above it",
+ "sql": "SELECT c.name, COUNT(*) FROM {orders} o JOIN {customers} c ON
o.customer_id = c.customer_id WHERE o.tags IN ('vip', 'normal') GROUP BY c.name
ORDER BY c.name",
+ "outputs": [
+ [
+ "Alice",
+ 1
+ ],
+ [
+ "Bob",
+ 1
+ ],
+ [
+ "Charlie",
+ 1
+ ]
+ ]
+ },
+ {
+ "description": "Not-equals on an MV column matches rows with no equal
value",
Review Comment:
Coverage gap (low): every input row has a non-empty, non-null MV. The `<>` /
`NOT IN` "no value matches" complement is most error-prone on an empty or NULL
MV row (where `= 'x'` is false and `<> 'x'` should be true). An extra row like
`["o6","c3",[],[6]]` (or a null MV) extending the `<>` / `NOT IN` expected
outputs would lock down that boundary — especially under null-handling-enabled.
##########
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 an arrayContains function instead, e.g.
arrayContainsString(mvCol, 'a')");
Review Comment:
Nit: for an INT MV column the only suggested example is
`arrayContainsString`, which won't type-check. Since `arrayContainsInt` exists,
a user on an INT column has to guess the right variant. Fine to leave as-is
given you intentionally softened this to one example, but mentioning both (or
picking by element type) would be friendlier.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/validate/PinotTypeCoercion.java:
##########
@@ -108,7 +134,65 @@ 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)
+ && isScalarValueListRejectingNull(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, and
throws on a `NULL` value: 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 isScalarValueListRejectingNull(SqlCallBinding
binding, SqlNodeList values) {
+ for (SqlNode value : values) {
+ if (SqlUtil.isNullLiteral(value, false)) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException(
+ "NULL is not supported in an IN list compared to multi-value
column: " + binding.operand(0));
Review Comment:
Small asymmetry worth a note: a NULL in an `IN` list errors here, but `mvCol
= NULL` on the binary path is silently wrapped and pushed down as `col = NULL`
(always no-match). Both are defensible, but one form erroring while the sibling
form is accepted may surprise users. Consider a one-line comment explaining the
difference, and maybe a `mvCol = NULL` test so the accepted behavior is pinned.
##########
pinot-query-runtime/src/test/resources/queries/MultiValueFilters.json:
##########
@@ -0,0 +1,370 @@
+{
+ "mv_filter": {
+ "comment": "Comparisons of a multi-value column to a value. As in the
single-stage engine, = and IN match a row if any value matches, <> and NOT IN
if no value matches. They are only supported in filters pushed down to the leaf
stage.",
+ "tables": {
+ "orders": {
+ "schema": [
+ {
+ "name": "order_id",
+ "type": "STRING"
+ },
+ {
+ "name": "customer_id",
+ "type": "STRING"
+ },
+ {
+ "name": "tags",
+ "type": "STRING",
+ "isSingleValue": false
+ },
+ {
+ "name": "codes",
+ "type": "INT",
+ "isSingleValue": false
+ }
+ ],
+ "inputs": [
+ [
+ "o1",
+ "c1",
+ [
+ "urgent",
+ "vip"
+ ],
+ [
+ 1,
+ 2
+ ]
+ ],
+ [
+ "o2",
+ "c1",
+ [
+ "urgent"
+ ],
+ [
+ 2
+ ]
+ ],
+ [
+ "o3",
+ "c2",
+ [
+ "urgent"
+ ],
+ [
+ 3
+ ]
+ ],
+ [
+ "o4",
+ "c2",
+ [
+ "vip"
+ ],
+ [
+ 4,
+ 5
+ ]
+ ],
+ [
+ "o5",
+ "c3",
+ [
+ "normal"
+ ],
+ [
+ 5
+ ]
+ ]
+ ]
+ },
+ "customers": {
+ "schema": [
+ {
+ "name": "customer_id",
+ "type": "STRING"
+ },
+ {
+ "name": "name",
+ "type": "STRING"
+ }
+ ],
+ "inputs": [
+ [
+ "c1",
+ "Alice"
+ ],
+ [
+ "c2",
+ "Bob"
+ ],
+ [
+ "c3",
+ "Charlie"
+ ]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "Equality on a STRING MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE tags = 'urgent' ORDER BY
order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o2"
+ ],
+ [
+ "o3"
+ ]
+ ]
+ },
+ {
+ "description": "Equality with the value on the left side",
+ "sql": "SELECT order_id FROM {orders} WHERE 'vip' = tags ORDER BY
order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o4"
+ ]
+ ]
+ },
+ {
+ "description": "IN on a STRING MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE tags IN ('vip', 'normal')
ORDER BY order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o4"
+ ],
+ [
+ "o5"
+ ]
+ ]
+ },
+ {
+ "description": "IN with a large value list on a STRING MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE tags IN ('v0', 'v1', 'v2',
'v3', 'v4', 'v5', 'v6', 'v7', 'v8', 'v9', 'v10', 'v11', 'v12', 'v13', 'v14',
'v15', 'v16', 'v17', 'v18', 'v19', 'v20', 'v21', 'v22', 'v23', 'v24', 'normal')
ORDER BY order_id",
+ "outputs": [
+ [
+ "o5"
+ ]
+ ]
+ },
+ {
+ "description": "Two equalities on the same MV column are not folded to
false",
+ "sql": "SELECT order_id FROM {orders} WHERE tags = 'urgent' AND tags =
'vip' ORDER BY order_id",
+ "outputs": [
+ [
+ "o1"
+ ]
+ ]
+ },
+ {
+ "description": "Equality and IN on an INT MV column",
+ "sql": "SELECT order_id FROM {orders} WHERE codes = 5 OR codes IN (1,
3) ORDER BY order_id",
+ "outputs": [
+ [
+ "o1"
+ ],
+ [
+ "o3"
+ ],
+ [
+ "o4"
+ ],
+ [
+ "o5"
+ ]
+ ]
+ },
+ {
+ "description": "MV equality pushed below a join",
+ "sql": "SELECT o.order_id, c.name FROM {orders} o JOIN {customers} c
ON o.customer_id = c.customer_id WHERE o.tags = 'urgent' ORDER BY o.order_id",
+ "outputs": [
+ [
+ "o1",
+ "Alice"
+ ],
+ [
+ "o2",
+ "Alice"
+ ],
+ [
+ "o3",
+ "Bob"
+ ]
+ ]
+ },
+ {
+ "description": "MV IN pushed below a join with an aggregate above it",
+ "sql": "SELECT c.name, COUNT(*) FROM {orders} o JOIN {customers} c ON
o.customer_id = c.customer_id WHERE o.tags IN ('vip', 'normal') GROUP BY c.name
ORDER BY c.name",
+ "outputs": [
+ [
+ "Alice",
+ 1
+ ],
+ [
+ "Bob",
+ 1
+ ],
+ [
+ "Charlie",
+ 1
+ ]
+ ]
+ },
+ {
+ "description": "Not-equals on an MV column matches rows with no equal
value",
Review Comment:
Coverage gap (low): every input row has a non-empty, non-null MV. The `<>` /
`NOT IN` "no value matches" complement is most error-prone on an empty or NULL
MV row (where `= 'x'` is false and `<> 'x'` should be true). An extra row like
`["o6","c3",[],[6]]` (or a null MV) extending the `<>` / `NOT IN` expected
outputs would lock down that boundary — especially under null-handling-enabled.
--
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]
