FrankChen021 commented on code in PR #19659:
URL: https://github.com/apache/druid/pull/19659#discussion_r3536419190
##########
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 = rewritten.rewriteRequiredColumns(virtualColumnRemap);
Review Comment:
[P2] Seed filter rewrites with identity entries
When `virtualColumnRemap` is non-empty, `rebuildCursorBuildSpec` calls
`rewriteRequiredColumns` with only the remapped VC entries. Existing filter
implementations throw if their own required column is missing from the map, so
a query that remaps one VC but still has an unchanged residual predicate, such
as a materialized VC filter plus `region = 'us-east-1'`, fails during cursor
construction instead of evaluating the residual column. Build an identity map
for the rewritten filter's required columns and overlay the remap, as the
aggregate projection path does.
##########
processing/src/main/java/org/apache/druid/segment/projections/Projections.java:
##########
@@ -681,7 +692,74 @@ 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 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)
+ {
+ final VirtualColumn[] all = queryVcs.getVirtualColumns();
+ if (all.length == 0) {
+ return Map.of();
+ }
+ // Candidate substitutions: query VCs that have a differently-named
equivalent materialized 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())) {
+ candidates.put(outputName, equivalent.getOutputName());
Review Comment:
[P2] Only remap to columns the group cursor can read
`buildClusterVirtualColumnRemap` treats any `groupVcs.findEquivalent` result
as a readable materialized column, but clustered summaries also carry
metadata-only virtual columns. For example, the query-granularity carrier
`__virtualGranularity` is explicitly not a stored column; an equivalent query
VC with another name is now dropped and remapped to `__virtualGranularity`,
which the clustering/per-group selector cannot serve. That returns null/empty
results instead of recomputing or reading `__time`. Restrict remap targets to
clustering columns or stored group columns, or special-case the granularity
carrier.
--
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]
