gianm commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1866259222
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -256,13 +275,11 @@ public CursorBuildSpecBuilder setInterval(Interval
interval)
}
/**
- * @see CursorBuildSpec#getGroupingColumns()
+ * @see CursorBuildSpec#getPhysicalColumns()
*/
- public CursorBuildSpecBuilder setGroupingColumns(
- @Nullable List<String> groupingColumns
- )
+ public CursorBuildSpecBuilder setPhyiscalColumns(@Nullable Set<String>
physicalColumns)
Review Comment:
Physical (spelling)
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentCursorFactory.java:
##########
@@ -149,6 +166,27 @@ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
);
cursorBuildSpecBuilder.setVirtualColumns(preJoinVirtualColumns);
+ // add all physical columns columns if they were originally set
+ if (physicalColumns != null) {
Review Comment:
Do we need the condition columns from `clauses` too?
##########
processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexColumnSelectorFactory.java:
##########
@@ -51,21 +54,44 @@ class IncrementalIndexColumnSelectorFactory implements
ColumnSelectorFactory, Ro
IncrementalIndexColumnSelectorFactory(
IncrementalIndexRowSelector rowSelector,
IncrementalIndexRowHolder rowHolder,
- VirtualColumns virtualColumns,
+ CursorBuildSpec cursorBuildSpec,
Order timeOrder
)
{
this.rowSelector = rowSelector;
- this.virtualColumns = virtualColumns;
+ this.virtualColumns = cursorBuildSpec.getVirtualColumns();
this.timeOrder = timeOrder;
this.rowHolder = rowHolder;
+
+ final Map<String, ColumnCapabilities> capabilitiesMap = new HashMap<>();
+ if (cursorBuildSpec.getPhysicalColumns() == null) {
+ // add everything
+ for (String column : rowSelector.getDimensionNames(true)) {
+ capabilitiesMap.put(column,
IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ for (String column : rowSelector.getMetricNames()) {
+ capabilitiesMap.put(column,
IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ for (String column :
cursorBuildSpec.getVirtualColumns().getColumnNames()) {
+ capabilitiesMap.put(column,
IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ } else {
+ // just add required columns
+ for (String column : cursorBuildSpec.getPhysicalColumns()) {
+ capabilitiesMap.put(column,
IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ // and virtual columns
+ for (String column :
cursorBuildSpec.getVirtualColumns().getColumnNames()) {
+ capabilitiesMap.put(column,
IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ }
this.snapshotColumnInspector = new ColumnInspector()
{
@Nullable
@Override
public ColumnCapabilities getColumnCapabilities(String column)
{
- return
IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column);
+ return capabilitiesMap.get(column);
Review Comment:
I wonder if we should have a defensive check for calling
`getColumnCapabilities` on a `column` that wasn't part of `physicalColumns`. At
least one that is activated during tests, even if it's not activated during
production.
--
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]
