You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org> on 2024/07/26 20:08:34 UTC
Re: [PR] rework cursor creation (druid)
github-advanced-security[bot] commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1693541054
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/LazilyDecoratedRowsAndColumns.java:
##########
@@ -280,26 +279,24 @@
sortColumns
);
- final FrameWriter frameWriter = frameWriterFactory.newFrameWriter(columnSelectorFactory);
- for (; !in.isDoneOrInterrupted() && remainingRowsToSkip > 0; remainingRowsToSkip--) {
- in.advance();
+ final FrameWriter writer = frameWriterFactory.newFrameWriter(columnSelectorFactory);
+ for (; !cursor.isDoneOrInterrupted() && remainingRowsToSkip > 0; remainingRowsToSkip--) {
+ cursor.advance();
}
- for (; !in.isDoneOrInterrupted() && remainingRowsToFetch > 0; remainingRowsToFetch--) {
- frameWriter.addSelection();
- in.advance();
+ for (; !cursor.isDoneOrInterrupted() && remainingRowsToFetch > 0; remainingRowsToFetch--) {
+ writer.addSelection();
Review Comment:
## Dereferenced variable may be null
Variable [writer](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/7605)
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/StorageAdapterRowsAndColumns.java:
##########
@@ -98,45 +95,35 @@
@Nonnull
private static RowsAndColumns materialize(StorageAdapter as)
{
- final Sequence<Cursor> cursors = as.makeCursors(
- null,
- Intervals.ETERNITY,
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
-
- RowSignature rowSignature = as.getRowSignature();
-
- FrameWriter writer = cursors.accumulate(null, (accumulated, in) -> {
- if (accumulated != null) {
- // We should not get multiple cursors because we set the granularity to ALL. So, this should never
- // actually happen, but it doesn't hurt us to defensive here, so we test against it.
- throw new ISE("accumulated[%s] non-null, why did we get multiple cursors?", accumulated);
+ try (final CursorMaker maker = as.asCursorMaker(CursorBuildSpec.FULL_SCAN)) {
+ final Cursor cursor = maker.makeCursor();
+
+ if (cursor == null) {
+ return new EmptyRowsAndColumns();
}
+
+ final RowSignature rowSignature = as.getRowSignature();
- final ColumnSelectorFactory columnSelectorFactory = in.getColumnSelectorFactory();
+ final ColumnSelectorFactory columnSelectorFactory = cursor.getColumnSelectorFactory();
final FrameWriterFactory frameWriterFactory = FrameWriters.makeColumnBasedFrameWriterFactory(
new ArenaMemoryAllocatorFactory(200 << 20), // 200 MB, because, why not?
rowSignature,
Collections.emptyList()
);
- final FrameWriter frameWriter = frameWriterFactory.newFrameWriter(columnSelectorFactory);
- while (!in.isDoneOrInterrupted()) {
- frameWriter.addSelection();
- in.advance();
+ final FrameWriter writer = frameWriterFactory.newFrameWriter(columnSelectorFactory);
+ while (!cursor.isDoneOrInterrupted()) {
+ writer.addSelection();
Review Comment:
## Dereferenced variable may be null
Variable [writer](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/7606)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
+ case LONG:
nulls = selector.getNullVector();
- float[] floats = selector.getFloatVector();
+ long[] longs = selector.getLongVector();
for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
}
- } else {
- nulls = selector.getNullVector();
- double[] doubles = selector.getDoubleVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ break;
+ case DOUBLE:
+ // special case to test floats just to get coverage on getFloatVector
+ if ("float2".equals(expression)) {
+ nulls = selector.getNullVector();
Review Comment:
## Dereferenced variable may be null
Variable [selector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/7609)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
+ case LONG:
nulls = selector.getNullVector();
- float[] floats = selector.getFloatVector();
+ long[] longs = selector.getLongVector();
for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
}
- } else {
- nulls = selector.getNullVector();
- double[] doubles = selector.getDoubleVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ break;
+ case DOUBLE:
+ // special case to test floats just to get coverage on getFloatVector
+ if ("float2".equals(expression)) {
+ nulls = selector.getNullVector();
+ float[] floats = selector.getFloatVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ }
+ } else {
+ nulls = selector.getNullVector();
Review Comment:
## Dereferenced variable may be null
Variable [selector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/7610)
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -33,34 +36,106 @@
*
* @see StorageAdapter
*/
-public interface CursorFactory
+public interface CursorFactory extends CursorMakerFactory
{
+ @Override
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Cursor makeCursor()
+ {
+ return Iterables.getOnlyElement(
+ CursorFactory.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ ).toList()
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.makeCursors](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7613)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
+ case LONG:
nulls = selector.getNullVector();
- float[] floats = selector.getFloatVector();
+ long[] longs = selector.getLongVector();
for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
}
- } else {
- nulls = selector.getNullVector();
- double[] doubles = selector.getDoubleVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ break;
+ case DOUBLE:
+ // special case to test floats just to get coverage on getFloatVector
+ if ("float2".equals(expression)) {
+ nulls = selector.getNullVector();
+ float[] floats = selector.getFloatVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ }
+ } else {
+ nulls = selector.getNullVector();
+ double[] doubles = selector.getDoubleVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ }
}
- }
- break;
- case STRING:
- Object[] objects = objectSelector.getObjectVector();
- for (int i = 0; i < objectSelector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(objects[i]);
- }
- break;
- }
+ break;
+ case STRING:
+ Object[] objects = objectSelector.getObjectVector();
Review Comment:
## Dereferenced variable may be null
Variable [objectSelector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/7607)
##########
processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapterTest.java:
##########
@@ -433,48 +431,42 @@
final StorageAdapter sa = new IncrementalIndexStorageAdapter(index);
- Sequence<Cursor> cursors = sa.makeCursors(
- null,
- Intervals.utc(timestamp - 60_000, timestamp + 60_000),
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
- final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
-
- cursors
- .map(cursor -> {
- DimensionSelector dimSelector = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- int cardinality = dimSelector.getValueCardinality();
-
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- for (int i = 0; i < 1; i++) {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v2" + i)));
- }
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- int rowNumInCursor = 0;
- // and then, cursoring continues in the other thread
- while (!cursor.isDone()) {
- IndexedInts row = dimSelector.getRow();
- row.forEach(i -> Assert.assertTrue(i < cardinality));
- cursor.advance();
- rowNumInCursor++;
- }
- Assert.assertEquals(2, rowNumInCursor);
- assertCursorsNotEmpty.incrementAndGet();
-
- return null;
- })
- .toList();
- Assert.assertEquals(1, assertCursorsNotEmpty.get());
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(Intervals.utc(timestamp - 60_000, timestamp + 60_000))
+ .setGranularity(Granularities.ALL)
+ .build();
+ try (final CursorMaker maker = sa.asCursorMaker(buildSpec)) {
+ Cursor cursor = maker.makeCursor();
+ final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
Review Comment:
## Unread local variable
Variable 'AtomicInteger assertCursorsNotEmpty' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7614)
##########
processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapterTest.java:
##########
@@ -497,37 +489,28 @@
final StorageAdapter sa = new IncrementalIndexStorageAdapter(index);
- Sequence<Cursor> cursors = sa.makeCursors(
- new DictionaryRaceTestFilter(index, timestamp),
- Intervals.utc(timestamp - 60_000, timestamp + 60_000),
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
- final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
-
- cursors
- .map(cursor -> {
- DimensionSelector dimSelector = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- int cardinality = dimSelector.getValueCardinality();
-
- int rowNumInCursor = 0;
- while (!cursor.isDone()) {
- IndexedInts row = dimSelector.getRow();
- row.forEach(i -> Assert.assertTrue(i < cardinality));
- cursor.advance();
- rowNumInCursor++;
- }
- Assert.assertEquals(5, rowNumInCursor);
- assertCursorsNotEmpty.incrementAndGet();
-
- return null;
- })
- .toList();
- Assert.assertEquals(1, assertCursorsNotEmpty.get());
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setFilter(new DictionaryRaceTestFilter(index, timestamp))
+ .setInterval(Intervals.utc(timestamp - 60_000, timestamp + 60_000))
+ .setGranularity(Granularities.ALL)
+ .build();
+ try (final CursorMaker maker = sa.asCursorMaker(buildSpec)) {
+ Cursor cursor = maker.makeCursor();
+ final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
Review Comment:
## Unread local variable
Variable 'AtomicInteger assertCursorsNotEmpty' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7615)
##########
processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapterTest.java:
##########
@@ -548,93 +531,88 @@
final StorageAdapter sa = new IncrementalIndexStorageAdapter(index);
- Sequence<Cursor> cursors = sa.makeCursors(
- null,
- Intervals.utc(timestamp - 60_000, timestamp + 60_000),
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
- final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
-
- cursors
- .map(cursor -> {
- DimensionSelector dimSelector1A = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- int cardinalityA = dimSelector1A.getValueCardinality();
-
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v1")));
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- DimensionSelector dimSelector1B = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v2")));
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy2"), ImmutableMap.of("billy2", "v3")));
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- DimensionSelector dimSelector1C = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
-
- DimensionSelector dimSelector2D = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy2", "billy2"));
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v3")));
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy3"), ImmutableMap.of("billy3", "")));
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- DimensionSelector dimSelector3E = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy3", "billy3"));
-
- int rowNumInCursor = 0;
- // and then, cursoring continues in the other thread
- while (!cursor.isDone()) {
- IndexedInts rowA = dimSelector1A.getRow();
- rowA.forEach(i -> Assert.assertTrue(i < cardinalityA));
- IndexedInts rowB = dimSelector1B.getRow();
- rowB.forEach(i -> Assert.assertTrue(i < cardinalityA));
- IndexedInts rowC = dimSelector1C.getRow();
- rowC.forEach(i -> Assert.assertTrue(i < cardinalityA));
- IndexedInts rowD = dimSelector2D.getRow();
- // no null id, so should get empty dims array
- Assert.assertEquals(0, rowD.size());
- IndexedInts rowE = dimSelector3E.getRow();
- if (NullHandling.replaceWithDefault()) {
- Assert.assertEquals(1, rowE.size());
- // the null id
- Assert.assertEquals(0, rowE.get(0));
- } else {
- Assert.assertEquals(0, rowE.size());
- }
- cursor.advance();
- rowNumInCursor++;
- }
- Assert.assertEquals(2, rowNumInCursor);
- assertCursorsNotEmpty.incrementAndGet();
-
- return null;
- })
- .toList();
- Assert.assertEquals(1, assertCursorsNotEmpty.get());
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(Intervals.utc(timestamp - 60_000, timestamp + 60_000))
+ .setGranularity(Granularities.ALL)
+ .build();
+ try (final CursorMaker maker = sa.asCursorMaker(buildSpec)) {
+ Cursor cursor = maker.makeCursor();
+ final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
Review Comment:
## Unread local variable
Variable 'AtomicInteger assertCursorsNotEmpty' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7616)
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/scan/ScanQueryFrameProcessor.java:
##########
@@ -252,21 +245,23 @@
if (cursor == null) {
final ResourceHolder<Segment> segmentHolder = closer.register(segment.getOrLoad());
- final Yielder<Cursor> cursorYielder = Yielders.each(
- makeCursors(
- query.withQuerySegmentSpec(new SpecificSegmentSpec(segment.getDescriptor())),
- mapSegment(segmentHolder.get()).asStorageAdapter()
- )
- );
+ final StorageAdapter adapter = mapSegment(segmentHolder.get()).asStorageAdapter();
+ if (adapter == null) {
+ throw new ISE(
+ "Null storage adapter found. Probably trying to issue a query against a segment being memory unmapped."
+ );
+ }
- if (cursorYielder.isDone()) {
+ final CursorMaker maker = closer.register(adapter.asCursorMaker(query.asCursorBuildSpec(null)));
+ final Cursor cursor = maker.makeCursor();
Review Comment:
## Possible confusion of local and field
Confusing name: method [runWithSegment](1) also refers to field [cursor](2) (without qualifying it with 'this').
[Show more details](https://github.com/apache/druid/security/code-scanning/7617)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
Review Comment:
## Missing enum case in switch
Switch statement does not have a case for [ARRAY](1) or [COMPLEX](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/7619)
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/scan/ScanQueryFrameProcessor.java:
##########
@@ -294,15 +289,22 @@
final Frame frame = inputChannel.read();
final FrameSegment frameSegment = new FrameSegment(frame, inputFrameReader, SegmentId.dummy("scan"));
- final long rowsFlushed = setNextCursor(
- Iterables.getOnlyElement(
- makeCursors(
- query.withQuerySegmentSpec(new MultipleIntervalSegmentSpec(Intervals.ONLY_ETERNITY)),
- mapSegment(frameSegment).asStorageAdapter()
- ).toList()
- ),
- frameSegment
- );
+ final StorageAdapter adapter = mapSegment(frameSegment).asStorageAdapter();
+ if (adapter == null) {
+ throw new ISE(
+ "Null storage adapter found. Probably trying to issue a query against a segment being memory unmapped."
+ );
+ }
+
+ final CursorMaker maker = closer.register(adapter.asCursorMaker(query.asCursorBuildSpec(null)));
+ final Cursor cursor = maker.makeCursor();
Review Comment:
## Possible confusion of local and field
Confusing name: method [runWithInputChannel](1) also refers to field [cursor](2) (without qualifying it with 'this').
[Show more details](https://github.com/apache/druid/security/code-scanning/7618)
--
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