You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2024/09/02 07:06:54 UTC
[PR] transition away from StorageAdapter (druid)
clintropolis opened a new pull request, #16985:
URL: https://github.com/apache/druid/pull/16985
### Description
Follow-up to finish what #16533 and #16849 started, moving everything completely away from `StorageAdapter`.
changes:
* `CursorHolderFactory` has been renamed to `CursorFactory` and moved off of `StorageAdapter`, instead fetched directly from the segment via `asCursorFactory`. The previous deprecated `CursorFactory` interface has been merged into `StorageAdapter`
* `StorageAdapter` is no longer used by any engines or tests and has been marked as deprecated with default implementations of all methods that throw exceptions indicating the new methods to call instead
* `StorageAdapter` methods not covered by `CursorFactory` (`CursorHolderFactory` prior to this change) have been moved into interfaces which are retrieved by `Segment.as`, the primary classes are the previously existing `Metadata`, as well as new interfaces `PhysicalSegmentInspector` and `TopNOptimizationInspector`
* added `UnnestSegment` and `FilteredSegment` that extend `WrappedSegmentReference` since their `StorageAdapter` implementations were previously provided by `WrappedSegmentReference`
* added `PhysicalSegmentInspector` which covers some of the previous `StorageAdapter` functionality which was primarily used for segment metadata queries and other metadata uses, and is implemented for `QueryableIndexSegment` and `IncrementalIndexSegment`
* added `TopNOptimizationInspector` to cover the oddly specific `StorageAdapter.hasBuiltInFilters` implementation, which is implemented for `HashJoinSegment`, `UnnestSegment`, and `FilteredSegment`
* Updated all engines and tests to no longer use `StorageAdapter`, deleted all `StorageAdapter` implementations
#### Release note
_tbd_
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #16985:
URL: https://github.com/apache/druid/pull/16985
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1742667284
##########
processing/src/main/java/org/apache/druid/segment/SimpleTopNOptimizationInspector.java:
##########
@@ -19,11 +19,18 @@
package org.apache.druid.segment;
-/**
- * @deprecated use {@link Segment} directly as this does nothing
- */
-@Deprecated
-public abstract class AbstractSegment implements Segment
Review Comment:
heh, no idea why git thinks this new file was a rename of this random old file i deleted
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749927835
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -64,25 +73,36 @@ public interface Segment extends Closeable
*
* @return instance of clazz, or null if the interface is not supported by this segment
*
- * @see StorageAdapter storage adapter for queries. Never null.
+ * @see CursorFactory to make cursors to run queries
* @see QueryableIndex index object, if this is a memory-mapped regular segment.
* @see IndexedTable table object, if this is a joinable indexed table.
* @see TimeBoundaryInspector inspector for min/max timestamps, if supported by this segment.
* @see MaxIngestedEventTimeInspector inspector for {@link DataSourceMetadataResultValue#getMaxIngestedEventTime()}
+ * @see PhysicalSegmentInspector inspector for {@link org.apache.druid.query.metadata.SegmentAnalyzer}
+ * @see Metadata information about how a physical segment was created
Review Comment:
moved `Metadata` to `PhysicalSegmentInspector.getMetadata`
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1750689244
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java:
##########
@@ -125,11 +125,11 @@ public <T> T as(Class<T> clazz)
return (T) WrappedTimeBoundaryInspector.create(baseSegment.as(TimeBoundaryInspector.class));
} else if (TopNOptimizationInspector.class.equals(clazz)) {
// if the baseFilter is not null, then rows from underlying cursor can be potentially filtered.
- // otherwise, a filtering inner join can also filter rows.
+ // otherwise, a filtering inner or left join can also filter rows.
return (T) new SimpleTopNOptimizationInspector(
- baseFilter != null || clauses.stream().anyMatch(
- clause -> clause.getJoinType() == JoinType.INNER && !clause.getCondition().isAlwaysTrue()
- )
+ !(baseFilter != null || clauses.stream().anyMatch(
+ clause -> clause.getJoinType().isLefty() && !clause.getCondition().isAlwaysTrue()
Review Comment:
oops, yes, fixed again. I copied your suggestion wrong last time (lost the ! on lefty check) and then flipped it to match the new contract of the inspector thingy, but should have just rewritten it since this is clearer.
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1742671957
##########
processing/src/main/java/org/apache/druid/segment/FilteredSegment.java:
##########
@@ -19,7 +19,38 @@
package org.apache.druid.segment;
-public interface CursorHolderFactory
Review Comment:
hmm, git seems to be confused here too, this is a new file not a rename 😅
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749614560
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java:
##########
@@ -104,16 +105,37 @@ public QueryableIndex asQueryableIndex()
}
@Override
- public StorageAdapter asStorageAdapter()
+ public CursorFactory asCursorFactory()
{
- return new HashJoinSegmentStorageAdapter(
- baseSegment.asStorageAdapter(),
+ return new HashJoinSegmentCursorFactory(
+ baseSegment.asCursorFactory(),
baseFilter,
clauses,
joinFilterPreAnalysis
);
}
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T> T as(Class<T> clazz)
+ {
+ if (CloseableShapeshifter.class.equals(clazz)) {
+ return (T) new CursorFactoryRowsAndColumns(asCursorFactory());
+ } else if (TimeBoundaryInspector.class.equals(clazz)) {
+ return (T) WrappedTimeBoundaryInspector.create(baseSegment.as(TimeBoundaryInspector.class));
+ } else if (TopNOptimizationInspector.class.equals(clazz)) {
+ // if the baseFilter is not null, then rows from underlying cursor can be potentially filtered.
+ // otherwise, a filtering inner join can also filter rows.
+ return (T) new SimpleTopNOptimizationInspector(
+ baseFilter != null || clauses.stream().anyMatch(
+ clause -> clause.getJoinType() == JoinType.INNER && !clause.getCondition().isAlwaysTrue()
Review Comment:
I think this is a pre-existing issue, but, this check should be:
```
!clause.getJoinType().isLefty() && !clause.getCondition().isAlwaysTrue()
```
Because `JoinType.RIGHT` can also filter rows from the base.
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -64,25 +73,36 @@ public interface Segment extends Closeable
*
* @return instance of clazz, or null if the interface is not supported by this segment
*
- * @see StorageAdapter storage adapter for queries. Never null.
+ * @see CursorFactory to make cursors to run queries
Review Comment:
Is this still never null? I would hope so. Useful to say that, I think, if so.
##########
processing/src/main/java/org/apache/druid/query/topn/BaseTopNAlgorithm.java:
##########
@@ -315,10 +316,11 @@ public Pair<Integer, Integer> computeStartEnd(int cardinality)
int endIndex = Math.min(ignoreFirstN + keepOnlyN, cardinality);
+ final TopNOptimizationInspector topNOptimizationInspector = cursorInspector.getFilteredSegmentInspector();
if (ignoreAfterThreshold &&
query.getDimensionsFilter() == null &&
- !storageAdapter.hasBuiltInFilters() &&
- query.getIntervals().stream().anyMatch(interval -> interval.contains(storageAdapter.getInterval()))) {
+ (topNOptimizationInspector == null || !topNOptimizationInspector.isFiltered()) &&
Review Comment:
This design seems brittle— it means if a Segment wraps another Segment and filters it, but forgets to implement `TopNOptimizationInspector`, then it'll be treated as non-filtered and the optimization will be allowed, potentially leading to incorrect results. It would be better to design things such that we avoid the optimization in such cases.
##########
processing/src/main/java/org/apache/druid/query/search/SearchStrategy.java:
##########
@@ -49,10 +55,27 @@ protected SearchStrategy(SearchQuery query)
public abstract List<SearchQueryExecutor> getExecutionPlan(SearchQuery query, Segment segment);
- static List<DimensionSpec> getDimsToSearch(Indexed<String> availableDimensions, List<DimensionSpec> dimensions)
+ static List<DimensionSpec> getDimsToSearch(Segment segment, List<DimensionSpec> dimensions)
{
if (dimensions == null || dimensions.isEmpty()) {
- return ImmutableList.copyOf(Iterables.transform(availableDimensions, Druids.DIMENSION_IDENTITY));
+ final RowSignature rowSignature = segment.asCursorFactory().getRowSignature();
+ final Set<String> dims = new LinkedHashSet<>();
+ final Set<String> ignore = new HashSet<>();
+ ignore.add(ColumnHolder.TIME_COLUMN_NAME);
+ final Metadata metadata = segment.as(Metadata.class);
Review Comment:
This isn't going to work for really old segments, where `availableDimensions` is defined but `Metadata` isn't. It's possible some clusters that have been around for a while still have segments that were written before `Metadata` was a thing.
I suppose the impact is that we'd search all columns, rather than just the dimension columns, in that case? I suppose that's probably fine. Although the situation could be improved by checking explicitly here for `as(QueryableIndex.class)` and using `getAvailableDimensions` if it's present.
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -64,25 +73,36 @@ public interface Segment extends Closeable
*
* @return instance of clazz, or null if the interface is not supported by this segment
*
- * @see StorageAdapter storage adapter for queries. Never null.
+ * @see CursorFactory to make cursors to run queries
* @see QueryableIndex index object, if this is a memory-mapped regular segment.
* @see IndexedTable table object, if this is a joinable indexed table.
* @see TimeBoundaryInspector inspector for min/max timestamps, if supported by this segment.
* @see MaxIngestedEventTimeInspector inspector for {@link DataSourceMetadataResultValue#getMaxIngestedEventTime()}
+ * @see PhysicalSegmentInspector inspector for {@link org.apache.druid.query.metadata.SegmentAnalyzer}
+ * @see Metadata information about how a physical segment was created
Review Comment:
Why is this not on `PhysicalSegmentInspector`?
##########
processing/src/main/java/org/apache/druid/query/datasourcemetadata/DataSourceMetadataQueryRunnerFactory.java:
##########
@@ -85,7 +85,7 @@ private static class DataSourceMetadataQueryRunner implements QueryRunner<Result
public DataSourceMetadataQueryRunner(Segment segment)
{
- this.segmentInterval = segment.asStorageAdapter().getInterval();
+ this.segmentInterval = segment.getDataInterval();
Review Comment:
These _sound_ like they should be different (`interval` vs `dataInterval`). But neither method has javadocs. Were they really equivalent?
##########
processing/src/main/java/org/apache/druid/segment/ColumnSelectorFactory.java:
##########
@@ -41,6 +43,19 @@ public interface ColumnSelectorFactory extends ColumnInspector
*/
ColumnValueSelector makeColumnValueSelector(String columnName);
+ @Override
+ default int getColumnValueCardinality(String columnName)
+ {
+ final ColumnCapabilities capabilities = getColumnCapabilities(columnName);
+ if (capabilities == null) {
+ return DimensionDictionarySelector.CARDINALITY_UNKNOWN;
+ }
+ if (capabilities.is(ValueType.STRING) && capabilities.isDictionaryEncoded().isTrue()) {
+ final DimensionSelector dimensionSelector = makeDimensionSelector(DefaultDimensionSpec.of(columnName));
Review Comment:
Hmm I worry about performance here. The `QueryableIndex` implementation of `makeDimensionSelector` caches selectors, but most others don't.
I wonder if we can remove this method, so we don't have to worry about performance. There's only two call sites:
- One in `GroupingEngine`, where by the time this method is called, dimension selectors have already been created. The `GroupingEngine` could check the dimension selectors directly.
- One in `TopNQueryEngine`, where this method is called before the dimension selector is created, but it probably could be restructured...
##########
processing/src/main/java/org/apache/druid/segment/ColumnCardinalityInspector.java:
##########
@@ -19,7 +19,14 @@
package org.apache.druid.segment;
-public interface CursorHolderFactory
+/**
+ * {@link ColumnInspector} + value cardinality. This can probably be merged into {@link ColumnInspector} at some point
+ * in the future.
+ */
+public interface ColumnCardinalityInspector extends ColumnInspector
{
- CursorHolder makeCursorHolder(CursorBuildSpec spec);
+ default int getColumnValueCardinality(String column)
Review Comment:
Should have javadocs at least describing what scenarios can result in `DimensionDictionarySelector.CARDINALITY_UNKNOWN`.
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1740468884
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -44,18 +52,114 @@
@Nullable
QueryableIndex asQueryableIndex();
- StorageAdapter asStorageAdapter();
+ @Deprecated
+ default StorageAdapter asStorageAdapter()
+ {
+ throw DruidException.defensive(
+ "asStorageAdapter is no longer supported, use Segment.asCursorFactory to build cursors, or Segment.as(..) to get various metadata information instead"
+ );
+ }
+
+ default CursorFactory asCursorFactory()
+ {
+ // todo (clint): should we just tear off the bandaid and make everyone implement this?
+ final StorageAdapter storageAdapter = asStorageAdapter();
Review Comment:
## Deprecated method or constructor invocation
Invoking [Segment.asStorageAdapter](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7768)
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -44,18 +52,114 @@
@Nullable
QueryableIndex asQueryableIndex();
- StorageAdapter asStorageAdapter();
+ @Deprecated
+ default StorageAdapter asStorageAdapter()
+ {
+ throw DruidException.defensive(
+ "asStorageAdapter is no longer supported, use Segment.asCursorFactory to build cursors, or Segment.as(..) to get various metadata information instead"
+ );
+ }
+
+ default CursorFactory asCursorFactory()
+ {
+ // todo (clint): should we just tear off the bandaid and make everyone implement this?
+ final StorageAdapter storageAdapter = asStorageAdapter();
+ return new CursorFactory()
+ {
+ @Override
+ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
+ {
+ // For backwards compatibility, the default implementation assumes the underlying rows are sorted by __time.
+ // Built-in implementations of StorageAdapter must override this method.
+ final List<OrderBy> ordering;
+ final boolean descending;
+ if (Cursors.preferDescendingTimeOrdering(spec)) {
+ ordering = Cursors.descendingTimeOrder();
+ descending = true;
+ } else {
+ ordering = Cursors.ascendingTimeOrder();
+ descending = false;
+ }
+ return new CursorHolder()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return storageAdapter.canVectorize(
+ spec.getFilter(),
+ spec.getVirtualColumns(),
+ descending
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [StorageAdapter.canVectorize](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7769)
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -44,18 +52,114 @@
@Nullable
QueryableIndex asQueryableIndex();
- StorageAdapter asStorageAdapter();
+ @Deprecated
+ default StorageAdapter asStorageAdapter()
+ {
+ throw DruidException.defensive(
+ "asStorageAdapter is no longer supported, use Segment.asCursorFactory to build cursors, or Segment.as(..) to get various metadata information instead"
+ );
+ }
+
+ default CursorFactory asCursorFactory()
+ {
+ // todo (clint): should we just tear off the bandaid and make everyone implement this?
+ final StorageAdapter storageAdapter = asStorageAdapter();
+ return new CursorFactory()
+ {
+ @Override
+ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
+ {
+ // For backwards compatibility, the default implementation assumes the underlying rows are sorted by __time.
+ // Built-in implementations of StorageAdapter must override this method.
+ final List<OrderBy> ordering;
+ final boolean descending;
+ if (Cursors.preferDescendingTimeOrdering(spec)) {
+ ordering = Cursors.descendingTimeOrder();
+ descending = true;
+ } else {
+ ordering = Cursors.ascendingTimeOrder();
+ descending = false;
+ }
+ return new CursorHolder()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return storageAdapter.canVectorize(
+ spec.getFilter(),
+ spec.getVirtualColumns(),
+ descending
+ );
+ }
+
+ @Override
+ public Cursor asCursor()
+ {
+ return Iterables.getOnlyElement(
+ storageAdapter.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ descending,
+ spec.getQueryMetrics()
+ ).toList()
+ );
+ }
+
+ @Override
+ public VectorCursor asVectorCursor()
+ {
+ return storageAdapter.makeVectorCursor(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ descending,
+ spec.getQueryContext().getVectorSize(),
+ spec.getQueryMetrics()
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [StorageAdapter.makeVectorCursor](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7771)
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -44,18 +52,114 @@
@Nullable
QueryableIndex asQueryableIndex();
- StorageAdapter asStorageAdapter();
+ @Deprecated
+ default StorageAdapter asStorageAdapter()
+ {
+ throw DruidException.defensive(
+ "asStorageAdapter is no longer supported, use Segment.asCursorFactory to build cursors, or Segment.as(..) to get various metadata information instead"
+ );
+ }
+
+ default CursorFactory asCursorFactory()
+ {
+ // todo (clint): should we just tear off the bandaid and make everyone implement this?
+ final StorageAdapter storageAdapter = asStorageAdapter();
+ return new CursorFactory()
+ {
+ @Override
+ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
+ {
+ // For backwards compatibility, the default implementation assumes the underlying rows are sorted by __time.
+ // Built-in implementations of StorageAdapter must override this method.
+ final List<OrderBy> ordering;
+ final boolean descending;
+ if (Cursors.preferDescendingTimeOrdering(spec)) {
+ ordering = Cursors.descendingTimeOrder();
+ descending = true;
+ } else {
+ ordering = Cursors.ascendingTimeOrder();
+ descending = false;
+ }
+ return new CursorHolder()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return storageAdapter.canVectorize(
+ spec.getFilter(),
+ spec.getVirtualColumns(),
+ descending
+ );
+ }
+
+ @Override
+ public Cursor asCursor()
+ {
+ return Iterables.getOnlyElement(
+ storageAdapter.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ descending,
+ spec.getQueryMetrics()
+ ).toList()
+ );
+ }
+
+ @Override
+ public VectorCursor asVectorCursor()
+ {
+ return storageAdapter.makeVectorCursor(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ descending,
+ spec.getQueryContext().getVectorSize(),
+ spec.getQueryMetrics()
+ );
+ }
+
+ @Nullable
+ @Override
+ public List<OrderBy> getOrdering()
+ {
+ return ordering;
+ }
+
+ @Override
+ public void close()
+ {
+ // consuming sequences of CursorFactory are expected to close themselves.
+ }
+ };
+ }
+
+ @Override
+ public RowSignature getRowSignature()
+ {
+ return storageAdapter.getRowSignature();
Review Comment:
## Deprecated method or constructor invocation
Invoking [StorageAdapter.getRowSignature](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7772)
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -44,18 +52,114 @@
@Nullable
QueryableIndex asQueryableIndex();
- StorageAdapter asStorageAdapter();
+ @Deprecated
+ default StorageAdapter asStorageAdapter()
+ {
+ throw DruidException.defensive(
+ "asStorageAdapter is no longer supported, use Segment.asCursorFactory to build cursors, or Segment.as(..) to get various metadata information instead"
+ );
+ }
+
+ default CursorFactory asCursorFactory()
+ {
+ // todo (clint): should we just tear off the bandaid and make everyone implement this?
+ final StorageAdapter storageAdapter = asStorageAdapter();
+ return new CursorFactory()
+ {
+ @Override
+ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
+ {
+ // For backwards compatibility, the default implementation assumes the underlying rows are sorted by __time.
+ // Built-in implementations of StorageAdapter must override this method.
+ final List<OrderBy> ordering;
+ final boolean descending;
+ if (Cursors.preferDescendingTimeOrdering(spec)) {
+ ordering = Cursors.descendingTimeOrder();
+ descending = true;
+ } else {
+ ordering = Cursors.ascendingTimeOrder();
+ descending = false;
+ }
+ return new CursorHolder()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return storageAdapter.canVectorize(
+ spec.getFilter(),
+ spec.getVirtualColumns(),
+ descending
+ );
+ }
+
+ @Override
+ public Cursor asCursor()
+ {
+ return Iterables.getOnlyElement(
+ storageAdapter.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ descending,
+ spec.getQueryMetrics()
+ ).toList()
Review Comment:
## Deprecated method or constructor invocation
Invoking [StorageAdapter.makeCursors](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7770)
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -44,18 +52,114 @@
@Nullable
QueryableIndex asQueryableIndex();
- StorageAdapter asStorageAdapter();
+ @Deprecated
+ default StorageAdapter asStorageAdapter()
+ {
+ throw DruidException.defensive(
+ "asStorageAdapter is no longer supported, use Segment.asCursorFactory to build cursors, or Segment.as(..) to get various metadata information instead"
+ );
+ }
+
+ default CursorFactory asCursorFactory()
+ {
+ // todo (clint): should we just tear off the bandaid and make everyone implement this?
+ final StorageAdapter storageAdapter = asStorageAdapter();
+ return new CursorFactory()
+ {
+ @Override
+ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
+ {
+ // For backwards compatibility, the default implementation assumes the underlying rows are sorted by __time.
+ // Built-in implementations of StorageAdapter must override this method.
+ final List<OrderBy> ordering;
+ final boolean descending;
+ if (Cursors.preferDescendingTimeOrdering(spec)) {
+ ordering = Cursors.descendingTimeOrder();
+ descending = true;
+ } else {
+ ordering = Cursors.ascendingTimeOrder();
+ descending = false;
+ }
+ return new CursorHolder()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return storageAdapter.canVectorize(
+ spec.getFilter(),
+ spec.getVirtualColumns(),
+ descending
+ );
+ }
+
+ @Override
+ public Cursor asCursor()
+ {
+ return Iterables.getOnlyElement(
+ storageAdapter.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ descending,
+ spec.getQueryMetrics()
+ ).toList()
+ );
+ }
+
+ @Override
+ public VectorCursor asVectorCursor()
+ {
+ return storageAdapter.makeVectorCursor(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ descending,
+ spec.getQueryContext().getVectorSize(),
+ spec.getQueryMetrics()
+ );
+ }
+
+ @Nullable
+ @Override
+ public List<OrderBy> getOrdering()
+ {
+ return ordering;
+ }
+
+ @Override
+ public void close()
+ {
+ // consuming sequences of CursorFactory are expected to close themselves.
+ }
+ };
+ }
+
+ @Override
+ public RowSignature getRowSignature()
+ {
+ return storageAdapter.getRowSignature();
+ }
+
+ @Override
+ @Nullable
+ public ColumnCapabilities getColumnCapabilities(String column)
+ {
+ return storageAdapter.getColumnCapabilities(column);
Review Comment:
## Deprecated method or constructor invocation
Invoking [StorageAdapter.getColumnCapabilities](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7773)
##########
processing/src/test/java/org/apache/druid/frame/processor/FrameProcessorExecutorTest.java:
##########
@@ -144,14 +145,14 @@
final ListenableFuture<Long> blasterFuture = exec.runFully(blaster, null);
final ListenableFuture<Long> muxerFuture = exec.runFully(muxer, null);
- Assert.assertEquals(adapter.getNumRows(), (long) blasterFuture.get());
- Assert.assertEquals(adapter.getNumRows() * 2, (long) muxerFuture.get());
+ Assert.assertEquals(index.size(), (long) blasterFuture.get());
+ Assert.assertEquals(index.size() * 2, (long) muxerFuture.get());
Assert.assertEquals(
- adapter.getNumRows() * 2,
+ index.size() * 2,
Review Comment:
## Result of multiplication cast to wider type
Potential overflow in [int multiplication](1) before it is converted to long by use in an invocation context.
[Show more details](https://github.com/apache/druid/security/code-scanning/7766)
##########
processing/src/test/java/org/apache/druid/frame/processor/FrameProcessorExecutorTest.java:
##########
@@ -144,14 +145,14 @@
final ListenableFuture<Long> blasterFuture = exec.runFully(blaster, null);
final ListenableFuture<Long> muxerFuture = exec.runFully(muxer, null);
- Assert.assertEquals(adapter.getNumRows(), (long) blasterFuture.get());
- Assert.assertEquals(adapter.getNumRows() * 2, (long) muxerFuture.get());
+ Assert.assertEquals(index.size(), (long) blasterFuture.get());
+ Assert.assertEquals(index.size() * 2, (long) muxerFuture.get());
Review Comment:
## Result of multiplication cast to wider type
Potential overflow in [int multiplication](1) before it is converted to long by use in an invocation context.
[Show more details](https://github.com/apache/druid/security/code-scanning/7765)
##########
processing/src/test/java/org/apache/druid/query/lookup/LookupSegmentTest.java:
##########
@@ -234,8 +190,8 @@
@Test
public void test_asStorageAdapter_isRowBasedAdapter()
{
- // This allows us to assume that RowBasedStorageAdapterTest is further exercising makeCursor and verifying misc.
+ // This allows us to assume that LookupSegmentTest is further exercising makeCursor and verifying misc.
// methods like getMinTime, getMaxTime, getMetadata, etc, without checking them explicitly in _this_ test class.
- Assert.assertThat(LOOKUP_SEGMENT.asStorageAdapter(), CoreMatchers.instanceOf(RowBasedStorageAdapter.class));
+ Assert.assertThat(LOOKUP_SEGMENT.asCursorFactory(), CoreMatchers.instanceOf(RowBasedCursorFactory.class));
Review Comment:
## Deprecated method or constructor invocation
Invoking [Assert.assertThat](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7776)
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749742493
##########
processing/src/main/java/org/apache/druid/query/datasourcemetadata/DataSourceMetadataQueryRunnerFactory.java:
##########
@@ -85,7 +85,7 @@ private static class DataSourceMetadataQueryRunner implements QueryRunner<Result
public DataSourceMetadataQueryRunner(Segment segment)
{
- this.segmentInterval = segment.asStorageAdapter().getInterval();
+ this.segmentInterval = segment.getDataInterval();
Review Comment:
they were equivalent as far as I could tell.
* `IncrementalIndexSegment` and `IncrementalIndexStorageAdapter` both returned `incrementalIndex.getDataInterval`
* `QueryableIndexSegment` and `QueryableIndexStorageAdapter` both returned `QueryableIndex.getDataInterval`
* `HashJoinSegment` returns base segment data interval, `HashJoinStorageAdapter` returns base adapter interval
* `FrameSegment` only uses segmentId with eternity interval, `FrameStorageAdapter` only used eternity interval
* `RowBasedSegment` uses `RowBasedStorageAdapter` interval
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749750883
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -64,25 +73,36 @@ public interface Segment extends Closeable
*
* @return instance of clazz, or null if the interface is not supported by this segment
*
- * @see StorageAdapter storage adapter for queries. Never null.
+ * @see CursorFactory to make cursors to run queries
Review Comment:
yea, though like, in the same way it was true for `StorageAdapter`. It can actually be null in practice because we got the storage adapter through `ReferenceCountingSegment`, which returns `null` for a lot of its methods if the backing segment has been closed due to being dropped, so the javadoc was kind of lies.
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749925583
##########
processing/src/main/java/org/apache/druid/segment/ColumnCardinalityInspector.java:
##########
@@ -19,7 +19,14 @@
package org.apache.druid.segment;
-public interface CursorHolderFactory
+/**
+ * {@link ColumnInspector} + value cardinality. This can probably be merged into {@link ColumnInspector} at some point
+ * in the future.
+ */
+public interface ColumnCardinalityInspector extends ColumnInspector
{
- CursorHolder makeCursorHolder(CursorBuildSpec spec);
+ default int getColumnValueCardinality(String column)
Review Comment:
this is removed instead, though did javadoc the similar method in the `GroupingSelector` that somewhat replaced this interface
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1750497106
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java:
##########
@@ -125,11 +125,11 @@ public <T> T as(Class<T> clazz)
return (T) WrappedTimeBoundaryInspector.create(baseSegment.as(TimeBoundaryInspector.class));
} else if (TopNOptimizationInspector.class.equals(clazz)) {
// if the baseFilter is not null, then rows from underlying cursor can be potentially filtered.
- // otherwise, a filtering inner join can also filter rows.
+ // otherwise, a filtering inner or left join can also filter rows.
return (T) new SimpleTopNOptimizationInspector(
- baseFilter != null || clauses.stream().anyMatch(
- clause -> clause.getJoinType() == JoinType.INNER && !clause.getCondition().isAlwaysTrue()
- )
+ !(baseFilter != null || clauses.stream().anyMatch(
+ clause -> clause.getJoinType().isLefty() && !clause.getCondition().isAlwaysTrue()
Review Comment:
I think this isn't correct. The condition we want is that the `baseFilter` is null, and all clauses are either lefty or always-true. Like this:
```
baseFilter == null && clauses.stream.allMatch(
clause -> clause.getJoinType().isLefty() || clause.getCondition().isAlwaysTrue()
)
```
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749746440
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -64,25 +73,36 @@ public interface Segment extends Closeable
*
* @return instance of clazz, or null if the interface is not supported by this segment
*
- * @see StorageAdapter storage adapter for queries. Never null.
+ * @see CursorFactory to make cursors to run queries
* @see QueryableIndex index object, if this is a memory-mapped regular segment.
* @see IndexedTable table object, if this is a joinable indexed table.
* @see TimeBoundaryInspector inspector for min/max timestamps, if supported by this segment.
* @see MaxIngestedEventTimeInspector inspector for {@link DataSourceMetadataResultValue#getMaxIngestedEventTime()}
+ * @see PhysicalSegmentInspector inspector for {@link org.apache.druid.query.metadata.SegmentAnalyzer}
+ * @see Metadata information about how a physical segment was created
Review Comment:
i guess it could be, originally i was calling `PhysicalSegmentInspector` `SegmentAnalysisInspector` because it was only used by segment metadata query, so i was avoiding putting stuff that might be used elsewhere here, but numRows is used by the `SegmentManager` to track load/drop row count so it got a more generic name, i suppose I should move it into there
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749927349
##########
processing/src/main/java/org/apache/druid/query/search/SearchStrategy.java:
##########
@@ -49,10 +55,27 @@ protected SearchStrategy(SearchQuery query)
public abstract List<SearchQueryExecutor> getExecutionPlan(SearchQuery query, Segment segment);
- static List<DimensionSpec> getDimsToSearch(Indexed<String> availableDimensions, List<DimensionSpec> dimensions)
+ static List<DimensionSpec> getDimsToSearch(Segment segment, List<DimensionSpec> dimensions)
{
if (dimensions == null || dimensions.isEmpty()) {
- return ImmutableList.copyOf(Iterables.transform(availableDimensions, Druids.DIMENSION_IDENTITY));
+ final RowSignature rowSignature = segment.asCursorFactory().getRowSignature();
+ final Set<String> dims = new LinkedHashSet<>();
+ final Set<String> ignore = new HashSet<>();
+ ignore.add(ColumnHolder.TIME_COLUMN_NAME);
+ final Metadata metadata = segment.as(Metadata.class);
Review Comment:
switched this to prefer `QueryableIndex` and fallback to physical inspector/metadata if not available.
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749924625
##########
processing/src/main/java/org/apache/druid/segment/ColumnSelectorFactory.java:
##########
@@ -41,6 +43,19 @@ public interface ColumnSelectorFactory extends ColumnInspector
*/
ColumnValueSelector makeColumnValueSelector(String columnName);
+ @Override
+ default int getColumnValueCardinality(String columnName)
+ {
+ final ColumnCapabilities capabilities = getColumnCapabilities(columnName);
+ if (capabilities == null) {
+ return DimensionDictionarySelector.CARDINALITY_UNKNOWN;
+ }
+ if (capabilities.is(ValueType.STRING) && capabilities.isDictionaryEncoded().isTrue()) {
+ final DimensionSelector dimensionSelector = makeDimensionSelector(DefaultDimensionSpec.of(columnName));
Review Comment:
ok did this, I made a `GroupingSelector` interface for `GroupByColumnSelectorPlus` and `GroupByVectorColumnSelector` to implement for grouping engine cardinality computation, and reworked topn to make its selectorplus outside of the run method (which is nicer too since it doesnt make a new one for every granularity bucket, which was unnecessary after the previous refactor)
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749742493
##########
processing/src/main/java/org/apache/druid/query/datasourcemetadata/DataSourceMetadataQueryRunnerFactory.java:
##########
@@ -85,7 +85,7 @@ private static class DataSourceMetadataQueryRunner implements QueryRunner<Result
public DataSourceMetadataQueryRunner(Segment segment)
{
- this.segmentInterval = segment.asStorageAdapter().getInterval();
+ this.segmentInterval = segment.getDataInterval();
Review Comment:
they were equivalent as far as I could tell.
* `IncrementalIndexSegment` and `IncrementalIndexStorageAdapter` both returned `incrementalIndex.getDataInterval`
* `QueryableIndexSegment` and `QueryableIndexStorageAdapter` both returned `QueryableIndex.getDataInterval`
* `HashJoinSegment` returns base segment data interval, `HashJoinStorageAdapter` returns base segment interval
* `FrameSegment` only uses segmentId with eternity interval, `FrameStorageAdapter` only used eternity interval
* `RowBasedSegment` uses `RowBasedStorageAdapter` interval
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749750883
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -64,25 +73,36 @@ public interface Segment extends Closeable
*
* @return instance of clazz, or null if the interface is not supported by this segment
*
- * @see StorageAdapter storage adapter for queries. Never null.
+ * @see CursorFactory to make cursors to run queries
Review Comment:
yea, though like, in the same way it was true for `StorageAdapter`. It could actually be null in practice because we got the storage adapter through `ReferenceCountingSegment`, which returns `null` for a lot of its methods if the backing segment has been closed due to being dropped, so the javadoc was kind of lies.
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749926538
##########
processing/src/main/java/org/apache/druid/query/topn/BaseTopNAlgorithm.java:
##########
@@ -315,10 +316,11 @@ public Pair<Integer, Integer> computeStartEnd(int cardinality)
int endIndex = Math.min(ignoreFirstN + keepOnlyN, cardinality);
+ final TopNOptimizationInspector topNOptimizationInspector = cursorInspector.getFilteredSegmentInspector();
if (ignoreAfterThreshold &&
query.getDimensionsFilter() == null &&
- !storageAdapter.hasBuiltInFilters() &&
- query.getIntervals().stream().anyMatch(interval -> interval.contains(storageAdapter.getInterval()))) {
+ (topNOptimizationInspector == null || !topNOptimizationInspector.isFiltered()) &&
Review Comment:
adjusted and renamed `isFiltered` to `areAllDictionaryIdsPresent` to better reflect usage
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java:
##########
@@ -104,16 +105,37 @@ public QueryableIndex asQueryableIndex()
}
@Override
- public StorageAdapter asStorageAdapter()
+ public CursorFactory asCursorFactory()
{
- return new HashJoinSegmentStorageAdapter(
- baseSegment.asStorageAdapter(),
+ return new HashJoinSegmentCursorFactory(
+ baseSegment.asCursorFactory(),
baseFilter,
clauses,
joinFilterPreAnalysis
);
}
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T> T as(Class<T> clazz)
+ {
+ if (CloseableShapeshifter.class.equals(clazz)) {
+ return (T) new CursorFactoryRowsAndColumns(asCursorFactory());
+ } else if (TimeBoundaryInspector.class.equals(clazz)) {
+ return (T) WrappedTimeBoundaryInspector.create(baseSegment.as(TimeBoundaryInspector.class));
+ } else if (TopNOptimizationInspector.class.equals(clazz)) {
+ // if the baseFilter is not null, then rows from underlying cursor can be potentially filtered.
+ // otherwise, a filtering inner join can also filter rows.
+ return (T) new SimpleTopNOptimizationInspector(
+ baseFilter != null || clauses.stream().anyMatch(
+ clause -> clause.getJoinType() == JoinType.INNER && !clause.getCondition().isAlwaysTrue()
Review Comment:
fixed
--
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
Re: [PR] transition away from StorageAdapter (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16985:
URL: https://github.com/apache/druid/pull/16985#discussion_r1749929778
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -64,25 +73,36 @@ public interface Segment extends Closeable
*
* @return instance of clazz, or null if the interface is not supported by this segment
*
- * @see StorageAdapter storage adapter for queries. Never null.
+ * @see CursorFactory to make cursors to run queries
Review Comment:
updated to specify never null. Did not clarify about `ReferenceCountingSegment` because its kind of true for almost every method on `Segment` and wasn't quite sure how to best describe it.
--
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