FrankChen021 commented on code in PR #19659:
URL: https://github.com/apache/druid/pull/19659#discussion_r3587152994
##########
processing/src/main/java/org/apache/druid/segment/projections/Projections.java:
##########
@@ -681,7 +694,86 @@ public static ClusterGroupQueryPlan planClusterGroupQuery(
kept.add(group);
}
}
- return new ClusterGroupQueryPlan(kept, rewriteCache::get);
+ return new ClusterGroupQueryPlan(kept, rewriteCache::get,
virtualColumnRemap);
+ }
+
+ /**
+ * Build a query-level remap of {@code queryVirtualColumnOutputName ->
materializedColumnName} for each query virtual
+ * column that has an equivalent materialized column in the clustered base
table (a clustering column produced by a
+ * group virtual column, or a non-clustering materialized virtual-column
output).
+ * <p>
+ * A remap target must be a column the per-group cursor can actually serve,
so {@code materializedColumns} restricts
+ * candidates to the summary's stored columns (clustering columns included
by construction). Group virtual columns
+ * whose output name is not a stored column are metadata-only carriers --
notably the {@code __virtualGranularity}
+ * query-granularity carrier -- and are never valid substitution targets; a
query VC equivalent to such a carrier is
+ * left in place to recompute (e.g. from {@code __time}) rather than
remapped to an unreadable column.
+ * <p>
+ * A substituted (dropped) query virtual column is read from its
materialized column and never recomputes, so it
+ * imposes no requirement on its own inputs. A query virtual column must
therefore be kept (recomputed, not
+ * substituted) only when it is transitively required by a kept query
virtual column (because query VCs are computed
+ * in the per-group cursor, below the concat-level remap, so a dropped input
a kept VC still references would
+ * incorrectly resolve to null.)
+ */
+ private static Map<String, String> buildClusterVirtualColumnRemap(
+ VirtualColumns queryVcs,
+ VirtualColumns groupVcs,
+ Set<String> materializedColumns
+ )
+ {
+ final VirtualColumn[] all = queryVcs.getVirtualColumns();
+ if (all.length == 0) {
+ return Map.of();
+ }
+ // Candidate substitutions: query VCs that have a differently-named
equivalent materialized (stored) column.
+ final Map<String, String> candidates = new HashMap<>();
+ final Map<String, VirtualColumn> byName = new HashMap<>();
+ for (VirtualColumn vc : all) {
+ final String outputName = vc.getOutputName();
+ byName.put(outputName, vc);
+ final VirtualColumns.Node queryNode = queryVcs.getNode(outputName);
+ if (queryNode == null) {
+ continue;
+ }
+ final VirtualColumn equivalent = groupVcs.findEquivalent(queryNode);
+ if (equivalent != null
+ && !outputName.equals(equivalent.getOutputName())
+ && materializedColumns.contains(equivalent.getOutputName())) {
+ candidates.put(outputName, equivalent.getOutputName());
Review Comment:
[P1] Avoid remapping through a shadowing query virtual column
The candidate target is checked only against stored columns, but query
virtual columns may shadow physical columns. If the query also retains a VC
named like the materialized target, the outer remap routes through the
per-group virtualized delegate and reads that unrelated VC instead of the
stored column. For example, with stored `region_upper = upper(region)`, query
VCs `v1 = upper(region)` and `region_upper = lower(region)` cause `v1` to
silently return lowercase values. Exclude targets shadowed by retained query
VCs or bypass the virtual layer when reading remapped physical columns.
##########
processing/src/main/java/org/apache/druid/segment/projections/Projections.java:
##########
@@ -856,17 +948,29 @@ private static boolean isUnalignedInterval(
}
private static Filter remapFilterToProjection(ProjectionMatchBuilder
matchBuilder, Filter aggFilter)
+ {
+ return rewriteFilterRequiredColumns(aggFilter,
matchBuilder.getRemapColumns());
+ }
+
+ /**
+ * Rewrite {@code filter}'s required columns through {@code remap},
tolerating columns the map doesn't mention.
+ * {@link Filter#rewriteRequiredColumns} throws on any required column
missing from the rewrite map, so we seed an
+ * identity mapping over the filter's own required columns and overlay
{@code remap} on top: residual leaves (a
+ * predicate on a column with no remap entry, e.g. {@code region =
'us-east-1'}) keep their own column, while matched
+ * columns are redirected to the materialized column. Shared by the
aggregate-projection remap
+ * ({@link #remapFilterToProjection}) and the clustered base-table
virtual-column remap
+ * ({@link ClusterGroupQueryPlan#rebuildCursorBuildSpec}).
+ */
+ static Filter rewriteFilterRequiredColumns(Filter filter, Map<String,
String> remap)
{
final Map<String, String> filterRewrites = new HashMap<>();
- // start with identity
- for (String required : aggFilter.getRequiredColumns()) {
+ // start with identity so residual columns not mentioned in the remap are
preserved rather than rejected
+ for (String required : filter.getRequiredColumns()) {
filterRewrites.put(required, required);
}
- // overlay projection rewrites
- filterRewrites.putAll(matchBuilder.getRemapColumns());
-
- final Filter remappedAggFilter =
aggFilter.rewriteRequiredColumns(filterRewrites);
- return remappedAggFilter;
+ // overlay the remap so matched columns win over their identity entry
+ filterRewrites.putAll(remap);
+ return filter.rewriteRequiredColumns(filterRewrites);
Review Comment:
[P2] Do not unconditionally rewrite unsupported filters
Whenever any VC remap exists, this calls `rewriteRequiredColumns` on the
residual filter even if none of its columns need remapping. Core filters
including `SpatialFilter`, `DimensionPredicateFilter`,
`ColumnComparisonFilter`, and `JavaScriptFilter` do not support required-column
rewriting, so a query that groups or selects a remappable VC alongside one of
these filters now throws `UnsupportedOperationException`. Preserve the filter
when its required columns do not intersect the remap; if they do intersect and
rewriting is unsupported, disable that substitution.
##########
processing/src/main/java/org/apache/druid/segment/projections/ClusterGroupQueryPlan.java:
##########
@@ -76,21 +93,51 @@ public Filter rewriteFor(TableClusterGroupSpec group)
/**
* Rebuild {@code spec} for {@code group}'s per-group cursor by swapping in
this plan's per-group filter rewrite
- * (see {@link #rewriteFor}). Returns {@code spec} unchanged when there is
no filter, or when the rewrite is
- * identical to the original (no clustering leaves folded), so the common
no-op case allocates nothing. Shared by
- * the {@link org.apache.druid.segment.QueryableIndexCursorFactory}
(historical) and
- * {@link
org.apache.druid.segment.incremental.IncrementalIndexCursorFactory} (realtime)
clustered dispatch.
+ * (see {@link #rewriteFor}) and, when {@link #virtualColumnRemap()} is
non-empty, substituting any query virtual
+ * columns that are equivalent to a materialized column.
+ * <p>
+ * When there is a remap, the matched query virtual columns (the remap keys)
are dropped from the spec's virtual
+ * columns, and the per-group filter's required columns are rewritten via
the remap so that non-clustering-VC filter
+ * leaves point at the materialized physical column (clustering-VC leaves
were already folded to TRUE / FALSE by the
+ * per-group rewrite walk, so they won't reference the dropped virtual
columns). The grouping / select / aggregator
+ * references are served instead by the {@link
org.apache.druid.segment.RemapColumnSelectorFactory} the cursor
+ * factory wraps around the {@link ClusteringColumnSelectorFactory}.
*/
public CursorBuildSpec rebuildCursorBuildSpec(CursorBuildSpec spec,
TableClusterGroupSpec group)
{
- if (spec.getFilter() == null) {
- return spec;
+ if (virtualColumnRemap.isEmpty()) {
+ if (spec.getFilter() == null) {
+ return spec;
+ }
+ final Filter rewritten = rewriteFor(group);
+ if (rewritten == spec.getFilter()) {
+ return spec;
+ }
+ return CursorBuildSpec.builder(spec).setFilter(rewritten).build();
}
- final Filter rewritten = rewriteFor(group);
- if (rewritten == spec.getFilter()) {
- return spec;
+
+ // Drop the remapped query virtual columns from the per-group spec; the
materialized columns they map to are
+ // either the per-group constant clustering column or a per-group physical
column, both served directly by the
+ // ClusteringColumnSelectorFactory (the cursor factory additionally wraps
it with a RemapColumnSelectorFactory so
+ // the original query VC names resolve to the materialized columns).
+ final List<VirtualColumn> prunedVcs = new ArrayList<>();
+ for (VirtualColumn vc : spec.getVirtualColumns().getVirtualColumns()) {
+ if (!virtualColumnRemap.containsKey(vc.getOutputName())) {
+ prunedVcs.add(vc);
+ }
}
- return CursorBuildSpec.builder(spec).setFilter(rewritten).build();
+
+ // Per-group filter rewrite first (folds clustering leaves), then remap
any surviving non-clustering-VC leaves to
+ // the materialized physical column.
+ Filter rewritten = spec.getFilter() == null ? null : rewriteFor(group);
+ if (rewritten != null) {
+ rewritten = Projections.rewriteFilterRequiredColumns(rewritten,
virtualColumnRemap);
+ }
+
+ return CursorBuildSpec.builder(spec)
+ .setFilter(rewritten)
+ .setVirtualColumns(VirtualColumns.create(prunedVcs))
Review Comment:
[P2] Update physicalColumns for materialized remap targets
The rebuilt spec drops remapped VCs but copies the original non-null
`physicalColumns`, which contains the VCs' raw inputs rather than their
materialized targets. Native engines populate this set. On partial segments,
prefetch therefore omits targets such as `region_upper`, and the later remapped
selector triggers synchronous deep-storage I/O during cursor use; incremental
selector capability snapshots can likewise reject the undeclared target. When
`physicalColumns` is non-null, add all remap target columns to the rebuilt set.
--
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]
