FrankChen021 commented on code in PR #19659: URL: https://github.com/apache/druid/pull/19659#discussion_r3645262023
########## processing/src/main/java/org/apache/druid/segment/incremental/ClusteringAwareIncrementalIndexColumnSelectorFactory.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.druid.segment.incremental; + +import org.apache.druid.query.Order; +import org.apache.druid.query.dimension.DimensionSpec; +import org.apache.druid.segment.ColumnValueSelector; +import org.apache.druid.segment.CursorBuildSpec; +import org.apache.druid.segment.DimensionSelector; +import org.apache.druid.segment.column.ColumnCapabilities; +import org.apache.druid.segment.column.RowSignature; +import org.apache.druid.segment.projections.ClusteringColumnSelectorFactory; + +import javax.annotation.Nullable; + +/** + * An {@link IncrementalIndexColumnSelectorFactory} that also serves a cluster group's clustering columns as per-group + * constants. A clustered base table stores each cluster group's rows WITHOUT the (constant) clustering columns, so the + * group's row selector cannot resolve them directly. + */ +final class ClusteringAwareIncrementalIndexColumnSelectorFactory extends IncrementalIndexColumnSelectorFactory +{ + private final RowSignature clusteringColumns; + /** + * Serves the clustering columns as per-group constants. Its delegate is intentionally the throwing placeholder: this + * factory is only ever asked for clustering columns, and {@link ClusteringColumnSelectorFactory} serves those from + * the group's constant tuple without touching its delegate. + */ + private final ClusteringColumnSelectorFactory clusteringConstants; + + ClusteringAwareIncrementalIndexColumnSelectorFactory( + IncrementalIndexRowSelector rowSelector, + IncrementalIndexRowHolder rowHolder, + CursorBuildSpec cursorBuildSpec, + Order timeOrder, + RowSignature clusteringColumns, + Object[] clusteringValues + ) + { + super(rowSelector, rowHolder, cursorBuildSpec, timeOrder); + this.clusteringColumns = clusteringColumns; + this.clusteringConstants = new ClusteringColumnSelectorFactory( + ClusteringColumnSelectorFactory.UNINITIALIZED_DELEGATE, + clusteringColumns, + clusteringValues + ); + } + + @Override + public DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec) + { + if (clusteringColumns.indexOf(dimensionSpec.getDimension()) < 0) { Review Comment: [P1] Preserve query virtual-column precedence These overrides intercept clustering names before the superclass can resolve query virtual columns. If a retained query VC depends on another query VC whose output shadows a clustering column, that dependency now reads the physical per-group constant instead of the query VC, silently changing realtime results. Check the query virtual columns first, and only serve a clustering constant when no query VC owns the requested name. ########## processing/src/main/java/org/apache/druid/data/input/impl/ClusteredValueGroupsBaseTableProjectionSpec.java: ########## @@ -311,6 +314,66 @@ private static void validate(List<DimensionSchema> columns, List<String> cluster } } + /** + * Rules to keep virtual columns definitions reasonable: + * <ul> + * <li><b>inputs</b>: every input of a virtual column must be a stored column (declared in {@code columns}) or + * another virtual column in the spec.</li> + * <li><b>outputs</b>: every virtual column must either be materialized (its output declared in {@code columns}) or + * be an intermediary that feeds another virtual column. A virtual column that is neither materializes nothing and + * is used by nothing and dead metadata and so it is rejected.</li> + * </ul> + * The query-granularity carrier ({@link Granularities#GRANULARITY_VIRTUAL_COLUMN_NAME}) is special handled to + * capture how __time is computed, so it is exempt from the output rule. + */ + private static void validateVirtualColumns(VirtualColumns virtualColumns, List<DimensionSchema> columns) + { + final VirtualColumn[] all = virtualColumns.getVirtualColumns(); + if (all.length == 0) { + return; + } + final Set<String> columnNames = Sets.newHashSetWithExpectedSize(columns.size()); + for (DimensionSchema column : columns) { + columnNames.add(column.getName()); + } + // The output rule below lets a virtual column go unstored when it is exempt: an intermediary that feeds another + // virtual column (collected during the input pass), or the metadata-only query-granularity carrier (seeded here). + final Set<String> outputExempt = new HashSet<>(); + outputExempt.add(Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME); + // input rule: every input must be a stored column or another virtual column; an input that is a virtual column + // makes that virtual column an intermediary, exempt from having to be materialized itself. + for (VirtualColumn virtualColumn : all) { + for (String input : virtualColumn.requiredColumns()) { + final boolean isStored = columnNames.contains(input); + final boolean isVirtual = virtualColumns.exists(input); + if (!isStored && !isVirtual) { + throw InvalidInput.exception( + "virtual column [%s] reads column [%s], which is neither a stored column nor another virtual column;" + + " clustered base table virtual columns must be computable from stored columns (retain [%s] in" + + " 'columns', or use a transformSpec for in-place transforms)", + virtualColumn.getOutputName(), + input, + input + ); + } + if (isVirtual) { + outputExempt.add(input); Review Comment: [P2] Normalize dot-notation virtual-column dependencies For a required field such as `map.foo`, `virtualColumns.exists(input)` succeeds through the dot-support VC whose actual output is `map`, but this records the literal `map.foo` as exempt. The later output pass checks `map`, does not find it in `outputExempt`, and rejects the valid intermediary as dangling. Resolve the owning virtual column and exempt its output name rather than the dotted reference. -- 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]
