gianm commented on a change in pull request #9731: URL: https://github.com/apache/druid/pull/9731#discussion_r428840570
########## File path: processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapter.java ########## @@ -142,24 +141,8 @@ public Comparable getMaxValue(String column) @Override public ColumnCapabilities getColumnCapabilities(String column) { - // Different from index.getCapabilities because, in a way, IncrementalIndex's string-typed dimensions - // are always potentially multi-valued at query time. (Missing / null values for a row can potentially be - // represented by an empty array; see StringDimensionIndexer.IndexerDimensionSelector's getRow method.) - // - // We don't want to represent this as having-multiple-values in index.getCapabilities, because that's used - // at index-persisting time to determine if we need a multi-value column or not. However, that means we - // need to tweak the capabilities here in the StorageAdapter (a query-time construct), so at query time - // they appear multi-valued. - - final ColumnCapabilities capabilitiesFromIndex = index.getCapabilities(column); - final IncrementalIndex.DimensionDesc dimensionDesc = index.getDimension(column); - if (dimensionDesc != null && dimensionDesc.getCapabilities().getType() == ValueType.STRING) { - final ColumnCapabilitiesImpl retVal = ColumnCapabilitiesImpl.copyOf(capabilitiesFromIndex); - retVal.setHasMultipleValues(true); - return retVal; - } else { - return capabilitiesFromIndex; - } + // snapshot the current state + return ColumnCapabilitiesImpl.complete(index.getCapabilities(column), false); Review comment: Yes, that's what I was thinking: HMV could be set at any time, including between cap-fetch and selector-creation. By the way, it looks like StringDimensionIndexer's handling of `hasMultipleValues` is not thread-safe (it's updated by the indexing thread and read by querying threads without locking). ########## File path: processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapter.java ########## @@ -142,24 +141,8 @@ public Comparable getMaxValue(String column) @Override public ColumnCapabilities getColumnCapabilities(String column) { - // Different from index.getCapabilities because, in a way, IncrementalIndex's string-typed dimensions - // are always potentially multi-valued at query time. (Missing / null values for a row can potentially be - // represented by an empty array; see StringDimensionIndexer.IndexerDimensionSelector's getRow method.) - // - // We don't want to represent this as having-multiple-values in index.getCapabilities, because that's used - // at index-persisting time to determine if we need a multi-value column or not. However, that means we - // need to tweak the capabilities here in the StorageAdapter (a query-time construct), so at query time - // they appear multi-valued. - - final ColumnCapabilities capabilitiesFromIndex = index.getCapabilities(column); - final IncrementalIndex.DimensionDesc dimensionDesc = index.getDimension(column); - if (dimensionDesc != null && dimensionDesc.getCapabilities().getType() == ValueType.STRING) { - final ColumnCapabilitiesImpl retVal = ColumnCapabilitiesImpl.copyOf(capabilitiesFromIndex); - retVal.setHasMultipleValues(true); - return retVal; - } else { - return capabilitiesFromIndex; - } + // snapshot the current state + return ColumnCapabilitiesImpl.complete(index.getCapabilities(column), false); Review comment: Yes, that's what I was thinking: HMV could be set at any time, including between cap-fetch and selector-creation. By the way, it looks like StringDimensionIndexer's handling of `hasMultipleValues` is not thread-safe (it's updated by the indexing thread and read by querying threads without locking). It'd be good to fix that too. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org