You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "imply-cheddar (via GitHub)" <gi...@apache.org> on 2023/02/08 06:52:13 UTC
[GitHub] [druid] imply-cheddar opened a new pull request, #13773: Add window-focused tests from Drill
imply-cheddar opened a new pull request, #13773:
URL: https://github.com/apache/druid/pull/13773
This commit borrows some test definitions from Drill's test suite and tries to use them to flesh out the full validation of window function capbilities.
In order to be able to run these tests, we also add the ability to run a Scan operation against segments, which also meant an implementation of RowsAndColumns for frames.
Initially, in trying to add these tests, I also started trying to
fix the problems that arose. One of which was being able to scan
data from segments for use in queries. This is necessary for these
tests because the Drill tests are generally not grouping on things
first and, instead, are essentially just resolving to scan operators.
After resolving that issue, I ran into another set of bugs specifically
associated with Calcite query planning, where Calcite did not give me
a logical plan that mapped correctly to the semantics of the query.
As I dove into that, I realized that it was a bigger ball of yarn and
this commit was already starting to sprawl out in scope, so I changed
strategy and am instead introducing the test framework and fixes
that have been made so far, will introduce the full set of 2000
files for the tests in a subsequent commit, and just focus this commit
on the code changes required to get everything in place.
Table of Contents (or, what to expect when reviewing this):
1. Changes in `parquet-extension`, these are some code changes to add a main (`ParquetToJson`) that can be used to convert parquet files to new-line delimited Json. This is just to have a utility for developers to use if we ever need to add a new dataset that is defined by parquet and is not a Main intended for a general audience
2. There is a change to the `FrameColumnReader` to have it be able to read out a RowsAndColumns column. This hopefully also provides a relatively straight-forward path for using Frame columns in cases where direct reads from locations makes more sense than the `ColumnSelector`/`DimensionSelector` routes that have been previously employed
3. There's a `DecoratableRowsAndColumn` semantic interface added that takes on "decorations" of a RAC and tries to lazily execute them. This is leveraged in making the ability to read the segment work. Note that the capabilities for reading segments have only the minimum implemented to make these tests run and are not implemented and wired up to be able to actually execute in a distributed environment.
4. There are only a few test cases checked in inside of this PR because, when I initially tried to include all of them, the PR was 2600 files. I didn't want the 2600 files to take away from the code that is deserving of review, so I decided to only check in a few of the files in this PR. Once this is merged, I will do a new PR with all of the other files, which will essentially just be creating new files in the `resources` directory without any actual code changes. That should make it easy to merge in the 2600 extra files for tests.
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] 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.
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1151194442
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/WireTransferable.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.semantic;
+
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+
+public interface WireTransferable
+{
+ static WireTransferable fromRAC(RowsAndColumns rac)
+ {
+ WireTransferable retVal = rac.as(WireTransferable.class);
Review Comment:
Documenting it at some point is definitely good. For now, there is SemanticTestBase and everything that extends 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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1151196356
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
+ this.signature = signature;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return signature.getColumnNames();
+ }
+
+ @Override
+ public int numRows()
+ {
+ return frame.numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ // Use contains so that we can negative cache.
+ if (!colCache.containsKey(name)) {
+ final int columnIndex = signature.indexOf(name);
+ if (columnIndex < 0) {
+ colCache.put(name, null);
+ } else {
+ final ColumnType columnType = signature
+ .getColumnType(columnIndex)
+ .orElseThrow(() -> new ISE("just got the id, why is columnType not there?"));
+
+ colCache.put(name, FrameColumnReaders.create(columnIndex, columnType).readRACColumn(frame));
Review Comment:
Isn't that what I did?
--
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] Add window-focused tests from Drill (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #13773:
URL: https://github.com/apache/druid/pull/13773
--
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] Add window-focused tests from Drill (druid)
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1252448582
##########
processing/src/test/java/org/apache/druid/query/operator/ScanOperatorFactoryTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.operator;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableSet;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.filter.DimFilters;
+import org.apache.druid.query.filter.InDimFilter;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.TestRowsAndColumns;
+import org.apache.druid.query.rowsandcols.semantic.RowsAndColumnsDecorator;
+import org.apache.druid.query.rowsandcols.semantic.TestRowsAndColumnsDecorator;
+import org.apache.druid.segment.VirtualColumns;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+@SuppressWarnings("unchecked")
+public class ScanOperatorFactoryTest
+{
+ static {
+ NullHandling.initializeForTests();
+ }
+
+ @Test
+ public void testSerialization() throws JsonProcessingException
+ {
+ ObjectMapper mapper = DefaultObjectMapper.INSTANCE;
+
+ ScanOperatorFactory factory = new ScanOperatorFactory(
+ Intervals.utc(0, 6),
+ DimFilters.dimEquals("abc", "d"),
+ 20,
+ Arrays.asList("dim1", "dim2"),
+ VirtualColumns.EMPTY,
+ Arrays.asList(ColumnWithDirection.descending("dim2"), ColumnWithDirection.ascending("dim1"))
+ );
+
+ final String asString = mapper.writeValueAsString(factory);
+ final ScanOperatorFactory deserialized = mapper.readValue(asString, ScanOperatorFactory.class);
+
+ Assert.assertEquals(factory, deserialized);
+ Assert.assertEquals(factory.hashCode(), deserialized.hashCode());
+ }
+
+ @Test
+ public void testWrappedOperatorCarriesThroughValues()
+ {
+ Interval[] intervals = new Interval[]{Intervals.utc(0, 6), Intervals.utc(6, 13), Intervals.utc(4, 8)};
+ DimFilter[] filters = new DimFilter[]{
+ new InDimFilter("dim", ImmutableSet.of("a", "b", "c", "e", "g")),
+ DimFilters.and(
+ new InDimFilter("dim", ImmutableSet.of("a", "b", "g")),
+ DimFilters.dimEquals("val", "789")
+ ),
+ DimFilters.or(
+ DimFilters.dimEquals("dim", "b"),
+ DimFilters.dimEquals("val", "789")
+ ),
+ DimFilters.dimEquals("dim", "f")
+ };
+ int[] limits = new int[]{100, 1000};
+ List<ColumnWithDirection>[] orderings = new List[]{
+ Arrays.asList(ColumnWithDirection.descending("__time"), ColumnWithDirection.ascending("dim")),
+ Collections.singletonList(ColumnWithDirection.ascending("val"))
+ };
+
+ for (int i = 2; i <= intervals.length; ++i) {
+ Interval interval = (i == 0 ? null : intervals[i - 1]);
Review Comment:
## Useless comparison test
Test is always false.
[Show more details](https://github.com/apache/druid/security/code-scanning/5213)
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java:
##########
@@ -219,5 +178,222 @@
}
return fn.apply(retVal);
}
+
+ private class PassThroughColumnValueSelector implements ColumnValueSelector
+ {
+ private final Class myClazz;
+ private final ColumnAccessor columnAccessor;
+
+ public PassThroughColumnValueSelector(
+ ColumnAccessor columnAccessor
+ )
+ {
+ this.columnAccessor = columnAccessor;
+ switch (columnAccessor.getType().getType()) {
+ case LONG:
+ myClazz = long.class;
+ break;
+ case DOUBLE:
+ myClazz = double.class;
+ break;
+ case FLOAT:
+ myClazz = float.class;
+ break;
+ case ARRAY:
+ myClazz = List.class;
+ default:
+ throw DruidException.defensive("this class cannot handle type [%s]", columnAccessor.getType());
+ }
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ return columnAccessor.getObject(cellIdSupplier.get());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public Class classOfObject()
+ {
+ return myClazz;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ return columnAccessor.isNull(cellIdSupplier.get());
+ }
+
+ @Override
+ public long getLong()
+ {
+ return columnAccessor.getLong(cellIdSupplier.get());
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return columnAccessor.getFloat(cellIdSupplier.get());
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return columnAccessor.getDouble(cellIdSupplier.get());
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+ }
+
+ private class StringColumnValueSelector implements ColumnValueSelector
+ {
+ private final ColumnAccessor columnAccessor;
+
+ public StringColumnValueSelector(
+ ColumnAccessor columnAccessor
+ )
+ {
+ this.columnAccessor = columnAccessor;
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ // We want our String columns to be ByteBuffers, but users of this ColumnValueSelector interface
+ // would generally expect String objects instead of UTF8 ByteBuffers, so we have to convert here
+ // if we get a ByteBuffer.
+
+ final Object retVal = columnAccessor.getObject(cellIdSupplier.get());
+ if (retVal instanceof ByteBuffer) {
+ return StringUtils.fromUtf8(((ByteBuffer) retVal).asReadOnlyBuffer());
+ }
+ return retVal;
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public Class classOfObject()
+ {
+ return String.class;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ return columnAccessor.isNull(cellIdSupplier.get());
+ }
+
+ @Override
+ public long getLong()
+ {
+ return columnAccessor.getLong(cellIdSupplier.get());
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return columnAccessor.getFloat(cellIdSupplier.get());
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return columnAccessor.getDouble(cellIdSupplier.get());
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+ }
+
+ private class ComplexColumnValueSelector implements ColumnValueSelector
+ {
+ private final AtomicReference<Class> myClazz;
+ private final ColumnAccessor columnAccessor;
+
+ public ComplexColumnValueSelector(ColumnAccessor columnAccessor)
+ {
+ this.columnAccessor = columnAccessor;
+ myClazz = new AtomicReference<>(null);
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ return columnAccessor.getObject(cellIdSupplier.get());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public Class classOfObject()
+ {
+ Class retVal = myClazz.get();
+ if (retVal == null) {
+ retVal = findClazz();
+ myClazz.set(retVal);
+ }
+ return retVal;
+ }
+
+ private Class findClazz()
+ {
+ final ColumnType type = columnAccessor.getType();
+ if (type.getType() == ValueType.COMPLEX) {
+ final ComplexMetricSerde serdeForType = ComplexMetrics.getSerdeForType(type.getComplexTypeName());
+ if (serdeForType != null && serdeForType.getObjectStrategy() != null) {
+ return serdeForType.getObjectStrategy().getClazz();
Review Comment:
## Deprecated method or constructor invocation
Invoking [ComplexMetricSerde.getObjectStrategy](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/5212)
##########
processing/src/test/java/org/apache/druid/query/rowsandcols/semantic/RowsAndColumnsDecoratorTest.java:
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.semantic;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.java.util.common.UOE;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.filter.InDimFilter;
+import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.query.rowsandcols.MapOfColumnsRowsAndColumns;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.ColumnAccessor;
+import org.apache.druid.segment.ArrayListSegment;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.Cursor;
+import org.apache.druid.segment.VirtualColumns;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.TypeStrategy;
+import org.apache.druid.segment.filter.AndFilter;
+import org.apache.druid.segment.filter.OrFilter;
+import org.apache.druid.segment.filter.SelectorFilter;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.function.Function;
+
+@SuppressWarnings({"unchecked", "rawtypes", "ConstantConditions", "SingleStatementInBlock", "VariableNotUsedInsideIf"})
+public class RowsAndColumnsDecoratorTest extends SemanticTestBase
+{
+ public RowsAndColumnsDecoratorTest(
+ String name,
+ Function<MapOfColumnsRowsAndColumns, RowsAndColumns> fn
+ )
+ {
+ super(name, fn);
+ }
+
+ @Test
+ public void testDecoration()
+ {
+ Object[][] vals = new Object[][]{
+ {1L, "a", 123L, 0L},
+ {2L, "a", 456L, 1L},
+ {3L, "b", 789L, 2L},
+ {4L, "b", 123L, 3L},
+ {5L, "c", 456L, 4L},
+ {6L, "c", 789L, 5L},
+ {7L, "c", 123L, 6L},
+ {8L, "d", 456L, 7L},
+ {9L, "e", 789L, 8L},
+ {10L, "f", 123L, 9L},
+ {11L, "f", 456L, 10L},
+ {12L, "g", 789L, 11L},
+ };
+
+ RowSignature siggy = RowSignature.builder()
+ .add("__time", ColumnType.LONG)
+ .add("dim", ColumnType.STRING)
+ .add("val", ColumnType.LONG)
+ .add("arrayIndex", ColumnType.LONG)
+ .build();
+
+ final RowsAndColumns base = make(MapOfColumnsRowsAndColumns.fromRowObjects(vals, siggy));
+
+ Interval[] intervals = new Interval[]{Intervals.utc(0, 6), Intervals.utc(6, 13), Intervals.utc(4, 8)};
+ Filter[] filters = new Filter[]{
+ new InDimFilter("dim", ImmutableSet.of("a", "b", "c", "e", "g")),
+ new AndFilter(Arrays.asList(
+ new InDimFilter("dim", ImmutableSet.of("a", "b", "g")),
+ new SelectorFilter("val", "789")
+ )),
+ new OrFilter(Arrays.asList(
+ new SelectorFilter("dim", "b"),
+ new SelectorFilter("val", "789")
+ )),
+ new SelectorFilter("dim", "f")
+ };
+ int[] limits = new int[]{3, 6, 100};
+ List<ColumnWithDirection>[] orderings = new List[]{
+ Arrays.asList(ColumnWithDirection.descending("__time"), ColumnWithDirection.ascending("dim")),
+ Collections.singletonList(ColumnWithDirection.ascending("val"))
+ };
+
+ // call the same method multiple times
+
+ for (int i = 2; i <= intervals.length; ++i) {
+ Interval interval = (i == 0 ? null : intervals[i - 1]);
Review Comment:
## Useless comparison test
Test is always false.
[Show more details](https://github.com/apache/druid/security/code-scanning/5214)
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultColumnSelectorFactoryMaker.java:
##########
@@ -219,5 +178,222 @@
}
return fn.apply(retVal);
}
+
+ private class PassThroughColumnValueSelector implements ColumnValueSelector
+ {
+ private final Class myClazz;
+ private final ColumnAccessor columnAccessor;
+
+ public PassThroughColumnValueSelector(
+ ColumnAccessor columnAccessor
+ )
+ {
+ this.columnAccessor = columnAccessor;
+ switch (columnAccessor.getType().getType()) {
+ case LONG:
+ myClazz = long.class;
+ break;
+ case DOUBLE:
+ myClazz = double.class;
+ break;
+ case FLOAT:
+ myClazz = float.class;
+ break;
+ case ARRAY:
+ myClazz = List.class;
+ default:
+ throw DruidException.defensive("this class cannot handle type [%s]", columnAccessor.getType());
+ }
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ return columnAccessor.getObject(cellIdSupplier.get());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public Class classOfObject()
+ {
+ return myClazz;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ return columnAccessor.isNull(cellIdSupplier.get());
+ }
+
+ @Override
+ public long getLong()
+ {
+ return columnAccessor.getLong(cellIdSupplier.get());
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return columnAccessor.getFloat(cellIdSupplier.get());
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return columnAccessor.getDouble(cellIdSupplier.get());
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+ }
+
+ private class StringColumnValueSelector implements ColumnValueSelector
+ {
+ private final ColumnAccessor columnAccessor;
+
+ public StringColumnValueSelector(
+ ColumnAccessor columnAccessor
+ )
+ {
+ this.columnAccessor = columnAccessor;
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ // We want our String columns to be ByteBuffers, but users of this ColumnValueSelector interface
+ // would generally expect String objects instead of UTF8 ByteBuffers, so we have to convert here
+ // if we get a ByteBuffer.
+
+ final Object retVal = columnAccessor.getObject(cellIdSupplier.get());
+ if (retVal instanceof ByteBuffer) {
+ return StringUtils.fromUtf8(((ByteBuffer) retVal).asReadOnlyBuffer());
+ }
+ return retVal;
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public Class classOfObject()
+ {
+ return String.class;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ return columnAccessor.isNull(cellIdSupplier.get());
+ }
+
+ @Override
+ public long getLong()
+ {
+ return columnAccessor.getLong(cellIdSupplier.get());
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return columnAccessor.getFloat(cellIdSupplier.get());
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return columnAccessor.getDouble(cellIdSupplier.get());
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+ }
+
+ private class ComplexColumnValueSelector implements ColumnValueSelector
+ {
+ private final AtomicReference<Class> myClazz;
+ private final ColumnAccessor columnAccessor;
+
+ public ComplexColumnValueSelector(ColumnAccessor columnAccessor)
+ {
+ this.columnAccessor = columnAccessor;
+ myClazz = new AtomicReference<>(null);
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ return columnAccessor.getObject(cellIdSupplier.get());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public Class classOfObject()
+ {
+ Class retVal = myClazz.get();
+ if (retVal == null) {
+ retVal = findClazz();
+ myClazz.set(retVal);
+ }
+ return retVal;
+ }
+
+ private Class findClazz()
+ {
+ final ColumnType type = columnAccessor.getType();
+ if (type.getType() == ValueType.COMPLEX) {
+ final ComplexMetricSerde serdeForType = ComplexMetrics.getSerdeForType(type.getComplexTypeName());
+ if (serdeForType != null && serdeForType.getObjectStrategy() != null) {
Review Comment:
## Deprecated method or constructor invocation
Invoking [ComplexMetricSerde.getObjectStrategy](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/5211)
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1151195096
##########
processing/src/main/java/org/apache/druid/frame/read/columnar/FrameColumnReader.java:
##########
@@ -28,6 +29,11 @@
*/
public interface FrameColumnReader
{
+ /**
+ * Returns a {@link Column} from the frame.
+ */
+ Column readRACColumn(Frame frame);
Review Comment:
ArrayListSegment is also a RAC and that's row-oriented. So, yes, it's OK. It's just code that needs to exist.
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1151197961
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
+ this.signature = signature;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return signature.getColumnNames();
+ }
+
+ @Override
+ public int numRows()
+ {
+ return frame.numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ // Use contains so that we can negative cache.
+ if (!colCache.containsKey(name)) {
+ final int columnIndex = signature.indexOf(name);
+ if (columnIndex < 0) {
+ colCache.put(name, null);
+ } else {
+ final ColumnType columnType = signature
+ .getColumnType(columnIndex)
+ .orElseThrow(() -> new ISE("just got the id, why is columnType not there?"));
+
+ colCache.put(name, FrameColumnReaders.create(columnIndex, columnType).readRACColumn(frame));
+ }
+ }
+ return colCache.get(name);
+
+ }
+
+ @Nullable
+ @Override
+ public <T> T as(Class<T> clazz)
+ {
+ throw new UnsupportedOperationException();
Review Comment:
Probably, yes. This is most likely just cruft that I overlooked. The introduce of the Frame stuff is still fresh.
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1151200580
##########
processing/src/main/java/org/apache/druid/query/operator/SegmentToRowsAndColumnsOperator.java:
##########
@@ -48,6 +48,9 @@ public Closeable goOrContinue(Closeable continuation, Receiver receiver)
}
RowsAndColumns rac = shifty.as(RowsAndColumns.class);
+ if (shifty instanceof RowsAndColumns) {
+ rac = (RowsAndColumns) shifty;
Review Comment:
This if statement is definitely placed quite weird-like, you are right
--
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
[GitHub] [druid] gianm commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1150255111
##########
processing/src/main/java/org/apache/druid/jackson/DruidDefaultSerializersModule.java:
##########
@@ -182,5 +184,21 @@ public ByteOrder deserialize(JsonParser jp, DeserializationContext ctxt) throws
}
);
addDeserializer(ResponseContext.class, new ResponseContextDeserializer());
+
+ addSerializer(RowsAndColumns.class, new JsonSerializer<RowsAndColumns>()
+ {
+ @Override
+ public void serialize(
+ RowsAndColumns value,
+ JsonGenerator gen,
+ SerializerProvider serializers
+ ) throws IOException
+ {
+ // It would be really cool if jackson offered an output stream that would allow us to push bytes
Review Comment:
would `gen.writeBinary(InputStream, int)` do it? i.e., have `WireTransferable` return an `InputStream` instead of `byte[]`.
--
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
[GitHub] [druid] gianm commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1151226325
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
+ this.signature = signature;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return signature.getColumnNames();
+ }
+
+ @Override
+ public int numRows()
+ {
+ return frame.numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ // Use contains so that we can negative cache.
+ if (!colCache.containsKey(name)) {
+ final int columnIndex = signature.indexOf(name);
+ if (columnIndex < 0) {
+ colCache.put(name, null);
+ } else {
+ final ColumnType columnType = signature
+ .getColumnType(columnIndex)
+ .orElseThrow(() -> new ISE("just got the id, why is columnType not there?"));
+
+ colCache.put(name, FrameColumnReaders.create(columnIndex, columnType).readRACColumn(frame));
Review Comment:
I was suggesting having `FrameReader#readRACColumn`, so this code would look like:
```java
@Nullable
@Override
public Column findColumn(String name)
{
if (!colCache.containsKey(name)) {
if (signature.contains(name)) {
colCache.put(name, null);
} else {
colCache.put(name, frameReader.readRACColumn(frame, name));
}
}
return colCache.get(name);
}
```
The `frameReader` would either be passed in to the constructor (to promote reuse) or created in the constructor using `FrameReader.create(signature)` (if reuse is not important).
Later on, if/when we add support for row-based-frame-based RowsAndColumns, the logic for that would live inside the `FrameReader`, rather than here, which keeps this method simple.
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1151196356
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
+ this.signature = signature;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return signature.getColumnNames();
+ }
+
+ @Override
+ public int numRows()
+ {
+ return frame.numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ // Use contains so that we can negative cache.
+ if (!colCache.containsKey(name)) {
+ final int columnIndex = signature.indexOf(name);
+ if (columnIndex < 0) {
+ colCache.put(name, null);
+ } else {
+ final ColumnType columnType = signature
+ .getColumnType(columnIndex)
+ .orElseThrow(() -> new ISE("just got the id, why is columnType not there?"));
+
+ colCache.put(name, FrameColumnReaders.create(columnIndex, columnType).readRACColumn(frame));
Review Comment:
Isn't that what I did? `FrameColumnReaders.create()` returns a `FrameColumnReader` and then `readRACColumn` is called on that?
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1239156566
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
+ this.signature = signature;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return signature.getColumnNames();
+ }
+
+ @Override
+ public int numRows()
+ {
+ return frame.numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ // Use contains so that we can negative cache.
+ if (!colCache.containsKey(name)) {
+ final int columnIndex = signature.indexOf(name);
+ if (columnIndex < 0) {
+ colCache.put(name, null);
+ } else {
+ final ColumnType columnType = signature
+ .getColumnType(columnIndex)
+ .orElseThrow(() -> new ISE("just got the id, why is columnType not there?"));
+
+ colCache.put(name, FrameColumnReaders.create(columnIndex, columnType).readRACColumn(frame));
Review Comment:
Will leave this as a thing for the future.
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1239150177
##########
processing/src/main/java/org/apache/druid/jackson/DruidDefaultSerializersModule.java:
##########
@@ -182,5 +184,21 @@ public ByteOrder deserialize(JsonParser jp, DeserializationContext ctxt) throws
}
);
addDeserializer(ResponseContext.class, new ResponseContextDeserializer());
+
+ addSerializer(RowsAndColumns.class, new JsonSerializer<RowsAndColumns>()
+ {
+ @Override
+ public void serialize(
+ RowsAndColumns value,
+ JsonGenerator gen,
+ SerializerProvider serializers
+ ) throws IOException
+ {
+ // It would be really cool if jackson offered an output stream that would allow us to push bytes
Review Comment:
It would seem like it, but switching things around to passing down an InputStream just makes the `WireTransferable` code a lot more complex as now it needs to lazily convert to bytes based on calls to `InputStream`. It's likely so unwieldy that implementations would just resort to building a `byte[]` and using a BAIS anyway. The natural way to code it would be to say "write yourself to this stream".
--
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] Add window-focused tests from Drill (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1254652197
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -1380,11 +1383,21 @@ private WindowOperatorQuery toWindowQuery()
return null;
}
+ final DataSource myDataSource;
+ if (dataSource instanceof TableDataSource) {
Review Comment:
What if the underlying datasource is a `join` or `unnest`? Do we need special handling or is that taken care of by the `WindowOperatorQuery` itself?
--
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] Add window-focused tests from Drill (druid)
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1255016876
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -1380,11 +1383,21 @@ private WindowOperatorQuery toWindowQuery()
return null;
}
+ final DataSource myDataSource;
+ if (dataSource instanceof TableDataSource) {
Review Comment:
This code is insufficient for those cases. I expect that the Drill tests will end up covering those cases. I.e. that's still WIP. In the "simplest" solution, however, this should perhaps be inverted to just check if it's already a `QueryDataSource` and if it's not, to make it one. That would cover all of the cases and handle things as a scan.
That said, the whole planning a scan query thing is a short-term hack/work-around anyway, I expect that in the end this will just plan the "operator query" all the way down to the segment (which is what happens with this workaround as well, it just happens in a really... odd... way)
--
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
[GitHub] [druid] gianm commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1150255111
##########
processing/src/main/java/org/apache/druid/jackson/DruidDefaultSerializersModule.java:
##########
@@ -182,5 +184,21 @@ public ByteOrder deserialize(JsonParser jp, DeserializationContext ctxt) throws
}
);
addDeserializer(ResponseContext.class, new ResponseContextDeserializer());
+
+ addSerializer(RowsAndColumns.class, new JsonSerializer<RowsAndColumns>()
+ {
+ @Override
+ public void serialize(
+ RowsAndColumns value,
+ JsonGenerator gen,
+ SerializerProvider serializers
+ ) throws IOException
+ {
+ // It would be really cool if jackson offered an output stream that would allow us to push bytes
Review Comment:
would `gen.writeBinary(InputStream, int)` do it? i.e., have `WireTransferable` return an`InputStream` instead of `byte[]`.
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/WireTransferable.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.semantic;
+
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+
+public interface WireTransferable
+{
+ static WireTransferable fromRAC(RowsAndColumns rac)
+ {
+ WireTransferable retVal = rac.as(WireTransferable.class);
Review Comment:
A disadvantage of `as` is that it's not clear what all could be passed in, and what each thing enables. I think we could use a list somewhere. Maybe on Javadoc for `RowsAndColumns#as` itself?
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
+ this.signature = signature;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return signature.getColumnNames();
+ }
+
+ @Override
+ public int numRows()
+ {
+ return frame.numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ // Use contains so that we can negative cache.
+ if (!colCache.containsKey(name)) {
+ final int columnIndex = signature.indexOf(name);
+ if (columnIndex < 0) {
+ colCache.put(name, null);
+ } else {
+ final ColumnType columnType = signature
+ .getColumnType(columnIndex)
+ .orElseThrow(() -> new ISE("just got the id, why is columnType not there?"));
+
+ colCache.put(name, FrameColumnReaders.create(columnIndex, columnType).readRACColumn(frame));
Review Comment:
I've been trying to re-use `FrameColumnReader` and `FieldReader` as much as possible (and centralize logic for reading from frames) by putting stuff like this into `FrameReader`.
I suggest having this be a method in there like `readRACColumn(Frame frame, String columnName)`. Its implementation would be similar to `columnCapabilities(Frame frame, String columnName)`. In the future, if this code is extended to handle row-based frames too, then having it inside `FrameReader` would provide a nice abstraction.
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
+ this.signature = signature;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return signature.getColumnNames();
+ }
+
+ @Override
+ public int numRows()
+ {
+ return frame.numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ // Use contains so that we can negative cache.
+ if (!colCache.containsKey(name)) {
+ final int columnIndex = signature.indexOf(name);
+ if (columnIndex < 0) {
+ colCache.put(name, null);
+ } else {
+ final ColumnType columnType = signature
+ .getColumnType(columnIndex)
+ .orElseThrow(() -> new ISE("just got the id, why is columnType not there?"));
+
+ colCache.put(name, FrameColumnReaders.create(columnIndex, columnType).readRACColumn(frame));
+ }
+ }
+ return colCache.get(name);
+
+ }
+
+ @Nullable
+ @Override
+ public <T> T as(Class<T> clazz)
+ {
+ throw new UnsupportedOperationException();
Review Comment:
Is `return null` a better implementation if we don't know how to become anything else?
##########
processing/src/main/java/org/apache/druid/frame/read/columnar/ComplexFrameColumnReader.java:
##########
@@ -47,6 +51,38 @@ public class ComplexFrameColumnReader implements FrameColumnReader
this.columnNumber = columnNumber;
}
+ @Override
+ public Column readRACColumn(Frame frame)
Review Comment:
Lot of copypasta here, could it be factored out into a method that makes a `ComplexFrameColumn`, and then the RACColumn and ColumnPlus readers could call that?
##########
processing/src/main/java/org/apache/druid/frame/read/columnar/DoubleFrameColumnReader.java:
##########
@@ -48,6 +51,18 @@ public class DoubleFrameColumnReader implements FrameColumnReader
this.columnNumber = columnNumber;
}
+ @Override
+ public Column readRACColumn(Frame frame)
Review Comment:
Similar copypasta comment for this & the other readers.
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/DecoratedRowsAndColumns.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.FrameType;
+import org.apache.druid.frame.allocation.ArenaMemoryAllocatorFactory;
+import org.apache.druid.frame.key.KeyColumn;
+import org.apache.druid.frame.key.KeyOrder;
+import org.apache.druid.frame.write.FrameWriter;
+import org.apache.druid.frame.write.FrameWriterFactory;
+import org.apache.druid.frame.write.FrameWriters;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.query.rowsandcols.concrete.FrameRowsAndColumns;
+import org.apache.druid.query.rowsandcols.semantic.DecoratableRowsAndColumns;
+import org.apache.druid.segment.ColumnSelectorFactory;
+import org.apache.druid.segment.Cursor;
+import org.apache.druid.segment.StorageAdapter;
+import org.apache.druid.segment.VirtualColumns;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.RowSignature;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+public class DecoratedRowsAndColumns implements DecoratableRowsAndColumns
+{
+ private final RowsAndColumns base;
+
+ private RowsAndColumns materialized = null;
+
+ private Interval interval = null;
+ private Filter filter = null;
+ private VirtualColumns virtualColumns = null;
+ private int limit = -1;
+ private LinkedHashSet<String> viewableColumns = null;
+ private List<ColumnWithDirection> ordering = null;
+
+ public DecoratedRowsAndColumns(
+ RowsAndColumns base
+ )
+ {
+ this.base = base;
+ }
+
+ @Override
+ public Collection<String> getColumnNames()
+ {
+ return viewableColumns == null ? base.getColumnNames() : viewableColumns;
+ }
+
+ @Override
+ public int numRows()
+ {
+ return materializedOrBase().numRows();
+ }
+
+ @Nullable
+ @Override
+ public Column findColumn(String name)
+ {
+ if (viewableColumns != null && !viewableColumns.contains(name)) {
+ return null;
+ }
+
+ return materializedOrBase().findColumn(name);
+ }
+
+ @Nullable
+ @Override
+ public <T> T as(Class<T> clazz)
+ {
+ return null;
+ }
+
+ @Override
+ public void limitTimeRange(Interval interval)
+ {
+ this.interval = interval;
Review Comment:
Throw an error if `interval` was already set? (Otherwise, if it's called twice by mistake, the prior value would be lost.) Unless it's intentional that the prior value be lost if this method is called twice? In which case it would be nice if the javadoc mentioned that.
(Similar comment for the other methods.)
##########
processing/src/main/java/org/apache/druid/query/operator/NaivePartitioningOperator.java:
##########
@@ -55,33 +57,120 @@ public NaivePartitioningOperator(
@Override
public Closeable goOrContinue(Closeable continuation, Receiver receiver)
{
- return child.goOrContinue(
+ if (continuation != null) {
+ Continuation cont = (Continuation) continuation;
+
+ if (cont.iter != null) {
+ while (cont.iter.hasNext()) {
+ final Signal signal = receiver.push(cont.iter.next());
+ switch (signal) {
+ case GO:
+ break;
+ case PAUSE:
+ if (cont.iter.hasNext()) {
+ return cont;
+ }
+
+ if (cont.subContinuation == null) {
+ // We were finished anyway
+ receiver.completed();
+ return null;
+ }
+
+ return new Continuation(null, cont.subContinuation);
+ case STOP:
+ receiver.completed();
+ try {
+ cont.close();
+ }
+ catch (IOException e) {
+ throw new RE(e, "Unable to close continutation");
+ }
+ return null;
+ default:
+ throw new RE("Unknown signal[%s]", signal);
+ }
+ }
+
+ if (cont.subContinuation == null) {
+ receiver.completed();
+ return null;
+ }
+ }
+
+ continuation = cont.subContinuation;
+ }
+
+ AtomicReference<Iterator<RowsAndColumns>> iterHolder = new AtomicReference<>();
+
+ final Closeable retVal = child.goOrContinue(
continuation,
new Receiver()
{
@Override
public Signal push(RowsAndColumns rac)
{
+ if (rac == null) {
+ throw new ISE("huh?");
Review Comment:
If I ever see this in prod I am gonna definitely say… "huh?"
##########
processing/src/main/java/org/apache/druid/frame/read/columnar/FrameColumnReader.java:
##########
@@ -28,6 +29,11 @@
*/
public interface FrameColumnReader
{
+ /**
+ * Returns a {@link Column} from the frame.
+ */
+ Column readRACColumn(Frame frame);
Review Comment:
Is it OK that only the columnar frames are readable as RAC?
The row-oriented frames are currently more common in prod, because they support fast sorting and merging, which is very important for most current usages of frames. (Keys are contiguous in memory, so we can compare keys using memory comparisons as bytes.)
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.concrete;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.read.columnar.FrameColumnReaders;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+
+import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+
+public class FrameRowsAndColumns implements RowsAndColumns
+{
+ private final Frame frame;
+ private final RowSignature signature;
+ private final LinkedHashMap<String, Column> colCache = new LinkedHashMap<>();
+
+ public FrameRowsAndColumns(Frame frame, RowSignature signature)
+ {
+ this.frame = frame;
Review Comment:
This code only works for columnar frames, so I'd suggest including a check here for fast-fail. This is the way it's done in other places:
```
this.frame = FrameType.COLUMNAR.ensureType(frame);
```
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/DecoratedRowsAndColumns.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.frame.FrameType;
+import org.apache.druid.frame.allocation.ArenaMemoryAllocatorFactory;
+import org.apache.druid.frame.key.KeyColumn;
+import org.apache.druid.frame.key.KeyOrder;
+import org.apache.druid.frame.write.FrameWriter;
+import org.apache.druid.frame.write.FrameWriterFactory;
+import org.apache.druid.frame.write.FrameWriters;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.query.rowsandcols.concrete.FrameRowsAndColumns;
+import org.apache.druid.query.rowsandcols.semantic.DecoratableRowsAndColumns;
+import org.apache.druid.segment.ColumnSelectorFactory;
+import org.apache.druid.segment.Cursor;
+import org.apache.druid.segment.StorageAdapter;
+import org.apache.druid.segment.VirtualColumns;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.RowSignature;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+public class DecoratedRowsAndColumns implements DecoratableRowsAndColumns
Review Comment:
Javadoc would be nice. Also, the concrete impl having a similar and only subtly different name to the interface is tricky on the eyes. Maybe `DecoratableRowsAndColumnsImpl` would be a better name.
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DecoratableRowsAndColumns.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.semantic;
+
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.query.rowsandcols.DecoratedRowsAndColumns;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.segment.VirtualColumns;
+import org.joda.time.Interval;
+
+import java.util.List;
+
+public interface DecoratableRowsAndColumns extends RowsAndColumns
Review Comment:
Javadocs would be nice. Not immediately obvious what this is for.
##########
processing/src/main/java/org/apache/druid/query/operator/SegmentToRowsAndColumnsOperator.java:
##########
@@ -48,6 +48,9 @@ public Closeable goOrContinue(Closeable continuation, Receiver receiver)
}
RowsAndColumns rac = shifty.as(RowsAndColumns.class);
+ if (shifty instanceof RowsAndColumns) {
+ rac = (RowsAndColumns) shifty;
Review Comment:
Shouldn't `shifty` have returned `this` from `as` in this case? Wonder why we need this block.
--
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
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #13773: Add window-focused tests from Drill
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #13773:
URL: https://github.com/apache/druid/pull/13773#discussion_r1239269491
##########
processing/src/test/java/org/apache/druid/query/operator/OperatorTestHelper.java:
##########
@@ -95,20 +101,49 @@
return this;
}
- public OperatorTestHelper withPushFn(JustPushMe fn)
+ public OperatorTestHelper withPushFn(Supplier<JustPushMe> fnSupplier)
{
- return withReceiver(() -> new TestReceiver(fn));
+ return withReceiver(() -> new TestReceiver(fnSupplier.get()));
}
- public OperatorTestHelper runToCompletion(Operator op)
+ public void runToCompletion(Operator op)
{
TestReceiver receiver = this.receiverSupply.get();
Operator.go(op, receiver);
Assert.assertTrue(receiver.isCompleted());
if (finalValidation != null) {
finalValidation.accept(receiver);
}
- return this;
+
+ for (int i = 1; i < receiver.getNumPushed(); ++i) {
Review Comment:
## Comparison of narrow type with wide type in loop condition
Comparison between [expression](1) of type int and [expression](2) of wider type long.
[Show more details](https://github.com/apache/druid/security/code-scanning/5176)
--
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