FrankChen021 commented on code in PR #19659:
URL: https://github.com/apache/druid/pull/19659#discussion_r3595323264
##########
processing/src/main/java/org/apache/druid/segment/projections/Projections.java:
##########
@@ -681,7 +703,99 @@ 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,
+ @Nullable Filter queryFilter
+ )
+ {
+ final VirtualColumn[] all = queryVcs.getVirtualColumns();
+ if (all.length == 0) {
+ return Map.of();
+ }
+ // Columns referenced by a filter that can't rewrite its required columns
must not be remapped, keeps it for the
+ // filter to reference
+ final Set<String> unrewritableFilterColumns =
+ queryFilter != null && !queryFilter.supportsRequiredColumnRewrite()
+ ? queryFilter.getRequiredColumns()
+ : Set.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())
+ // Don't remap to a materialized column whose name is shadowed by
another query virtual column: the remapped
+ // read goes through the per-group delegate, which would resolve the
target name to that (unrelated) query VC
+ // instead of the stored column. Leaving this VC makes it recompute
from its own inputs instead.
+ && queryVcs.getVirtualColumn(equivalent.getOutputName()) == null
+ // Don't remap a VC an unrewritable filter references (see above):
keep it computed for the filter.
+ && !unrewritableFilterColumns.contains(outputName)) {
Review Comment:
[P1] Do not recompute retained VCs from omitted source columns
This exclusion assumes leaving the VC in the spec makes it safely
recomputable, but clustered schemas may materialize a VC while omitting its raw
inputs; this PR's `tenant_lower` example stores `lower(tenant)` without storing
`tenant`. With a non-rewritable filter on `v0 := lower(tenant)`, `v0` is
removed from the remap here, so the per-group filter evaluates it from the
missing `tenant` column instead of `tenant_lower` and can silently discard
valid rows. The same unsafe fallback is used when retained VCs depend on a
materialized candidate. Remapped names need to be available inside the
per-group filter and VC layer without requiring `rewriteRequiredColumns`, or
recomputation must only be chosen when every source input is actually available.
--
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]
