xiangfu0 commented on code in PR #19101:
URL: https://github.com/apache/pinot/pull/19101#discussion_r3920906239
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FilterOperand.java:
##########
@@ -193,7 +197,12 @@ public Predicate(List<RexExpression> operands, DataSchema
dataSchema, IntPredica
ColumnDataType lhsType = _lhs.getResultType();
ColumnDataType rhsType = _rhs.getResultType();
- if (lhsType == rhsType) {
+ // Reject raw VARIANT operands only; other non-orderable types (OBJECT,
arrays, MAP) keep their existing
+ // best-effort comparison behavior. VARIANT is opaque because its PVAR
byte encoding is not a canonical
+ // semantic ordering, so a comparison must extract a typed scalar first.
+ Preconditions.checkArgument(lhsType != ColumnDataType.VARIANT && rhsType
!= ColumnDataType.VARIANT,
+ "Raw VARIANT values do not support comparison; extract a typed path
with variantGet first");
+ if (lhsType == ColumnDataType.UNKNOWN || rhsType ==
ColumnDataType.UNKNOWN || lhsType == rhsType) {
Review Comment:
Kept the SQL-null short-circuit in both engines and added an explicit
compatibility note in the PR description. The existing left/right NULL-literal
regressions remain part of verification.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnarValueNormalizer.java:
##########
@@ -28,21 +28,21 @@
/// (`buildColumnar`) path.
///
/// The row-major build runs each record through a `TransformPipeline` whose
-/// `NullValueTransformer` substitutes [FieldSpec#getDefaultNullValue()] for
`null`
-/// and whose `DataTypeTransformer` coerces every value to the column's stored
type (e.g.
+/// `DataTypeTransformer` coerces every non-null value to the column's stored
type (e.g.
/// `Boolean` → `Integer` for a `BOOLEAN` column stored as `INT`,
-/// `Timestamp` → `Long` for `TIMESTAMP`). The column-major driver
deliberately runs
-/// with no transform pipeline, so a non-segment source (e.g. Arrow) delivers
values in the source's
+/// `Timestamp` → `Long` for `TIMESTAMP`) and whose `NullValueTransformer`
substitutes
+/// [FieldSpec#getDefaultNullValue()] for values that remain `null`. The
column-major driver deliberately runs with
+/// no transform pipeline, so a non-segment source (e.g. Arrow) delivers
values in the source's
/// logical type with raw `null`s — which the typed collectors / index
creators do not accept.
///
/// This helper applies the equivalent of those two transformers to one value,
in the same order:
///
-/// 1. `NullValueTransformer`: a `null` value becomes the column default — the
scalar
-/// default for single-value columns, or a one-element `Object[]` of
that scalar for
-/// multi-value columns (matching
`NullValueTransformerUtils.getDefaultNullValue`).
-/// 2. `DataTypeTransformer`: [DataTypeTransformerUtils#transformValue]
standardizes the
+/// 1. `DataTypeTransformer`: [DataTypeTransformerUtils#transformValue]
standardizes the
Review Comment:
Documented the columnar normalization ordering change and its shared-path
impact in the PR description. The implementation now matches the row pipeline.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnarValueNormalizerTest.java:
##########
@@ -0,0 +1,43 @@
+/**
+ * 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.segment.local.segment.creator.impl;
+
+import org.apache.pinot.spi.data.DimensionFieldSpec;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.PinotDataType;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertSame;
+
+
+public class ColumnarValueNormalizerTest {
+ private static final String COLUMN = "payload";
+
+ @Test
+ public void testNullVariantReturnsDefaultSentinelWithoutDecoding() {
Review Comment:
Expanded `ColumnarValueNormalizerTest` to pin BOOLEAN as `Integer 0`,
TIMESTAMP as `Long 0`, a one-element MV default array, and the Variant sentinel.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1970,7 +2060,7 @@ private static void
validateStarTreeIndexConfigs(List<StarTreeIndexConfig> starT
List<String> dimensionsSplitOrder =
starTreeIndexConfig.getDimensionsSplitOrder();
assert CollectionUtils.isNotEmpty(dimensionsSplitOrder);
for (String dimension : dimensionsSplitOrder) {
- if (timestampIndexColumns.contains(dimension)) {
+ if (timestampIndexColumns.contains(dimension) &&
schema.getFieldSpecFor(dimension) == null) {
Review Comment:
Reworked this to preserve the unconditional timestamp-derived-name skip,
avoiding schema-provenance-dependent validation. A schema-declared collision is
checked only to reject VARIANT; declared non-Variant and generated-column cases
stay valid. Both paths are tested, and the change is isolated in `07d3dea1da`.
##########
pinot-common/pom.xml:
##########
@@ -200,6 +200,18 @@
<groupId>org.apache.pinot</groupId>
<artifactId>pinot-timeseries-spi</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.parquet</groupId>
+ <artifactId>parquet-variant</artifactId>
Review Comment:
Removed the unsound parquet-column exclusion, excluded unused
parquet-jackson, and added the requested global-shade / apache/pinot#18459
note. `mvn -pl pinot-common dependency:tree -Dincludes=org.apache.parquet` now
shows parquet-variant -> parquet-common/format-structures plus
parquet-column/encoding, with no parquet-jackson.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -470,6 +472,11 @@ public void lookupOperatorOverloads(SqlIdentifier opName,
@Nullable SqlFunctionC
if (!opName.isSimple()) {
return;
}
+ if (!_nullHandlingEnabled &&
TransformFunctionType.requiresNullHandling(opName.getSimple())) {
Review Comment:
The operator table now canonicalizes once, returns on map miss, and consults
a constructor-cached empty-or-canonical gated-name set only for Pinot operator
hits. The user error is a `QueryException` with `QUERY_VALIDATION`, matching
broker/result validation.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -117,6 +122,17 @@ public enum TransformFunctionType {
JSON_EXTRACT_KEY("jsonExtractKey", ReturnTypes.TO_ARRAY,
OperandTypes.family(
List.of(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER,
SqlTypeFamily.CHARACTER), i -> i > 1)),
+ VARIANT_GET("variantGet",
TransformFunctionType::variantGetReturnTypeInference,
variantGetOperandTypeChecker()),
+ TRY_VARIANT_GET("tryVariantGet",
TransformFunctionType::variantGetReturnTypeInference,
+ variantGetOperandTypeChecker()),
+ VARIANT_EXISTS("variantExists", ReturnTypes.BOOLEAN_NULLABLE,
variantPathOperandTypeChecker()),
+ IS_VARIANT_NULL("isVariantNull", ReturnTypes.BOOLEAN,
optionalVariantPathOperandTypeChecker()),
+ VARIANT_TYPE_OF("variantTypeOf", ReturnTypes.VARCHAR_2000_NULLABLE,
optionalVariantPathOperandTypeChecker()),
+ VARIANT_TO_JSON("variantToJson", ReturnTypes.VARCHAR_2000_NULLABLE,
OperandTypes.ANY),
+ PARSE_JSON_TO_VARIANT("parseJson",
TransformFunctionType::nullableVariantReturnTypeInference,
Review Comment:
Made `parseJsonToVariant` / `tryParseJsonToVariant` the primary names and
retained Spark-compatible `parseJson` aliases. `variantToJson` now uses a
VARIANT operand checker, so non-Variant arguments fail during planning.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/HashJoinOperator.java:
##########
@@ -79,18 +99,55 @@ public HashJoinOperator(OpChainExecutionContext context,
MultiStageOperator left
_nullKeyRightRows = needUnmatchedRightRows() ? new ArrayList<>() : null;
}
- /// Constructor that takes the schema for NonEquiEvaluator as an argument
+ /// Constructor that takes the schema for NonEquiEvaluator as an argument.
+ ///
+ /// <p>For SEMI and ANTI joins whose node does not carry its inputs, the
result schema contains only left columns, so
+ /// this legacy constructor cannot validate the right key's logical type.
New callers that need right-side VARIANT
+ /// validation must use the overload that accepts {@code rightSchema}.
public HashJoinOperator(OpChainExecutionContext context, MultiStageOperator
leftInput, DataSchema leftSchema,
MultiStageOperator rightInput, JoinNode node, DataSchema
nonEquiEvaluationSchema) {
+ this(context, leftInput, leftSchema, rightInput,
tryInferRightSchema(leftSchema, node), node,
+ nonEquiEvaluationSchema, false);
+ }
+
+ /// Constructor that takes the schema for NonEquiEvaluator as an argument
+ public HashJoinOperator(OpChainExecutionContext context, MultiStageOperator
leftInput, DataSchema leftSchema,
+ MultiStageOperator rightInput, DataSchema rightSchema, JoinNode node,
DataSchema nonEquiEvaluationSchema) {
+ this(context, leftInput, leftSchema, rightInput, rightSchema, node,
nonEquiEvaluationSchema, true);
+ }
+
+ private HashJoinOperator(OpChainExecutionContext context, MultiStageOperator
leftInput, DataSchema leftSchema,
+ MultiStageOperator rightInput, @Nullable DataSchema rightSchema,
JoinNode node,
+ DataSchema nonEquiEvaluationSchema, boolean rightSchemaRequired) {
super(context, leftInput, leftSchema, rightInput, node,
nonEquiEvaluationSchema);
List<Integer> leftKeys = node.getLeftKeys();
Preconditions.checkState(!leftKeys.isEmpty(), "Hash join operator requires
join keys");
+ Preconditions.checkArgument(!rightSchemaRequired || rightSchema != null,
"Right input schema must not be null");
+ JoinKeyTypeValidator.validate(node, leftSchema, rightSchema);
_leftKeySelector = KeySelectorFactory.getKeySelector(leftKeys);
_rightKeySelector = KeySelectorFactory.getKeySelector(node.getRightKeys());
_rightTable = createLookupTable(leftKeys, leftSchema);
_matchedRightRows = needUnmatchedRightRows() ? new HashMap<>() : null;
}
+ @Nullable
+ private static DataSchema tryInferRightSchema(DataSchema leftSchema,
JoinNode node) {
Review Comment:
Deprecated both legacy constructors and made the null-right-schema path
still validate left keys. All in-tree construction now passes the right schema
explicitly, including SEMI/ANTI joins, and tests cover right-side raw VARIANT
rejection. The limitation is disclosed in the PR body.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/DistinctPlanNode.java:
##########
@@ -54,6 +55,18 @@ public DistinctPlanNode(SegmentContext segmentContext,
QueryContext queryContext
@Override
public Operator<DistinctResultsBlock> run() {
List<ExpressionContext> expressions = _queryContext.getSelectExpressions();
+ for (ExpressionContext expression : expressions) {
+ String column = expression.getIdentifier();
+ if (column != null) {
+ DataType dataType = _indexSegment.getDataSource(column,
_queryContext.getSchema())
Review Comment:
Added a one-shot cached `QueryContext.hasVariantColumns()` flag. DISTINCT
and aggregation identifier validation skip all per-expression data-source
lookups when the query schema has no VARIANT; focused tests cover both
fast-skip and validation paths.
##########
compatibility-verifier/compCheck.sh:
##########
@@ -496,14 +498,24 @@ setupControllerVariables
setupBrokerVariables
setupServerVariables
-export JAVA_OPTS="-DControllerPort=${CONTROLLER_PORT}
-DBrokerQueryPort=${BROKER_QUERY_PORT} -DServerAdminPort=${SERVER_ADMIN_PORT}"
-
mkdir ${PID_DIR}
mkdir ${LOG_DIR}
oldTargetDir="$workingDir"/oldTargetDir
newTargetDir="$workingDir"/newTargetDir
+oldServerSupportsVariant=false
+oldExpressionsProto="${oldTargetDir}/pinot-common/src/main/proto/expressions.proto"
+if [ -f "${oldExpressionsProto}" ] \
+ && grep -Eq '^[[:space:]]*VARIANT[[:space:]]*=[[:space:]]*24[[:space:]]*;'
"${oldExpressionsProto}"; then
Review Comment:
Replaced source-text grep with capability detection against the built old
`pinot-common` artifact via `FileContainsOp`; inability to inspect is a hard
failure rather than a silent skip. Also documented intentional generation reuse
for the old-broker/new-server phase.
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/main/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordReader.java:
##########
@@ -122,9 +159,11 @@ public GenericRow next(GenericRow reuse)
} catch (Exception e) {
Review Comment:
Moved the full reader init/rewind/close robustness changes into standalone
commit `55f1aa6d37` for backportability. `useAvroParquetRecordReader()` now
retains explicit selected-reader state before init and after close, with a
dedicated regression test.
--
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]
