FrankChen021 commented on code in PR #19659:
URL: https://github.com/apache/druid/pull/19659#discussion_r3536280074
##########
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 residual filter rewrites with identity mappings
When a remap exists, this rewrites the entire residual filter using only
`virtualColumnRemap`. Filters such as `EqualityFilter` and `RangeFilter`
require the rewrite map to contain their own required column and throw if it is
absent. A query with `v0` remapped plus an unrelated residual filter like
`region = 'us-east-1'` will fold/remap `v0`, leave `region` as a residual
filter, then pass only `{v0 -> tenant_lower}` here and fail during cursor
construction. Build an identity map for `rewritten.getRequiredColumns()` before
overlaying the remap.
##########
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 readable clustered columns
`findEquivalent` searches all group virtual-column metadata, but not every
group virtual-column output is a stored/readable column. For example, clustered
base-table query-granularity carriers live in `virtualColumns` and are
explicitly not stored in columns. If a query VC with a different name matches
such a metadata-only VC, this records a remap, `rebuildCursorBuildSpec` drops
the query VC, and the selector reads the missing target column, producing null
or incorrect results. Gate candidates to clustering columns or names present in
the clustered summary's stored columns.
--
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]
