clintropolis commented on code in PR #19659:
URL: https://github.com/apache/druid/pull/19659#discussion_r3642245039
##########
processing/src/main/java/org/apache/druid/segment/projections/Projections.java:
##########
@@ -681,13 +703,122 @@ 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>
+ * The substitution is always a pure optimisation: {@code
ClusteredValueGroupsBaseTableProjectionSpec} requires every
+ * clustered virtual column's inputs to be stored columns, so a query VC
equivalent to one can always be recomputed
+ * from stored columns and reading the materialized column merely skips that
recomputation.
+ * <p>
+ * A query VC that {@code queryFilter} references is left unremapped when
the filter can't rewrite its required
+ * columns ({@link Filter#supportsRequiredColumnRewrite()} is false, e.g.
spatial / javascript / column-comparison
+ * filters): the filter can't have the VC name swapped for the materialized
column, and since a remappable VC is
+ * (per the spec) recomputable from stored columns, keeping it lets the
filter read the recomputed value instead of
+ * throwing on an unsupported rewrite.
+ * <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();
+ }
+ // A filter that can't rewrite its required columns (spatial, javascript,
column-comparison, ...) can't have a
+ // remapped VC name swapped for the materialized column in its own
required-column set, so any VC it references
+ // must stay in place. ClusteredValueGroupsBaseTableProjectionSpec
guarantees every clustered VC is recomputable
Review Comment:
clustered segments are not released, and undocumented/experimental, so I
think this is an acceptable risk. In practice the schema can only be populated
from the spec, so the spec validation should be sufficient
--
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]
