xiangfu0 commented on code in PR #19101:
URL: https://github.com/apache/pinot/pull/19101#discussion_r3920905680
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -684,6 +691,9 @@ protected void appendDefaultNullValue(ObjectNode jsonNode) {
case BYTES:
jsonNode.put(key, BytesUtils.toHexString((byte[])
_defaultNullValue));
break;
+ case VARIANT:
Review Comment:
Collapsed BYTES and VARIANT into the shared serialization switch arm. The
conversion arms remain separate because only VARIANT validates non-empty
envelopes.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -465,6 +473,56 @@ public ColumnDataType getStoredType() {
return _storedColumnDataType;
}
+ public boolean supportsEquality() {
Review Comment:
Kept the capability predicates as generic metadata but scoped every new
enforcement caller specifically to raw VARIANT. The generic capability visitor
was replaced with `RawVariantValidationVisitor`, so arrays/OBJECT/MAP are not
newly rejected.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/validation/RawVariantValidationVisitor.java:
##########
@@ -0,0 +1,199 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.query.planner.validation;
+
+import java.util.List;
+import org.apache.calcite.rel.RelFieldCollation;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.VariantUtils;
+import org.apache.pinot.query.planner.logical.RexExpression;
+import org.apache.pinot.query.planner.plannode.AggregateNode;
+import org.apache.pinot.query.planner.plannode.JoinNode;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.apache.pinot.query.planner.plannode.PlanNodeVisitor;
+import org.apache.pinot.query.planner.plannode.SetOpNode;
+import org.apache.pinot.query.planner.plannode.SortNode;
+import org.apache.pinot.query.planner.plannode.WindowNode;
+import org.apache.pinot.spi.exception.QueryErrorCode;
+import org.apache.pinot.spi.exception.QueryException;
+
+
+/// Validates that each logical input type supports the capabilities required
by its operation, including equality,
+/// hashing, ordering, aggregation, and lossless result projection. The
visitor has no mutable state and is thread-safe,
+/// so callers may share {@link #INSTANCE}.
+public final class TypeCapabilityValidationVisitor extends
PlanNodeVisitor.DepthFirstVisitor<Void, Void> {
+ public static final TypeCapabilityValidationVisitor INSTANCE = new
TypeCapabilityValidationVisitor();
+
+ private TypeCapabilityValidationVisitor() {
+ }
+
+ @Override
+ protected boolean traverseStageBoundary() {
+ return false;
+ }
+
+ @Override
+ public Void visitAggregate(AggregateNode node, Void context) {
+ List<PlanNode> inputs = node.getInputs();
+ if (inputs.size() == 1) {
+ validateAggregateInputs(node, inputs.get(0).getDataSchema());
+ }
+ return super.visitAggregate(node, context);
+ }
+
+ /// Validates aggregate operands against their logical input schema.
+ ///
+ /// <p>This method is also invoked by the runtime as a defensive check for
plans that did not pass through the
+ /// current broker planner.
+ public static void validateAggregateInputs(AggregateNode node, DataSchema
inputSchema) {
+ for (int key : node.getGroupKeys()) {
+ DataSchema.ColumnDataType dataType = inputSchema.getColumnDataType(key);
+ if (!dataType.supportsEquality() || !dataType.supportsHashing()) {
+ throw unsupported("GROUP BY", dataType);
+ }
+ }
+ validateAggregateInputs(node.getAggCalls(), inputSchema);
+ }
+
+ /// Validates aggregate or window-function operands against their logical
input schema.
+ public static void validateAggregateInputs(List<RexExpression.FunctionCall>
aggCalls, DataSchema inputSchema) {
+ for (RexExpression.FunctionCall aggCall : aggCalls) {
+ if (isRawVariantIndependent(aggCall)) {
+ continue;
+ }
+ for (RexExpression operand : aggCall.getFunctionOperands()) {
+ DataSchema.ColumnDataType dataType = getLogicalType(operand,
inputSchema);
+ if (!dataType.supportsDirectAggregation()) {
+ throw unsupported("Aggregate function " + aggCall.getFunctionName(),
dataType);
+ }
+ }
+ }
+ }
+
+ /// Rejects a raw VARIANT result when query null handling is disabled.
Without the null bitmap, the reserved empty
+ /// byte placeholder cannot participate in normal disabled-null semantics
while also remaining distinguishable from
+ /// an encoded Variant null.
+ public static void validateResultSchema(DataSchema resultSchema, boolean
nullHandlingEnabled) {
+ if (VariantUtils.requiresNullHandlingForRawVariantResult(resultSchema,
nullHandlingEnabled)) {
+ throw new QueryException(QueryErrorCode.QUERY_PLANNING,
+ VariantUtils.RAW_VARIANT_REQUIRES_NULL_HANDLING_ERROR);
+ }
+ }
+
+ @Override
+ public Void visitSort(SortNode node, Void context) {
+ DataSchema dataSchema = node.getDataSchema();
+ for (RelFieldCollation collation : node.getCollations()) {
+ int fieldIndex = collation.getFieldIndex();
+ DataSchema.ColumnDataType dataType =
dataSchema.getColumnDataType(fieldIndex);
+ if (!dataType.supportsOrdering()) {
+ throw unsupported("ORDER BY", dataType);
+ }
+ }
+ return super.visitSort(node, context);
+ }
+
+ @Override
+ public Void visitSetOp(SetOpNode node, Void context) {
Review Comment:
Replaced the generic visitor with `RawVariantValidationVisitor`; it checks
only raw VARIANT. Existing array/OBJECT/MAP set/sort/group/window behavior is
retained, so the `ArrayToMvValidationVisitor` ordering is no longer
load-bearing for this feature.
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/planner/validation/TypeCapabilityValidationVisitorTest.java:
##########
@@ -0,0 +1,276 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.query.planner.validation;
+
+import java.util.List;
+import org.apache.calcite.rel.RelFieldCollation;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.query.planner.logical.RexExpression;
+import org.apache.pinot.query.planner.plannode.AggregateNode;
+import org.apache.pinot.query.planner.plannode.JoinNode;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.apache.pinot.query.planner.plannode.SetOpNode;
+import org.apache.pinot.query.planner.plannode.SortNode;
+import org.apache.pinot.query.planner.plannode.ValueNode;
+import org.apache.pinot.query.planner.plannode.WindowNode;
+import org.apache.pinot.spi.exception.QueryException;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TypeCapabilityValidationVisitorTest {
+ private static final DataSchema VARIANT_SCHEMA =
+ new DataSchema(new String[]{"payload"}, new
DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.VARIANT});
+ private static final DataSchema TYPED_EXTRACTION_SCHEMA =
+ new DataSchema(new String[]{"typedPayload"}, new
DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.STRING});
+
+ @Test
+ public void testRejectsVariantOrderBy() {
+ SortNode sortNode = new SortNode(0, VARIANT_SCHEMA,
PlanNode.NodeHint.EMPTY, List.of(),
+ List.of(new RelFieldCollation(0)), 10, 0);
+
+ QueryException exception =
+ Assert.expectThrows(QueryException.class, () ->
sortNode.visit(TypeCapabilityValidationVisitor.INSTANCE, null));
+ Assert.assertTrue(exception.getMessage().contains("ORDER BY"));
+ }
+
+ @Test
+ public void testNamesUnsupportedNonVariantOrderByType() {
Review Comment:
Removed the generic capability test suite with the generic visitor. The
replacement suite validates raw VARIANT only; because the proposed array
widening was reverted, there is no new array-rejection contract to pin.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/utils/OrderByComparatorFactory.java:
##########
@@ -51,6 +52,12 @@ public static Comparator<Object[]>
getComparator(List<OrderByExpressionContext>
throw new BadQueryRequestException("MV expression: " +
orderByExpressions.get(i)
+ " should not be included in the ORDER-BY clause");
}
+ FieldSpec.DataType dataType = orderByColumnContexts[i].getDataType();
+ if (!dataType.supportsOrdering()) {
Review Comment:
Narrowed the single-stage ORDER BY and predicate guards to raw VARIANT.
`DataType.UNKNOWN` remains allowed for `ORDER BY NULL`, and RANGE no longer
rejects UNKNOWN.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/utils/OrderByComparatorFactoryTest.java:
##########
@@ -104,4 +109,32 @@ public void testTwoNullsCompareNextColumn() {
assertEquals(extractColumn(_rows, COLUMN2_INDEX), Arrays.asList(1, 2, 3));
}
+
+ @Test
+ public void testRejectsRawVariant() {
Review Comment:
Added `testAllowsUnknownForOrderByNull`, which constructs an UNKNOWN column
context and verifies comparator creation succeeds.
--
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]
