feat: remap query virtual columns to clustered base table columns when equivalent#19659
feat: remap query virtual columns to clustered base table columns when equivalent#19659clintropolis wants to merge 3 commits into
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.5
| // the materialized physical column. | ||
| Filter rewritten = spec.getFilter() == null ? null : rewriteFor(group); | ||
| if (rewritten != null) { | ||
| rewritten = rewritten.rewriteRequiredColumns(virtualColumnRemap); |
There was a problem hiding this 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.
| } | ||
| final VirtualColumn equivalent = groupVcs.findEquivalent(queryNode); | ||
| if (equivalent != null && !outputName.equals(equivalent.getOutputName())) { | ||
| candidates.put(outputName, equivalent.getOutputName()); |
There was a problem hiding this 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.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
Reviewed 11 of 11 changed files.
Found 2 issues: residual filter rewriting can fail for unchanged columns, and remapping can target metadata-only virtual columns that group cursors cannot read.
This is an automated review by Codex GPT-5.5
| // the materialized physical column. | ||
| Filter rewritten = spec.getFilter() == null ? null : rewriteFor(group); | ||
| if (rewritten != null) { | ||
| rewritten = rewritten.rewriteRequiredColumns(virtualColumnRemap); |
There was a problem hiding this 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.
| } | ||
| final VirtualColumn equivalent = groupVcs.findEquivalent(queryNode); | ||
| if (equivalent != null && !outputName.equals(equivalent.getOutputName())) { | ||
| candidates.put(outputName, equivalent.getOutputName()); |
There was a problem hiding this 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.
| // 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<>(); |
There was a problem hiding this comment.
naming seems backwards. aren't these actually whare are not being pruned out of the query?
| private static List<String> collectDimension(Cursor cursor, String column) | ||
| { | ||
| final DimensionSelector sel = | ||
| cursor.getColumnSelectorFactory().makeDimensionSelector(DefaultDimensionSpec.of(column)); | ||
| final List<String> out = new ArrayList<>(); | ||
| while (!cursor.isDone()) { | ||
| out.add(sel.getRow().size() == 0 ? null : sel.lookupName(sel.getRow().get(0))); | ||
| cursor.advance(); | ||
| } | ||
| return out; | ||
| } | ||
|
|
||
| private static List<String> collectObjectVector(VectorCursor cursor, String column) | ||
| { | ||
| final VectorObjectSelector sel = cursor.getColumnSelectorFactory().makeObjectSelector(column); | ||
| final List<String> out = new ArrayList<>(); | ||
| while (!cursor.isDone()) { | ||
| final Object[] vector = sel.getObjectVector(); | ||
| final int size = cursor.getCurrentVectorSize(); | ||
| for (int i = 0; i < size; i++) { | ||
| out.add((String) vector[i]); | ||
| } | ||
| cursor.advance(); | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
can we move to top or bottom of file to avoid interleaving with tests
| * 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}. |
There was a problem hiding this comment.
clustering-VC leaves were already folded to TRUE / FALSE by the per-group rewrite walk, so they won't reference the dropped virtual columns
This is a not entirely true. walkClusterGroupFilter only folds Equality, In, and Null. Everything else falls through unchanged. This will result in wrong results when something like a Range filter on a query vc with equivalent clustering column survives the walk.
Repro wrong empty results:
- tenant_lower := lower(tenant) clustered vc
- v0 := lower(tenant) ⇒ remap v0 → tenant_lower
- same test tenants as always
- RangeFilter("v0", ["a","z"])
- Both groups survive and you expect all rows but get empty result back
3 participants
Description
This PR is a minor optimization to clustered segments to allow query virtual columns to be substituted with physical columns if the clustered segment was created with an 'equivalent' column (clustered segments preserve any virtual columns used during their creation in their metadata). For example, if the function
lower(some_column)is used to create the column, after this patch running the same expression at query time will be swapped out to read the value from the column directly instead of recomputing theloweroperation.