xiangfu0 commented on code in PR #19101:
URL: https://github.com/apache/pinot/pull/19101#discussion_r3742852804
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SortOperator.java:
##########
@@ -73,6 +74,11 @@ public SortOperator(OpChainExecutionContext context,
MultiStageOperator input, S
// - There is no collation
// - Input is already sorted
List<RelFieldCollation> collations = node.getCollations();
+ for (RelFieldCollation collation : collations) {
+ Preconditions.checkArgument(
+
_dataSchema.getColumnDataType(collation.getFieldIndex()).supportsOrdering(),
+ "ORDER BY does not support raw VARIANT values; extract a typed path
with variantGet first");
Review Comment:
**Misleading message (Low).** `supportsOrdering()` is false for VARIANT
**and** arrays/OBJECT/MAP/STRUCT/LIST, so this rejects e.g. `ORDER BY
array_col` with a VARIANT-specific message. The rejection is correct; only the
text is wrong for non-VARIANT types. Same
hard-coded-VARIANT-message-on-a-generic-`supportsOrdering()`-gate appears in
`SortedMailboxReceiveOperator`, `OrderByComparatorFactory`, and
`VariantTypeValidationVisitor#visitSort`/`visitWindow`. Consider naming the
actual unsupported type.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/validation/VariantTypeValidationVisitor.java:
##########
@@ -0,0 +1,185 @@
+/**
+ * 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;
+
+
+/// Rejects operations that would otherwise assign physical byte ordering,
equality, or hashing semantics to a raw
+/// VARIANT value. The visitor has no mutable state and is thread-safe, so
callers may share {@link #INSTANCE}.
+public final class VariantTypeValidationVisitor extends
PlanNodeVisitor.DepthFirstVisitor<Void, Void> {
+ public static final VariantTypeValidationVisitor INSTANCE = new
VariantTypeValidationVisitor();
+
+ private VariantTypeValidationVisitor() {
+ }
+
+ @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) {
Review Comment:
**Consistency (Low, not exploitable).** `validateAggregateInputs` iterates
only the agg-call operands; it never checks `AggregateNode.getGroupKeys()`, so
the planner gate doesn't reject `GROUP BY raw_variant`. This is *not* a hole —
`MultistageGroupByExecutor` and the single-stage GroupBy operators reject it at
runtime via `supportsEquality()/supportsHashing()`. But since every other
operation (sort, set-op, join, window partition) is validated in this visitor,
adding the group-key check here would make the planner the consistent
authoritative gate.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1448,6 +1456,10 @@ static void validatePartialUpsertStrategies(TableConfig
tableConfig, Schema sche
FieldSpec fieldSpec = schema.getFieldSpecFor(column);
Preconditions.checkState(fieldSpec != null, "Merger cannot be applied
to non-existing column: %s", column);
+ if (fieldSpec.getDataType() == DataType.VARIANT) {
+ Preconditions.checkState(columnStrategy ==
UpsertConfig.Strategy.OVERWRITE,
+ "VARIANT column supports only OVERWRITE partial-upsert strategy:
%s", column);
Review Comment:
**Validation gap (Medium).** This VARIANT `OVERWRITE`-only check only runs
for columns explicitly present in `partialUpsertStrategies`. A VARIANT column
**not listed** here falls back to
`upsertConfig.getDefaultPartialUpsertStrategy()` at merge time — see
`PartialUpsertColumnarMerger#merge` (`_column2Mergers.getOrDefault(column,
_defaultColumnValueMerger)`), where the default merger is built from
`getDefaultPartialUpsertStrategy()` and is user-settable to
`INCREMENT`/`APPEND`/`UNION`/`IGNORE`.
Failing scenario: a partial-upsert table with an unlisted VARIANT dimension
and `defaultPartialUpsertStrategy: INCREMENT`. Validation passes, then at
runtime `IncrementMerger` is applied to the byte envelope → failure/corruption;
`IGNORE` would silently retain stale variant values. This violates the
documented "partial upsert only OVERWRITE" contract.
Suggest: when a VARIANT column exists under active partial upsert, also
require the effective default strategy to be OVERWRITE. (Also note the custom
`partialUpsertMergerClass` path applies no VARIANT guard — likely acceptable as
user code, but worth a doc note.)
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FilterOperand.java:
##########
@@ -193,7 +196,10 @@ public Predicate(List<RexExpression> operands, DataSchema
dataSchema, IntPredica
ColumnDataType lhsType = _lhs.getResultType();
ColumnDataType rhsType = _rhs.getResultType();
- if (lhsType == rhsType) {
+ Preconditions.checkArgument((lhsType == ColumnDataType.UNKNOWN ||
lhsType.supportsOrdering())
+ && (rhsType == ColumnDataType.UNKNOWN ||
rhsType.supportsOrdering()),
+ "Raw VARIANT values do not support comparison; extract a typed path
with variantGet first");
Review Comment:
**Behavior regression + misleading message (Medium).** This guard requires
both operands to be `UNKNOWN || supportsOrdering()`, but
`ColumnDataType.supportsOrdering()` is false for **OBJECT, arrays, and MAP** —
not just VARIANT. The class Javadoc just above ("if either side is null or
OBJECT, we best-effort cast data into the other side's data type") documents
OBJECT as an intentionally-supported case, and `OBJECT = OBJECT` previously
took the equal-types branch and ran. After this change such a comparison throws
`"Raw VARIANT values do not support comparison"` — both a narrowing of the
existing contract and a wrong error for non-VARIANT types.
Suggest gating specifically on the VARIANT case (or restoring the
OBJECT/UNKNOWN best-effort path) and making the message name the actual
offending type. Same pattern applies to the `In` guard at line 120
(`supportsEquality()`, also false for OBJECT/arrays).
--
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]
