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: [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]
