[GitHub] [druid] imply-cheddar commented on a diff in pull request #14053: fix bug in nested v4 format merger from refactoring

["imply-cheddar (via GitHub)" <gi...@apache.org>]

imply-cheddar commented on code in PR #14053:
URL: https://github.com/apache/druid/pull/14053#discussion_r1161483419


##########
processing/src/test/java/org/apache/druid/segment/IndexBuilder.java:
##########
@@ -222,15 +223,31 @@ public QueryableIndex buildMMappedIndex()
     Preconditions.checkNotNull(indexMerger, "indexMerger");
     Preconditions.checkNotNull(tmpDir, "tmpDir");
     try (final IncrementalIndex incrementalIndex = buildIncrementalIndex()) {
+      List<IndexableAdapter> adapters = Collections.singletonList(
+          new QueryableIndexIndexableAdapter(
+              indexIO.loadIndex(
+                  indexMerger.persist(
+                      incrementalIndex,
+                      new File(
+                          tmpDir,
+                          StringUtils.format("testIndex-%s", ThreadLocalRandom.current().nextInt(Integer.MAX_VALUE))
+                      ),
+                      indexSpec,
+                      null
+                  )
+              )
+          )
+      );
+      // still merge it since that follows the normal path of persist then merge

Review Comment:
   nit: "still merge" implies that this comment is referring to a change.  A new reader is not going to know what that change is...  It looks like you are exercising the behavior of what happens when it reads back over the segment to persist a new one?  Perhaps changing this to 
   
   > Do a merge, which will do yet another persist and load again to validate that the behavior of writing and then
   > reading still does good things
   
   This is gonna have performance implications for test run times too, I fear.  But, if we only ever do this once for each data set that we are indexing, it shouldn't be good expensive...



##########
processing/src/main/java/org/apache/druid/segment/QueryableIndexIndexableAdapter.java:
##########
@@ -174,7 +175,9 @@ public NestedColumnMergable getNestedColumnMergeables(String columnName)
     if (columnHolder == null) {
       return null;
     }
-    if (!(columnHolder.getColumnFormat() instanceof NestedCommonFormatColumn.Format)) {
+    final ColumnFormat format = columnHolder.getColumnFormat();
+    if (!(format instanceof NestedCommonFormatColumn.Format)
+        && !(format instanceof NestedDataComplexTypeSerde.NestedColumnFormatV4)) {

Review Comment:
   nit: `!(format instanceof NestedCommonFormatColumn.Format || format instanceof NestedDataComplexTypeSerde.NestedColumnFormatV4)`
   
   is perhaps a bit more easier to understand the intentions of for this.



-- 
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