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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
