Re: [PR] snapshot column capabilities for realtime cursors (druid)

["gianm (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org