You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "ankit0811 (via GitHub)" <gi...@apache.org> on 2023/06/21 21:58:07 UTC
[GitHub] [druid] ankit0811 opened a new pull request, #14462: Add support for first/last for double/float/long #10702
ankit0811 opened a new pull request, #14462:
URL: https://github.com/apache/druid/pull/14462
Fixes #10702
### Description
This PR revives https://github.com/apache/druid/pull/10949 which address issue #10702 by adding support for doubleLast, doubleFirst, FloatLast, FloatFirst, longLast and longFirst.
It follows the code flow similar to stringFirst and stringLast
#### Release note
<hr>
##### Key changed/added classes in this PR
Introduces the following new Classes
* `AbstractSerializableLongObjectPairSerde` abstract class to share serde between double/float/long
* `SerializablePairLongDoubleComplexMetricSerde` for double
* `SerializablePairLongFloatComplexMetricSerde` for float
* `SerializablePairLongStringComplexMetricSerde` for long
* `GenericFirstAggregateCombiner` first agg combiner to share between double/float/long
* `GenericLastAggregateCombiner` first agg combiner to share between double/float/long
* `DoubleFirstAggregatorFactory`, `DoubleLastAggregatorFactory`
* `FloatFirstAggregatorFactory`, `FloatLastAggregatorFactory`
* `LongFirstAggregatorFactory`, `LongLastAggregatorFactory`
Further introduces new `ColumnType`
* serializablePairLongDouble
* serializablePairLongFloat
* serializablePairLongString
To-Do
These will be taken up in a follow-up PR
- SerializablePairLongStringComplexMetricSerde to extend AbstractSerializableLongObjectPairSerde
- Add ingestion support from UI for first/last types
- SQL compatibility for querying (latest/earliest) the new ingestion type (Today this is queryable via native json queries not via sql. Needs more discussion on making this SQL compatible)
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [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.
- [x] added integration tests.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419699401
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectColumnHeader.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.java.util.common.RE;
+import org.apache.druid.segment.serde.cell.LongSerializer;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.WritableByteChannel;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectColumnHeader<T extends SerializablePair<Long, ?>>
+{
+ private static final int HEADER_SIZE_BYTES = 4;
Review Comment:
There's a missing comment here from the original code.
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectColumnSerializer.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.java.util.common.io.smoosh.FileSmoosher;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.serde.cell.ByteBufferProvider;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import java.io.IOException;
+import java.nio.channels.WritableByteChannel;
+
+public abstract class AbstractSerializablePairLongObjectColumnSerializer<T extends SerializablePair<Long, ?>> implements
Review Comment:
There are missing Javadocs from the original code. I do think that they should be migrated here.
##########
processing/src/main/java/org/apache/druid/query/aggregation/last/GenericLastAggregateCombiner.java:
##########
@@ -19,42 +19,48 @@
package org.apache.druid.query.aggregation.last;
+import com.google.common.primitives.Longs;
+import org.apache.druid.collections.SerializablePair;
import org.apache.druid.query.aggregation.ObjectAggregateCombiner;
-import org.apache.druid.query.aggregation.SerializablePairLongString;
-import org.apache.druid.query.aggregation.first.StringFirstAggregatorFactory;
import org.apache.druid.segment.ColumnValueSelector;
import javax.annotation.Nullable;
-public class StringLastAggregateCombiner extends ObjectAggregateCombiner<SerializablePairLongString>
+public class GenericLastAggregateCombiner<T extends SerializablePair<Long, ?>> extends ObjectAggregateCombiner<T>
{
- private SerializablePairLongString lastValue;
+ private T lastValue;
+ private final Class<T> pairClass;
+
+ public GenericLastAggregateCombiner(Class<T> pairClass)
+ {
+ this.pairClass = pairClass;
+ }
@Override
public void reset(ColumnValueSelector selector)
{
- lastValue = (SerializablePairLongString) selector.getObject();
+ lastValue = (T) selector.getObject();
}
@Override
public void fold(ColumnValueSelector selector)
{
- SerializablePairLongString newValue = (SerializablePairLongString) selector.getObject();
- if (StringFirstAggregatorFactory.TIME_COMPARATOR.compare(lastValue, newValue) < 0) {
- lastValue = (SerializablePairLongString) selector.getObject();
+ T newValue = (T) selector.getObject();
+ if (Longs.compare(lastValue.lhs, newValue.lhs) <= 0) {
Review Comment:
Why have we converted it from '<' to '<='? If unintentional, let's keep it the same way. Although the logic is still sound, it can lead to changes in the results of the queries of the users who have been using the LATEST function.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/FirstLastUtils.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.aggregation.first;
+
+import org.apache.druid.segment.BaseObjectColumnValueSelector;
+import org.apache.druid.segment.NilColumnValueSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+
+public class FirstLastUtils
+{
+
+ /**
+ * Returns whether a given value selector *might* contain object assignable from pairClass (SerializablePairLong*).
+ */
+ public static boolean selectorNeedsFoldCheck(
+ final BaseObjectColumnValueSelector<?> valueSelector,
+ @Nullable final ColumnCapabilities valueSelectorCapabilities,
+ Class pairClass
+ )
+ {
+ if (valueSelectorCapabilities != null && !valueSelectorCapabilities.is(ValueType.COMPLEX)) {
+ // Known, non-complex type.
+ return false;
+ }
+
+ if (valueSelector instanceof NilColumnValueSelector) {
+ // Nil column, definitely no SerializablePairLongStrings.
Review Comment:
Comment should be generic.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/FirstLastUtils.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.aggregation.first;
+
+import org.apache.druid.segment.BaseObjectColumnValueSelector;
+import org.apache.druid.segment.NilColumnValueSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+
+public class FirstLastUtils
+{
+
+ /**
+ * Returns whether a given value selector *might* contain SerializablePairLongString objects.
+ */
+ public static boolean selectorNeedsFoldCheck(
+ final BaseObjectColumnValueSelector<?> valueSelector,
+ @Nullable final ColumnCapabilities valueSelectorCapabilities,
+ Class pairClass
+ )
+ {
+ if (valueSelectorCapabilities != null && !valueSelectorCapabilities.is(ValueType.COMPLEX)) {
+ // Known, non-complex type.
+ return false;
+ }
+
+ if (valueSelector instanceof NilColumnValueSelector) {
+ // Nil column, definitely no SerializablePairLongStrings.
+ return false;
+ }
+
+ // Check if the selector class could possibly be a SerializablePairLongString (either a superclass or subclass).
+ final Class<?> clazz = valueSelector.classOfObject();
+ return clazz.isAssignableFrom(pairClass)
+ || pairClass.isAssignableFrom(clazz);
+ }
+
+ /**
+ * Returns whether an object *might* contain SerializablePairLongString objects.
Review Comment:
It is still outdated
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongStringColumnSerializer.java:
##########
Review Comment:
* Move the Javadoc up.
* Do we need to override all the methods here? I suppose implementing it as the other numeric serializers should have been sufficient, and we can get rid of the duplicate code
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongLongComplexMetricSerde.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.column.ColumnBuilder;
+import org.apache.druid.segment.data.ObjectStrategy;
+import org.apache.druid.segment.serde.cell.NativeClearedByteBufferProvider;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Comparator;
+
+public class SerializablePairLongLongComplexMetricSerde extends AbstractSerializableLongObjectPairSerde<SerializablePairLongLong>
+{
+ public static final int EXPECTED_VERSION = 1;
Review Comment:
Lets make it 3 as in other serde.
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectDeltaEncodedStagedSerde.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import com.google.common.primitives.Ints;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectDeltaEncodedStagedSerde<T extends SerializablePair<Long, ?>> implements
Review Comment:
nit: Javadocs missing
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectSimpleStagedSerde.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectSimpleStagedSerde<T extends SerializablePair<Long, ?>> implements StagedSerde<T>
Review Comment:
nit: Javadoc
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongFloatBufferStore.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.aggregation;
+
+import javax.annotation.Nonnull;
+
+public class SerializablePairLongFloatBufferStore extends AbstractSerializablePairLongObjectBufferStore<SerializablePairLongFloat>
+{
+ public SerializablePairLongFloatBufferStore(SerializedStorage<SerializablePairLongFloat> serializedStorage)
+ {
+ super(serializedStorage);
+ }
+
+
+ @Override
+ @Nonnull
+ public AbstractSerializablePairLongObjectColumnHeader<SerializablePairLongFloat> createColumnHeader()
+ {
+ long maxDelta = maxValue - minValue;
+ SerializablePairLongFloatColumnHeader columnHeader;
+
+ if (minValue < maxValue && maxDelta < 0 || minValue > maxValue) {
+ // true iff
+ // 1. we have overflow in our range || 2. we have only seen null values
+ // in this case, effectively disable delta encoding by using longs and a min value of 0
Review Comment:
Duplication of comment. We should add it as info at the top level comment of the Abstract serializer, and not specific to implementation. Might have been a good refactor candidate but since the PR is close to completion, let's minimize how much code we change up.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregator.java:
##########
@@ -19,30 +19,36 @@
package org.apache.druid.query.aggregation.first;
-import org.apache.druid.collections.SerializablePair;
-import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.query.aggregation.SerializablePairLongDouble;
import org.apache.druid.segment.BaseLongColumnValueSelector;
+import org.apache.druid.segment.ColumnValueSelector;
-public class DoubleFirstAggregator extends NumericFirstAggregator<BaseDoubleColumnValueSelector>
+public class DoubleFirstAggregator extends NumericFirstAggregator
{
double firstValue;
- public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, BaseDoubleColumnValueSelector valueSelector)
+ public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, ColumnValueSelector valueSelector, boolean needsFoldCheck)
Review Comment:
It **should** require a folds check in case of rollups I think.
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongFloatComplexMetricSerde.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.column.ColumnBuilder;
+import org.apache.druid.segment.data.ObjectStrategy;
+import org.apache.druid.segment.serde.cell.NativeClearedByteBufferProvider;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Comparator;
+
+public class SerializablePairLongFloatComplexMetricSerde extends AbstractSerializableLongObjectPairSerde<SerializablePairLongFloat>
+{
+ public static final int EXPECTED_VERSION = 1;
Review Comment:
Let's set it to 3 as mentioned in the other comment. Also, for the numeric serdes, please add a comment about the version 3 along the lines of
"To maintain parity with the longString pair serde, we have made it 3 because it uses the same delta & block encoding technique. 0,1,2 versions for numeric serdes donot exist."
Perhaps we should mention it at the top level AbstractSerializableLongObjectPairSerde itself.
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectBufferStore.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.java.util.common.io.smoosh.FileSmoosher;
+import org.apache.druid.segment.serde.Serializer;
+import org.apache.druid.segment.serde.cell.ByteBufferProvider;
+import org.apache.druid.segment.serde.cell.CellWriter;
+import org.apache.druid.segment.serde.cell.IOIterator;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.io.IOException;
+import java.nio.channels.WritableByteChannel;
+
+public abstract class AbstractSerializablePairLongObjectBufferStore<T extends SerializablePair<Long, ?>>
+{
+ private final SerializedStorage<T> serializedStorage;
+
+ long minValue = Long.MAX_VALUE;
+ long maxValue = Long.MIN_VALUE;
+
+ AbstractSerializablePairLongObjectBufferStore(SerializedStorage<T> serializedStorage)
+ {
+ this.serializedStorage = serializedStorage;
+ }
+
+ public void store(@Nullable T pairLongObject) throws IOException
+ {
+ if (pairLongObject != null && pairLongObject.lhs != null) {
+ minValue = Math.min(minValue, pairLongObject.lhs);
+ maxValue = Math.max(maxValue, pairLongObject.lhs);
+ }
+
+ serializedStorage.store(pairLongObject);
+ }
+
+ public TransferredBuffer transferToRowWriter(
+ ByteBufferProvider byteBufferProvider,
+ SegmentWriteOutMedium segmentWriteOutMedium
+ ) throws IOException
+ {
+ AbstractSerializablePairLongObjectColumnHeader<T> columnHeader = createColumnHeader();
+ AbstractSerializablePairLongObjectDeltaEncodedStagedSerde<T> deltaEncodedSerde = createDeltaEncodedSerde(columnHeader);
+
+ try (CellWriter cellWriter = new CellWriter.Builder(segmentWriteOutMedium).setByteBufferProvider(byteBufferProvider)
+ .build()) {
+ try (IOIterator<T> bufferIterator = iterator()) {
+ while (bufferIterator.hasNext()) {
+ T pairLongObject = bufferIterator.next();
+ byte[] serialized = deltaEncodedSerde.serialize(pairLongObject);
+
+ cellWriter.write(serialized);
+ }
+
+ cellWriter.close();
+
+ return new TransferredBuffer(cellWriter, columnHeader);
+ }
+ }
+ }
+
+ public abstract AbstractSerializablePairLongObjectColumnHeader<T> createColumnHeader();
+ public abstract AbstractSerializablePairLongObjectDeltaEncodedStagedSerde<T> createDeltaEncodedSerde(AbstractSerializablePairLongObjectColumnHeader<T> columnHeader);
+
+ public IOIterator<T> iterator() throws IOException
+ {
+ return serializedStorage.iterator();
+ }
+
+ public static class TransferredBuffer implements Serializer
+ {
+ private final CellWriter cellWriter;
+ private final AbstractSerializablePairLongObjectColumnHeader<?> columnHeader;
+
+ public TransferredBuffer(
+ CellWriter cellWriter,
+ AbstractSerializablePairLongObjectColumnHeader<?> columnHeader
+ )
+ {
+ this.cellWriter = cellWriter;
+ this.columnHeader = columnHeader;
+ }
+
+ @Override
+ public long getSerializedSize()
+ {
+ return columnHeader.getSerializedSize() + cellWriter.getSerializedSize();
+ }
+
+ @Override
+ public void writeTo(WritableByteChannel channel, FileSmoosher smoosher) throws IOException
+ {
+ columnHeader.transferTo(channel);
+ cellWriter.writeTo(channel, smoosher);
+ }
+ }
Review Comment:
Can we not duplicate this class in the String implementation. `CellWriter` and `AbstractSerializablePairLongObjectColumnHeader` are generic interfaces that are universal to all the implementations.
--
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] ankit0811 commented on pull request #14462: Add support for first/last for double/float/long #10702
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1602815630
@abhishekagarwal87 I missed adding the fold logic in NumericLastVectorAggregator::aggregator
Will update the PR soon
--
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] LakshSingla commented on pull request #14462: Add support for first/last for double/float/long #10702
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1610721980
Thanks @ankit0811 for the PR!
This will also allow MSQ to use these aggregators for ingestion and querying. We should update the MSQ's limitations once this PR is merged in a follow-up PR (after testing).
--
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 #14462: Add support for first/last for double/float/long #10702
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1237788492
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstAggregator.java:
##########
@@ -49,21 +52,46 @@
/**
* Store the current primitive typed 'first' value
*/
- abstract void setCurrentValue();
+ abstract void setFirstValue(ColumnValueSelector valueSelector);
+
+ /**
+ * Store a non-null first value
+ */
+ abstract void setFirstValue(Number firstValue);
@Override
public void aggregate()
{
if (timeSelector.isNull()) {
return;
}
+
+ if (needsFoldCheck) {
+ final Object object = valueSelector.getObject();
+ if (object instanceof SerializablePair) {
+ SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) object;
+
+ if (inPair != null && inPair.lhs < firstTime) {
Review Comment:
## Useless null check
This check is useless. [inPair](1) cannot be null at this check, since it is guarded by [...instanceof...](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/5146)
##########
processing/src/main/java/org/apache/druid/query/aggregation/last/NumericLastAggregator.java:
##########
@@ -19,47 +19,79 @@
package org.apache.druid.query.aggregation.last;
+import org.apache.druid.collections.SerializablePair;
import org.apache.druid.common.config.NullHandling;
import org.apache.druid.query.aggregation.Aggregator;
import org.apache.druid.segment.BaseLongColumnValueSelector;
-import org.apache.druid.segment.BaseNullableColumnValueSelector;
+import org.apache.druid.segment.ColumnValueSelector;
/**
* Base type for on heap 'last' aggregator for primitive numeric column selectors..
*
* This could probably share a base class with {@link org.apache.druid.query.aggregation.first.NumericFirstAggregator}
*/
-public abstract class NumericLastAggregator<TSelector extends BaseNullableColumnValueSelector> implements Aggregator
+public abstract class NumericLastAggregator implements Aggregator
{
private final boolean useDefault = NullHandling.replaceWithDefault();
+
private final BaseLongColumnValueSelector timeSelector;
+ final ColumnValueSelector valueSelector;
+ final boolean needsFoldCheck;
- final TSelector valueSelector;
long lastTime;
boolean rhsNull;
- public NumericLastAggregator(BaseLongColumnValueSelector timeSelector, TSelector valueSelector)
+ public NumericLastAggregator(BaseLongColumnValueSelector timeSelector, ColumnValueSelector valueSelector, boolean needsFoldCheck)
{
this.timeSelector = timeSelector;
this.valueSelector = valueSelector;
+ this.needsFoldCheck = needsFoldCheck;
lastTime = Long.MIN_VALUE;
rhsNull = !useDefault;
}
+ /**
+ * Store the current primitive typed 'last' value
+ */
+ abstract void setLastValue(ColumnValueSelector valueSelector);
+
+ abstract void setLastValue(Number lastValue);
+
@Override
public void aggregate()
{
if (timeSelector.isNull()) {
return;
}
+
+ if (needsFoldCheck) {
+ final Object object = valueSelector.getObject();
+ if (object instanceof SerializablePair) {
+ final SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) object;
+
+ if (inPair != null && inPair.lhs >= lastTime) {
Review Comment:
## Useless null check
This check is useless. [inPair](1) cannot be null at this check, since it is guarded by [...instanceof...](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/5148)
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstBufferAggregator.java:
##########
@@ -89,11 +100,28 @@
if (timeSelector.isNull()) {
return;
}
- long time = timeSelector.getLong();
+
long firstTime = buf.getLong(position);
+ if (needsFoldCheck) {
+ final Object object = valueSelector.getObject();
+ if (object instanceof SerializablePair) {
+ final SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) object;
+ if (inPair != null && inPair.lhs < firstTime) {
Review Comment:
## Useless null check
This check is useless. [inPair](1) cannot be null at this check, since it is guarded by [...instanceof...](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/5147)
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializableLongObjectPairSerde.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.data.input.InputRow;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.column.ColumnBuilder;
+import org.apache.druid.segment.data.GenericIndexed;
+import org.apache.druid.segment.serde.ComplexColumnPartSupplier;
+import org.apache.druid.segment.serde.ComplexMetricExtractor;
+import org.apache.druid.segment.serde.ComplexMetricSerde;
+import org.apache.druid.segment.serde.LargeColumnSupportedComplexColumnSerializer;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+
+public abstract class AbstractSerializableLongObjectPairSerde<T extends SerializablePair<Long, ?>> extends
+ ComplexMetricSerde
+{
+ private final Class<T> pairClass;
+
+ public AbstractSerializableLongObjectPairSerde(Class<T> pairClass)
+ {
+ this.pairClass = pairClass;
+ }
+
+ @Override
+ public ComplexMetricExtractor<?> getExtractor()
+ {
+ return new ComplexMetricExtractor<Object>()
+ {
+ @Override
+ public Class<T> extractedClass()
+ {
+ return pairClass;
+ }
+
+ @Nullable
+ @Override
+ public Object extractValue(InputRow inputRow, String metricName)
+ {
+ return inputRow.getMetric(metricName);
+ }
+ };
+ }
+
+ @Override
+ public void deserializeColumn(ByteBuffer buffer, ColumnBuilder columnBuilder)
+ {
+ final GenericIndexed<?> column = GenericIndexed.read(buffer, getObjectStrategy(), columnBuilder.getFileMapper());
+ columnBuilder.setComplexColumnSupplier(new ComplexColumnPartSupplier(getTypeName(), column));
+ }
+
+ @Override
+ public GenericColumnSerializer<T> getSerializer(SegmentWriteOutMedium segmentWriteOutMedium, String column)
+ {
+ return LargeColumnSupportedComplexColumnSerializer.create(
+ segmentWriteOutMedium,
+ column,
+ getObjectStrategy()
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/5151)
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializableLongObjectPairSerde.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.data.input.InputRow;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.column.ColumnBuilder;
+import org.apache.druid.segment.data.GenericIndexed;
+import org.apache.druid.segment.serde.ComplexColumnPartSupplier;
+import org.apache.druid.segment.serde.ComplexMetricExtractor;
+import org.apache.druid.segment.serde.ComplexMetricSerde;
+import org.apache.druid.segment.serde.LargeColumnSupportedComplexColumnSerializer;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+
+public abstract class AbstractSerializableLongObjectPairSerde<T extends SerializablePair<Long, ?>> extends
+ ComplexMetricSerde
+{
+ private final Class<T> pairClass;
+
+ public AbstractSerializableLongObjectPairSerde(Class<T> pairClass)
+ {
+ this.pairClass = pairClass;
+ }
+
+ @Override
+ public ComplexMetricExtractor<?> getExtractor()
+ {
+ return new ComplexMetricExtractor<Object>()
+ {
+ @Override
+ public Class<T> extractedClass()
+ {
+ return pairClass;
+ }
+
+ @Nullable
+ @Override
+ public Object extractValue(InputRow inputRow, String metricName)
+ {
+ return inputRow.getMetric(metricName);
+ }
+ };
+ }
+
+ @Override
+ public void deserializeColumn(ByteBuffer buffer, ColumnBuilder columnBuilder)
+ {
+ final GenericIndexed<?> column = GenericIndexed.read(buffer, getObjectStrategy(), columnBuilder.getFileMapper());
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/5150)
##########
processing/src/main/java/org/apache/druid/query/aggregation/last/NumericLastBufferAggregator.java:
##########
@@ -92,11 +103,29 @@
if (timeSelector.isNull()) {
return;
}
- long time = timeSelector.getLong();
+
long lastTime = buf.getLong(position);
+ if (needsFoldCheck) {
+ final Object object = valueSelector.getObject();
+ if (object instanceof SerializablePair) {
+ final SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) object;
+
+ if (inPair != null && inPair.lhs >= lastTime) {
Review Comment:
## Useless null check
This check is useless. [inPair](1) cannot be null at this check, since it is guarded by [...instanceof...](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/5149)
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1282323434
##########
processing/src/main/java/org/apache/druid/query/aggregation/last/NumericLastBufferAggregator.java:
##########
@@ -22,7 +22,7 @@
import org.apache.druid.collections.SerializablePair;
import org.apache.druid.common.config.NullHandling;
import org.apache.druid.query.aggregation.BufferAggregator;
-import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.query.monomor:Q:q!phicprocessing.RuntimeShapeInspector;
Review Comment:
Seems like came from vi editor. This needs a change
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1266218893
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -100,27 +106,37 @@ public DoubleFirstAggregatorFactory(
@Override
public Aggregator factorize(ColumnSelectorFactory metricFactory)
{
- final BaseDoubleColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
+ final ColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
if (valueSelector instanceof NilColumnValueSelector) {
return NIL_AGGREGATOR;
} else {
return new DoubleFirstAggregator(
metricFactory.makeColumnValueSelector(timeColumn),
- valueSelector
+ valueSelector,
+ StringFirstLastUtils.selectorNeedsFoldCheck(
+ valueSelector,
+ metricFactory.getColumnCapabilities(fieldName),
+ SerializablePairLongDouble.class
+ )
);
}
}
@Override
public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory)
{
- final BaseDoubleColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
+ final ColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
if (valueSelector instanceof NilColumnValueSelector) {
return NIL_BUFFER_AGGREGATOR;
} else {
return new DoubleFirstBufferAggregator(
metricFactory.makeColumnValueSelector(timeColumn),
- valueSelector
+ valueSelector,
+ StringFirstLastUtils.selectorNeedsFoldCheck(
Review Comment:
Ack. Will create a new numeric util
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1670327082
>There is a better way that the stringLast/First aggregators already use, can we please adjust the ComplexMetricSerde implementations to align with the pattern followed in SerializablePairLongStringComplexMetricSerde instead? Specifically in how that implements getSerializer() and deserializeColumn(). There are code paths for compatibility that use the GenericIndexed stuff, the pattern that we want to follow is the ones that use SerializableLongPairStringColumnSerializer and SerializablePairLongStringComplexColumn.Builder respectively.
Random drive-by comment, but I don't really think that is the best pattern to use here either though, since the pairs here are fixed width and so a much more appropriate way of compressing them that doesn't involve storing offsets separately could be used...
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1779720653
@daniel-imgarena The branch for Druid 28 has been cut, and only release blockers & regressions are allowed at this point. Unfortunately, we won't be seeing this in the 28 release, since it's a new feature.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1266218658
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregator.java:
##########
@@ -19,30 +19,36 @@
package org.apache.druid.query.aggregation.first;
-import org.apache.druid.collections.SerializablePair;
-import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.query.aggregation.SerializablePairLongDouble;
import org.apache.druid.segment.BaseLongColumnValueSelector;
+import org.apache.druid.segment.ColumnValueSelector;
-public class DoubleFirstAggregator extends NumericFirstAggregator<BaseDoubleColumnValueSelector>
+public class DoubleFirstAggregator extends NumericFirstAggregator
{
double firstValue;
- public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, BaseDoubleColumnValueSelector valueSelector)
+ public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, ColumnValueSelector valueSelector, boolean needsFoldCheck)
Review Comment:
I believe so. Let me know if I am mistaken
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1752428533
@ankit0811
```
1 {version=v1, timestamp=2013-08-31T00:00:00.000Z, event={continent=Asia, earliest_user=masterYi, latest_user=stringer, double_first_delta=111.0, double_last_delta=-9.0, long_first_delta=111, long_last_delta=-9, float_first_delta=111.0, float_ last_delta=-9.0}}],
1 {version=v1, timestamp=2013-08-31T00:00:00.000Z, event={continent=Asia, double_first_delta=null, long_last_delta=null, double_last_delta=null, float_first_delta=null, earliest_user=masterYi, long_first_delta=null, latest_user=stringer, float _last_delta=null}}]
```
There are some failing ITs. Can you please take a look at them? Looks like you should be able to identify the culprit after looking at the output diff. (A very wild guess - the new metric isn't hooked up properly perhaps? )
If you need help setting up the ITs for debugging, then please reach out, and I should be able to help with it.
We do want to get this change in, and a green build would expedite the process.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1410155186
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/StringFirstAggregatorFactory.java:
##########
Review Comment:
Sure. I can modify the PR to make StringFirst and Last use the Generic/Abstract classes.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419954009
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectSimpleStagedSerde.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectSimpleStagedSerde<T extends SerializablePair<Long, ?>> implements StagedSerde<T>
+{
+
+ private final Class<?> pairCLass;
+
+ AbstractSerializablePairLongObjectSimpleStagedSerde(Class<?> pairCLass)
+ {
+ this.pairCLass = pairCLass;
+ }
+
+ @Override
+ public StorableBuffer serializeDelayed(
+ @Nullable T value
+ )
+ {
+ if (value == null) {
+ return StorableBuffer.EMPTY;
+ }
+
+ Object rhsObject = value.getRhs();
+
+ return new StorableBuffer()
+ {
+ @Override
+ public void store(ByteBuffer byteBuffer)
+ {
+ Preconditions.checkNotNull(value.getLhs(), String.format(Locale.ENGLISH, "Long in %s must be non-null", pairCLass.getSimpleName()));
+ byteBuffer.putLong(value.getLhs());
+ if (rhsObject != null) {
+ byteBuffer.put(NullHandling.IS_NOT_NULL_BYTE);
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ byteBuffer.putLong((long) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ byteBuffer.putDouble((double) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ byteBuffer.putFloat((float) rhsObject);
+ }
+ } else {
+ byteBuffer.put(NullHandling.IS_NULL_BYTE);
+ }
+ }
+
+ @Override
+ public int getSerializedSize()
+ {
+ int rhsBytes = 0;
+
+ if (rhsObject != null) {
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ rhsBytes = Long.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ rhsBytes = Double.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ rhsBytes = Float.BYTES;
+ }
+ }
+ return Long.BYTES + Byte.BYTES + rhsBytes;
Review Comment:
rhsBytes is assigned to 0 and is assigned value only is `rhsBytes` is non null. So isn't it already conditional?
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1422221961
##########
processing/src/test/java/org/apache/druid/query/aggregation/first/DoubleFirstVectorAggregationTest.java:
##########
@@ -86,39 +91,80 @@ public long[] getLongVector()
@Override
public boolean[] getNullVector()
{
- return NULLS;
+ return null;
}
};
- selector = new BaseDoubleVectorValueSelector(new NoFilterVectorOffset(VALUES.length, 0, VALUES.length)
+ selector = new VectorObjectSelector()
Review Comment:
Curious if this change is required?
--
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] abhishekagarwal87 commented on pull request #14462: Add support for first/last for double/float/long #10702
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1602741033
Thank you @ankit0811 for reviving this PR. I will review this soon.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1287515301
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializableLongObjectPairSerde.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.data.input.InputRow;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.column.ColumnBuilder;
+import org.apache.druid.segment.data.GenericIndexed;
+import org.apache.druid.segment.serde.ComplexColumnPartSupplier;
+import org.apache.druid.segment.serde.ComplexMetricExtractor;
+import org.apache.druid.segment.serde.ComplexMetricSerde;
+import org.apache.druid.segment.serde.LargeColumnSupportedComplexColumnSerializer;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+
+public abstract class AbstractSerializableLongObjectPairSerde<T extends SerializablePair<Long, ?>> extends
+ ComplexMetricSerde
+{
+ private final Class<T> pairClass;
+
+ public AbstractSerializableLongObjectPairSerde(Class<T> pairClass)
+ {
+ this.pairClass = pairClass;
+ }
+
+ @Override
+ public ComplexMetricExtractor<?> getExtractor()
+ {
+ return new ComplexMetricExtractor<Object>()
+ {
+ @Override
+ public Class<T> extractedClass()
+ {
+ return pairClass;
+ }
+
+ @Nullable
+ @Override
+ public Object extractValue(InputRow inputRow, String metricName)
+ {
+ return inputRow.getRaw(metricName);
+ }
+ };
+ }
+
+ @Override
+ public void deserializeColumn(ByteBuffer buffer, ColumnBuilder columnBuilder)
+ {
+ final GenericIndexed<?> column = GenericIndexed.read(buffer, getObjectStrategy(), columnBuilder.getFileMapper());
+ columnBuilder.setComplexColumnSupplier(new ComplexColumnPartSupplier(getTypeName(), column));
+ }
+
+ @Override
+ public GenericColumnSerializer<T> getSerializer(SegmentWriteOutMedium segmentWriteOutMedium, String column)
+ {
+ return LargeColumnSupportedComplexColumnSerializer.create(
+ segmentWriteOutMedium,
+ column,
+ getObjectStrategy()
+ );
+ }
Review Comment:
nit: We can use generics to remove the unchecked type warnings that we are seeing. This will also get rid of the deprecated warning.
```suggestion
}
@Override
public abstract ObjectStrategy<T> getObjectStrategy();
```
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializableLongObjectPairSerde.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.data.input.InputRow;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.column.ColumnBuilder;
+import org.apache.druid.segment.data.GenericIndexed;
+import org.apache.druid.segment.serde.ComplexColumnPartSupplier;
+import org.apache.druid.segment.serde.ComplexMetricExtractor;
+import org.apache.druid.segment.serde.ComplexMetricSerde;
+import org.apache.druid.segment.serde.LargeColumnSupportedComplexColumnSerializer;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+
+public abstract class AbstractSerializableLongObjectPairSerde<T extends SerializablePair<Long, ?>> extends
+ ComplexMetricSerde
+{
+ private final Class<T> pairClass;
+
+ public AbstractSerializableLongObjectPairSerde(Class<T> pairClass)
+ {
+ this.pairClass = pairClass;
+ }
+
+ @Override
+ public ComplexMetricExtractor<?> getExtractor()
+ {
+ return new ComplexMetricExtractor<Object>()
+ {
+ @Override
+ public Class<T> extractedClass()
+ {
+ return pairClass;
+ }
+
+ @Nullable
+ @Override
+ public Object extractValue(InputRow inputRow, String metricName)
+ {
+ return inputRow.getRaw(metricName);
+ }
+ };
+ }
+
+ @Override
+ public void deserializeColumn(ByteBuffer buffer, ColumnBuilder columnBuilder)
+ {
+ final GenericIndexed<?> column = GenericIndexed.read(buffer, getObjectStrategy(), columnBuilder.getFileMapper());
Review Comment:
```suggestion
final GenericIndexed<T> column = GenericIndexed.read(buffer, getObjectStrategy(), columnBuilder.getFileMapper());
```
If we create an abstract method as below, we can get rid of the wildcard
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/StringFirstLastUtils.java:
##########
@@ -57,8 +58,8 @@ public static boolean selectorNeedsFoldCheck(
// Check if the selector class could possibly be a SerializablePairLongString (either a superclass or subclass).
Review Comment:
```suggestion
// Check if the selector class could possibly be a 'pairClass' (either a superclass or subclass).
```
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongFloatComplexMetricSerde.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.data.ObjectStrategy;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Comparator;
+
+public class SerializablePairLongFloatComplexMetricSerde extends AbstractSerializableLongObjectPairSerde<SerializablePairLongFloat>
+{
+ public static final String TYPE_NAME = "serializablePairLongFloat";
+
+ private static final Comparator<SerializablePair<Long, Float>> COMPARATOR = SerializablePair.createNullHandlingComparator(
+ Float::compare,
+ true
+ );
+
+ public SerializablePairLongFloatComplexMetricSerde()
+ {
+ super(SerializablePairLongFloat.class);
+ }
+
+ @Override
+ public String getTypeName()
+ {
+ return TYPE_NAME;
+ }
+
+ @Override
+ public ObjectStrategy<SerializablePairLongFloat> getObjectStrategy()
+ {
+ return new ObjectStrategy<SerializablePairLongFloat>()
+ {
+ @Override
+ public int compare(SerializablePairLongFloat o1, SerializablePairLongFloat o2)
+ {
+ return COMPARATOR.compare(o1, o2);
+ }
+
+ @Override
+ public Class<? extends SerializablePairLongFloat> getClazz()
+ {
+ return SerializablePairLongFloat.class;
+ }
+
+ @Override
+ public SerializablePairLongFloat fromByteBuffer(ByteBuffer buffer, int numBytes)
+ {
+ final ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer();
+ long lhs = readOnlyBuffer.getLong();
+ boolean isNotNull = readOnlyBuffer.get() == NullHandling.IS_NOT_NULL_BYTE;
+ if (isNotNull) {
+ return new SerializablePairLongFloat(lhs, readOnlyBuffer.getFloat());
+ } else {
+ return new SerializablePairLongFloat(lhs, null);
+ }
+ }
+
+ @Override
+ public byte[] toBytes(@Nullable SerializablePairLongFloat inPair)
+ {
+ if (inPair == null) {
+ return new byte[]{};
+ }
+
+ ByteBuffer bbuf = ByteBuffer.allocate(Long.BYTES + Byte.BYTES + Float.BYTES);
Review Comment:
Similar comment as above, in case it is null, we should avoid allocating the space for Float.BYTES. (Should apply for the remaining factories as well)
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongDoubleComplexMetricSerde.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.data.ObjectStrategy;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Comparator;
+
+public class SerializablePairLongDoubleComplexMetricSerde extends AbstractSerializableLongObjectPairSerde<SerializablePairLongDouble>
+{
+ public static final String TYPE_NAME = "serializablePairLongDouble";
+
+ private static final Comparator<SerializablePair<Long, Double>> COMPARATOR = SerializablePair.createNullHandlingComparator(
+ Double::compare,
+ true
+ );
+
+ public SerializablePairLongDoubleComplexMetricSerde()
+ {
+ super(SerializablePairLongDouble.class);
+ }
+
+ @Override
+ public String getTypeName()
+ {
+ return TYPE_NAME;
+ }
+
+ @Override
+ public ObjectStrategy<SerializablePairLongDouble> getObjectStrategy()
+ {
+ return new ObjectStrategy<SerializablePairLongDouble>()
+ {
+ @Override
+ public int compare(SerializablePairLongDouble o1, SerializablePairLongDouble o2)
+ {
+ return COMPARATOR.compare(o1, o2);
+ }
+
+ @Override
+ public Class<? extends SerializablePairLongDouble> getClazz()
+ {
+ return SerializablePairLongDouble.class;
+ }
+
+ @Override
+ public SerializablePairLongDouble fromByteBuffer(ByteBuffer buffer, int numBytes)
+ {
+ final ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer();
+ long lhs = readOnlyBuffer.getLong();
+ boolean isNotNull = readOnlyBuffer.get() == NullHandling.IS_NOT_NULL_BYTE;
+ if (isNotNull) {
+ return new SerializablePairLongDouble(lhs, readOnlyBuffer.getDouble());
+ } else {
+ return new SerializablePairLongDouble(lhs, null);
+ }
+ }
+
+ @Override
+ public byte[] toBytes(@Nullable SerializablePairLongDouble inPair)
+ {
+ if (inPair == null) {
+ return new byte[]{};
+ }
+
+ ByteBuffer bbuf = ByteBuffer.allocate(Long.BYTES + Byte.BYTES + Double.BYTES);
Review Comment:
Bumping up this comment.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1753238045
@LakshSingla this seems to be cos of some changes wrt vectorization. Will try and address this in a couple of days.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419950602
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectSimpleStagedSerde.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectSimpleStagedSerde<T extends SerializablePair<Long, ?>> implements StagedSerde<T>
+{
+
+ private final Class<?> pairCLass;
+
+ AbstractSerializablePairLongObjectSimpleStagedSerde(Class<?> pairCLass)
+ {
+ this.pairCLass = pairCLass;
+ }
+
+ @Override
+ public StorableBuffer serializeDelayed(
+ @Nullable T value
+ )
+ {
+ if (value == null) {
+ return StorableBuffer.EMPTY;
+ }
+
+ Object rhsObject = value.getRhs();
+
+ return new StorableBuffer()
+ {
+ @Override
+ public void store(ByteBuffer byteBuffer)
+ {
+ Preconditions.checkNotNull(value.getLhs(), String.format(Locale.ENGLISH, "Long in %s must be non-null", pairCLass.getSimpleName()));
Review Comment:
Don't need to use String.format.
```suggestion
Preconditions.checkNotNull(value.getLhs(), "Long in %s must be non-null", pairCLass.getSimpleName());
```
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1846601776
@LakshSingla thank you so much for reviewing this PR and being patient throughout the process. Have tried to address your comments let me know if theres any thing more left
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1282709307
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregator.java:
##########
@@ -19,30 +19,36 @@
package org.apache.druid.query.aggregation.first;
-import org.apache.druid.collections.SerializablePair;
-import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.query.aggregation.SerializablePairLongDouble;
import org.apache.druid.segment.BaseLongColumnValueSelector;
+import org.apache.druid.segment.ColumnValueSelector;
-public class DoubleFirstAggregator extends NumericFirstAggregator<BaseDoubleColumnValueSelector>
+public class DoubleFirstAggregator extends NumericFirstAggregator
{
double firstValue;
- public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, BaseDoubleColumnValueSelector valueSelector)
+ public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, ColumnValueSelector valueSelector, boolean needsFoldCheck)
Review Comment:
@somu-imply I am not completely aware of the foldCheck, which implementation of the selectors produces results that cannot be optimized? Also, is there any particular reason that double won't require the check, since it seems analogous to what StringFirst et al. are doing?
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1639471268
> Thanks for raising this PR!! I am relatively unfamiliar with the complex column's serde and am still going through the implementation, however, I have a few high-level comments, based on my understanding of the StringFirst/Last aggregators:
>
> 1. In the `deserializeColumn`, we should allocate the first byte for a version number. This would help if we decide to update the strategy down the line and want backward compatibility.
> 2. The current StringFirst/Last comparators use delta encoding and compression on the numeric column to reduce the size. Also, the PR ([Improve String Last/First Storage Efficiency #12879](https://github.com/apache/druid/pull/12879)) which introduced this change also added a few classes which help in this. I suppose that we would want it sometime. The versioning as mentioned above would help in making these changes if we decide that we don't want it now. However, the complexity of changing from one version to another is high and we require backward compatibility therefore we should question if that should be done in the original PR itself (check this comment: https://github.com/apache/druid/blob/d63eff3b1b6b15fac1bb278a784ce6c391a52ce2/processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongStringComplexMetricSerde.java#L54-L66
> )
> 3. Can we refactor the preexisting LongString serde classes to be subclasses of the newly introduced `AbstractSerializableLongObjectPairSerde`? I see that there are some similarities in the methods. In any case, it would be helpful if all of them are under the same umbrella, and we can further categorize the newly added classes as children of `AbstractSerializableLongNumbericPairSerde`
Best if we take this in a separate PR? else it will be too big a PR to be reviewed. It already touches quite a few classes
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419954009
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectSimpleStagedSerde.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectSimpleStagedSerde<T extends SerializablePair<Long, ?>> implements StagedSerde<T>
+{
+
+ private final Class<?> pairCLass;
+
+ AbstractSerializablePairLongObjectSimpleStagedSerde(Class<?> pairCLass)
+ {
+ this.pairCLass = pairCLass;
+ }
+
+ @Override
+ public StorableBuffer serializeDelayed(
+ @Nullable T value
+ )
+ {
+ if (value == null) {
+ return StorableBuffer.EMPTY;
+ }
+
+ Object rhsObject = value.getRhs();
+
+ return new StorableBuffer()
+ {
+ @Override
+ public void store(ByteBuffer byteBuffer)
+ {
+ Preconditions.checkNotNull(value.getLhs(), String.format(Locale.ENGLISH, "Long in %s must be non-null", pairCLass.getSimpleName()));
+ byteBuffer.putLong(value.getLhs());
+ if (rhsObject != null) {
+ byteBuffer.put(NullHandling.IS_NOT_NULL_BYTE);
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ byteBuffer.putLong((long) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ byteBuffer.putDouble((double) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ byteBuffer.putFloat((float) rhsObject);
+ }
+ } else {
+ byteBuffer.put(NullHandling.IS_NULL_BYTE);
+ }
+ }
+
+ @Override
+ public int getSerializedSize()
+ {
+ int rhsBytes = 0;
+
+ if (rhsObject != null) {
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ rhsBytes = Long.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ rhsBytes = Double.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ rhsBytes = Float.BYTES;
+ }
+ }
+ return Long.BYTES + Byte.BYTES + rhsBytes;
Review Comment:
`rhsBytes` is assigned to 0 and is assigned value only if `rhsBytes` is non null. So isn't it already conditional?
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1849759414
Can you add a release note for the patch
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "daniel-imgarena (via GitHub)" <gi...@apache.org>.
daniel-imgarena commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1777757168
Will we see this in Druid 28? I see myself back on the need for a doubleLast or longFirst for way too long
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1281535560
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/FloatFirstAggregatorFactory.java:
##########
@@ -281,7 +236,7 @@ public byte[] getCacheKey()
public ColumnType getIntermediateType()
{
// if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde
Review Comment:
Wont this throw an exception as `AggregateFactory::getIntermediateType` default implementation calls `getType` which throws `UnsupportedOperationException` and looks like the doc says `getType` will be deprecated in support of `getIntermediateType`
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419952447
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectSimpleStagedSerde.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectSimpleStagedSerde<T extends SerializablePair<Long, ?>> implements StagedSerde<T>
+{
+
+ private final Class<?> pairCLass;
+
+ AbstractSerializablePairLongObjectSimpleStagedSerde(Class<?> pairCLass)
+ {
+ this.pairCLass = pairCLass;
+ }
+
+ @Override
+ public StorableBuffer serializeDelayed(
+ @Nullable T value
+ )
+ {
+ if (value == null) {
+ return StorableBuffer.EMPTY;
+ }
+
+ Object rhsObject = value.getRhs();
+
+ return new StorableBuffer()
+ {
+ @Override
+ public void store(ByteBuffer byteBuffer)
+ {
+ Preconditions.checkNotNull(value.getLhs(), String.format(Locale.ENGLISH, "Long in %s must be non-null", pairCLass.getSimpleName()));
+ byteBuffer.putLong(value.getLhs());
+ if (rhsObject != null) {
+ byteBuffer.put(NullHandling.IS_NOT_NULL_BYTE);
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ byteBuffer.putLong((long) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ byteBuffer.putDouble((double) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ byteBuffer.putFloat((float) rhsObject);
+ }
+ } else {
+ byteBuffer.put(NullHandling.IS_NULL_BYTE);
+ }
+ }
+
+ @Override
+ public int getSerializedSize()
+ {
+ int rhsBytes = 0;
+
+ if (rhsObject != null) {
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ rhsBytes = Long.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ rhsBytes = Double.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ rhsBytes = Float.BYTES;
+ }
+ }
+ return Long.BYTES + Byte.BYTES + rhsBytes;
Review Comment:
```suggestion
return Long.BYTES + Byte.BYTES + (rhsObject == null ? 0 : rhsBytes);
```
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1423429737
##########
processing/src/test/java/org/apache/druid/query/aggregation/first/DoubleFirstVectorAggregationTest.java:
##########
@@ -86,39 +91,80 @@ public long[] getLongVector()
@Override
public boolean[] getNullVector()
{
- return NULLS;
+ return null;
}
};
- selector = new BaseDoubleVectorValueSelector(new NoFilterVectorOffset(VALUES.length, 0, VALUES.length)
+ selector = new VectorObjectSelector()
Review Comment:
Yes. It will be required as the selector can now be of either VectorObject (pair) or VectorValue (numeric).
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1423429737
##########
processing/src/test/java/org/apache/druid/query/aggregation/first/DoubleFirstVectorAggregationTest.java:
##########
@@ -86,39 +91,80 @@ public long[] getLongVector()
@Override
public boolean[] getNullVector()
{
- return NULLS;
+ return null;
}
};
- selector = new BaseDoubleVectorValueSelector(new NoFilterVectorOffset(VALUES.length, 0, VALUES.length)
+ selector = new VectorObjectSelector()
Review Comment:
Yes. It will be required as the selector can now be of either VectorObject or VectorValue.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1423445323
##########
processing/src/test/java/org/apache/druid/query/aggregation/first/DoubleFirstVectorAggregationTest.java:
##########
@@ -86,39 +91,80 @@ public long[] getLongVector()
@Override
public boolean[] getNullVector()
{
- return NULLS;
+ return null;
}
};
- selector = new BaseDoubleVectorValueSelector(new NoFilterVectorOffset(VALUES.length, 0, VALUES.length)
+ selector = new VectorObjectSelector()
Review Comment:
Thanks for the explanation
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1356140288
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -110,20 +135,45 @@
int positionOffset
)
{
- boolean[] nulls = useDefault ? null : valueSelector.getNullVector();
- long[] timeVector = timeSelector.getLongVector();
+ final long[] timeVector = timeSelector.getLongVector();
+ final Object[] objectsWhichMightBeNumeric = valueSelector.getObjectVector();
+ final boolean[] nullValueVector = FirstLastUtils.getNullVector(objectsWhichMightBeNumeric);
+ boolean[] nulls = useDefault ? null : nullValueVector;
+
+ // iterate once over the object vector to find first non null element and
+ // determine if the type is Pair or not
+ boolean foldNeeded = false;
+ for (Object obj : objectsWhichMightBeNumeric) {
+ if (obj != null) {
+ foldNeeded = FirstLastUtils.objectNeedsFoldCheck(obj, pairClass);
+ break;
+ }
+ }
for (int i = 0; i < numRows; i++) {
int position = positions[i] + positionOffset;
int row = rows == null ? i : rows[i];
- long firstTime = buf.getLong(position);
- if (timeVector[row] < firstTime) {
- if (useDefault || nulls == null || !nulls[row]) {
- updateTimeWithValue(buf, position, timeVector[row], row);
+ firstTime = buf.getLong(position);
+
+ if (foldNeeded) {
+ final SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) objectsWhichMightBeNumeric[row];
+ if (useDefault || inPair != null) {
+ if (inPair.lhs < firstTime) {
+ updateTimeWithValue(buf, position, inPair.lhs, inPair.rhs);
+ }
} else {
- updateTimeWithNull(buf, position, timeVector[row]);
+ updateTimeWithNull(buf, position, inPair.lhs);
Review Comment:
## Dereferenced variable is always null
Variable [inPair](1) is always null at this dereference.
[Show more details](https://github.com/apache/druid/security/code-scanning/5888)
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -110,20 +135,45 @@
int positionOffset
)
{
- boolean[] nulls = useDefault ? null : valueSelector.getNullVector();
- long[] timeVector = timeSelector.getLongVector();
+ final long[] timeVector = timeSelector.getLongVector();
+ final Object[] objectsWhichMightBeNumeric = valueSelector.getObjectVector();
+ final boolean[] nullValueVector = FirstLastUtils.getNullVector(objectsWhichMightBeNumeric);
+ boolean[] nulls = useDefault ? null : nullValueVector;
+
+ // iterate once over the object vector to find first non null element and
+ // determine if the type is Pair or not
+ boolean foldNeeded = false;
+ for (Object obj : objectsWhichMightBeNumeric) {
+ if (obj != null) {
+ foldNeeded = FirstLastUtils.objectNeedsFoldCheck(obj, pairClass);
+ break;
+ }
+ }
for (int i = 0; i < numRows; i++) {
int position = positions[i] + positionOffset;
int row = rows == null ? i : rows[i];
- long firstTime = buf.getLong(position);
- if (timeVector[row] < firstTime) {
- if (useDefault || nulls == null || !nulls[row]) {
- updateTimeWithValue(buf, position, timeVector[row], row);
+ firstTime = buf.getLong(position);
+
+ if (foldNeeded) {
+ final SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) objectsWhichMightBeNumeric[row];
+ if (useDefault || inPair != null) {
+ if (inPair.lhs < firstTime) {
Review Comment:
## Dereferenced variable may be null
Variable [inPair](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/5889)
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1411617207
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -142,12 +160,24 @@ public VectorAggregator factorizeVector(
VectorColumnSelectorFactory columnSelectorFactory
)
{
+ VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(timeColumn);
ColumnCapabilities capabilities = columnSelectorFactory.getColumnCapabilities(fieldName);
+
if (Types.isNumeric(capabilities)) {
VectorValueSelector valueSelector = columnSelectorFactory.makeValueSelector(fieldName);
- VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(
- timeColumn);
- return new DoubleFirstVectorAggregator(timeSelector, valueSelector);
+ VectorObjectSelector objectSelector = ExpressionVectorSelectors.castValueSelectorToObject(
+ columnSelectorFactory.getReadableVectorInspector(),
+ fieldName,
+ valueSelector,
+ capabilities.toColumnType(),
+ ColumnType.DOUBLE
+ );
+ return new DoubleFirstVectorAggregator(timeSelector, objectSelector);
+ }
Review Comment:
With the string factory, we'd need to check if it is of a numeric type and cast if so. But if it is a numeric, then it shouldn't be the case, at least for DOUBLE objects. Also, if it is a String aggregator, now we return a DoubleFirstVectorAggregator (if capabilities correspond to String capabilities).
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419952274
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectDeltaEncodedStagedSerde.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import com.google.common.primitives.Ints;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectDeltaEncodedStagedSerde<T extends SerializablePair<Long, ?>> implements
+ StagedSerde<T>
+{
+
+ final long minValue;
+ final boolean useIntegerDelta;
+ private final Class<?> pairClass;
+
+ AbstractSerializablePairLongObjectDeltaEncodedStagedSerde(
+ long minValue,
+ boolean useIntegerDelta,
+ Class<?> pairClass
+ )
+ {
+ this.minValue = minValue;
+ this.useIntegerDelta = useIntegerDelta;
+ this.pairClass = pairClass;
+ }
+
+ @Override
+ public StorableBuffer serializeDelayed(@Nullable T value)
+ {
+ if (value == null) {
+ return StorableBuffer.EMPTY;
+ }
+
+ Object rhsObject = value.getRhs();
+
+ return new StorableBuffer()
+ {
+ @Override
+ public void store(ByteBuffer byteBuffer)
+ {
+ Preconditions.checkNotNull(value.lhs, String.format(Locale.ENGLISH, "Long in %s must be non-null", pairClass.getSimpleName()));
+
+ long delta = value.lhs - minValue;
+
+ Preconditions.checkState(delta >= 0 || delta == value.lhs);
+
+ if (useIntegerDelta) {
+ byteBuffer.putInt(Ints.checkedCast(delta));
+ } else {
+ byteBuffer.putLong(delta);
+ }
+
+ if (rhsObject != null) {
+ byteBuffer.put(NullHandling.IS_NOT_NULL_BYTE);
+ if (pairClass.isAssignableFrom(SerializablePairLongLong.class)) {
+ byteBuffer.putLong((long) rhsObject);
+ } else if (pairClass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ byteBuffer.putDouble((double) rhsObject);
+ } else if (pairClass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ byteBuffer.putFloat((float) rhsObject);
+ }
+ } else {
+ byteBuffer.put(NullHandling.IS_NULL_BYTE);
+ }
+ }
+
+ @Override
+ public int getSerializedSize()
+ {
+ int rhsBytes = 0;
+
+ if (rhsObject != null) {
+ if (pairClass.isAssignableFrom(SerializablePairLongLong.class)) {
+ rhsBytes = Long.BYTES;
+ } else if (pairClass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ rhsBytes = Double.BYTES;
+ } else if (pairClass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ rhsBytes = Float.BYTES;
+ }
+ }
+
+ return (useIntegerDelta ? Integer.BYTES : Long.BYTES) + Byte.BYTES + rhsBytes;
Review Comment:
```suggestion
return (useIntegerDelta ? Integer.BYTES : Long.BYTES) + Byte.BYTES + (rhsObject == null ? 0 : rhsBytes);
```
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1851352255
I still believe that we can clean up the code a bit further if we get rid of the SerializablePairLongString abstractions and keep them as generics, however, I don't know what entails such a change. That would allow the reuse of multiple classes. However, that will be taken in a follow up PR, so its a go-ahead from my side.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1356444427
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -110,18 +135,44 @@
int positionOffset
)
{
- boolean[] nulls = useDefault ? null : valueSelector.getNullVector();
- long[] timeVector = timeSelector.getLongVector();
+ final long[] timeVector = timeSelector.getLongVector();
+ final Object[] objectsWhichMightBeNumeric = valueSelector.getObjectVector();
+ final boolean[] nullValueVector = FirstLastUtils.getNullVector(objectsWhichMightBeNumeric);
+ boolean[] nulls = useDefault ? null : nullValueVector;
+
+ // iterate once over the object vector to find first non null element and
+ // determine if the type is Pair or not
+ boolean foldNeeded = false;
+ for (Object obj : objectsWhichMightBeNumeric) {
+ if (obj != null) {
+ foldNeeded = FirstLastUtils.objectNeedsFoldCheck(obj, pairClass);
+ break;
+ }
+ }
for (int i = 0; i < numRows; i++) {
int position = positions[i] + positionOffset;
int row = rows == null ? i : rows[i];
- long firstTime = buf.getLong(position);
- if (timeVector[row] < firstTime) {
- if (useDefault || nulls == null || !nulls[row]) {
- updateTimeWithValue(buf, position, timeVector[row], row);
- } else {
- updateTimeWithNull(buf, position, timeVector[row]);
+ firstTime = buf.getLong(position);
+
+ if (foldNeeded) {
+ final SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) objectsWhichMightBeNumeric[row];
+ if (useDefault || inPair != null) {
+ if (inPair.lhs != null && inPair.lhs < firstTime) {
Review Comment:
## Dereferenced variable may be null
Variable [inPair](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/5890)
##########
processing/src/main/java/org/apache/druid/query/aggregation/last/NumericLastVectorAggregator.java:
##########
@@ -113,19 +135,46 @@
int positionOffset
)
{
+ if (timeSelector == null) {
+ return;
+ }
+
+ final long[] timeVector = timeSelector.getLongVector();
- boolean[] nulls = useDefault ? null : valueSelector.getNullVector();
- long[] timeVector = timeSelector.getLongVector();
+ final Object[] objectsWhichMightBeNumeric = valueSelector.getObjectVector();
+ boolean[] nulls = useDefault ? null : FirstLastUtils.getNullVector(objectsWhichMightBeNumeric);
+
+ boolean foldNeeded = false;
+ for (Object obj : objectsWhichMightBeNumeric) {
+ if (obj != null) {
+ foldNeeded = FirstLastUtils.objectNeedsFoldCheck(obj, pairClass);
+ break;
+ }
+ }
for (int i = 0; i < numRows; i++) {
int position = positions[i] + positionOffset;
int row = rows == null ? i : rows[i];
long lastTime = buf.getLong(position);
- if (timeVector[row] >= lastTime) {
- if (useDefault || nulls == null || !nulls[row]) {
- updateTimeWithValue(buf, position, timeVector[row], row);
- } else {
- updateTimeWithNull(buf, position, timeVector[row]);
+
+ if (foldNeeded) {
+ final SerializablePair<Long, Number> inPair = (SerializablePair<Long, Number>) objectsWhichMightBeNumeric[row];
+ if (useDefault || inPair != null) {
+ if (inPair.lhs != null && inPair.lhs >= lastTime) {
Review Comment:
## Dereferenced variable may be null
Variable [inPair](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/5891)
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1688938248
@imply-cheddar a qq for clarifications since this part of code is new to me
1. the current `SerializablePairLongStringComplexColumn.Builder` has a version check. I am assuming this is not required for long numeric type implementation. is the assumption correct here?
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1641320445
> Best if we take this in a separate PR? else it will be too big a PR to be reviewed. It already touches quite a few classes
Seems reasonable to me, we can mark this as an improvement once we get the functionality in.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1849757761
Thanks for the patch @ankit0811.
I think there's some follow up that can be done, in this area
1. Make sure string pairs work as expected with null & empty strings - This has been a regression before the patch, and I have raised a PR for the same
2. Make the EARLIEST and the LATEST work with SQL
3. Add tests for MSQ
4. Look for some cleaner abstractions
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1393650692
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongDoubleComplexMetricSerde.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.segment.GenericColumnSerializer;
+import org.apache.druid.segment.column.ColumnBuilder;
+import org.apache.druid.segment.data.ObjectStrategy;
+import org.apache.druid.segment.serde.cell.NativeClearedByteBufferProvider;
+import org.apache.druid.segment.writeout.SegmentWriteOutMedium;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Comparator;
+
+public class SerializablePairLongDoubleComplexMetricSerde extends AbstractSerializableLongObjectPairSerde<SerializablePairLongDouble>
Review Comment:
There should be a version associated with the serde, at the first position Check out the string implementation which does this. This will help in future iterations of the serde, without making any backward incompatible changes.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1393669010
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -142,12 +160,24 @@ public VectorAggregator factorizeVector(
VectorColumnSelectorFactory columnSelectorFactory
)
{
+ VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(timeColumn);
ColumnCapabilities capabilities = columnSelectorFactory.getColumnCapabilities(fieldName);
+
if (Types.isNumeric(capabilities)) {
VectorValueSelector valueSelector = columnSelectorFactory.makeValueSelector(fieldName);
- VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(
- timeColumn);
- return new DoubleFirstVectorAggregator(timeSelector, valueSelector);
+ VectorObjectSelector objectSelector = ExpressionVectorSelectors.castValueSelectorToObject(
+ columnSelectorFactory.getReadableVectorInspector(),
+ fieldName,
+ valueSelector,
+ capabilities.toColumnType(),
+ ColumnType.DOUBLE
+ );
+ return new DoubleFirstVectorAggregator(timeSelector, objectSelector);
+ }
Review Comment:
I am not sure if this is correct. If the type is numeric, why do we need an explicit cast to `Double`. I could be wrong, but this seems kinda suspicious.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -164,7 +215,7 @@ void updateTimeWithNull(ByteBuffer buf, int position, long time)
* Abstract function which needs to be overridden by subclasses to set the
* latest value in the buffer depending on the datatype
*/
- abstract void putValue(ByteBuffer buf, int position, int index);
+ abstract void putValue(ByteBuffer buf, int position, Number number);
Review Comment:
Let's revert this change if we can. It will lead to some duplication, however, this will lead to the boxing of the primitives, which is what the column selectors aim to reduce (if they can). Therefore there are separate methods like `isNull` + `getLong` which are preferred instead of `getObject` because the former doesn't auto-box the primitive variable to an object.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -132,14 +183,14 @@ public void aggregate(
*
* @param buf byte buffer storing the byte array representation of the aggregate
* @param position offset within the byte buffer at which the current aggregate value is stored
- * @param time the time to be updated in the buffer as the last time
- * @param index the index of the vectorized vector which is the last value
+ * @param time the time to be updated in the buffer as the first time
+ * @param number number which is the first value
*/
- void updateTimeWithValue(ByteBuffer buf, int position, long time, int index)
+ void updateTimeWithValue(ByteBuffer buf, int position, long time, Number number)
Review Comment:
It might not be as clean as the current code, however, there shouldn't be a lot of duplication.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -132,14 +183,14 @@ public void aggregate(
*
* @param buf byte buffer storing the byte array representation of the aggregate
* @param position offset within the byte buffer at which the current aggregate value is stored
- * @param time the time to be updated in the buffer as the last time
- * @param index the index of the vectorized vector which is the last value
+ * @param time the time to be updated in the buffer as the first time
+ * @param number number which is the first value
*/
- void updateTimeWithValue(ByteBuffer buf, int position, long time, int index)
+ void updateTimeWithValue(ByteBuffer buf, int position, long time, Number number)
Review Comment:
Autoboxing alert, as mentioned in the other comment. Let's see if we can get away without this change.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -142,12 +160,24 @@ public VectorAggregator factorizeVector(
VectorColumnSelectorFactory columnSelectorFactory
)
{
+ VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(timeColumn);
ColumnCapabilities capabilities = columnSelectorFactory.getColumnCapabilities(fieldName);
+
if (Types.isNumeric(capabilities)) {
VectorValueSelector valueSelector = columnSelectorFactory.makeValueSelector(fieldName);
- VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(
- timeColumn);
- return new DoubleFirstVectorAggregator(timeSelector, valueSelector);
+ VectorObjectSelector objectSelector = ExpressionVectorSelectors.castValueSelectorToObject(
+ columnSelectorFactory.getReadableVectorInspector(),
+ fieldName,
+ valueSelector,
+ capabilities.toColumnType(),
+ ColumnType.DOUBLE
+ );
+ return new DoubleFirstVectorAggregator(timeSelector, objectSelector);
+ }
+
+ VectorObjectSelector vSelector = columnSelectorFactory.makeObjectSelector(fieldName);
+ if (capabilities != null) {
+ return new DoubleFirstVectorAggregator(timeSelector, vSelector);
Review Comment:
Why is this check required? I don't think we are using the capabilities inside the aggregator
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "ankit0811 (via GitHub)" <gi...@apache.org>.
ankit0811 commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1411612349
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -142,12 +160,24 @@ public VectorAggregator factorizeVector(
VectorColumnSelectorFactory columnSelectorFactory
)
{
+ VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(timeColumn);
ColumnCapabilities capabilities = columnSelectorFactory.getColumnCapabilities(fieldName);
+
if (Types.isNumeric(capabilities)) {
VectorValueSelector valueSelector = columnSelectorFactory.makeValueSelector(fieldName);
- VectorValueSelector timeSelector = columnSelectorFactory.makeValueSelector(
- timeColumn);
- return new DoubleFirstVectorAggregator(timeSelector, valueSelector);
+ VectorObjectSelector objectSelector = ExpressionVectorSelectors.castValueSelectorToObject(
+ columnSelectorFactory.getReadableVectorInspector(),
+ fieldName,
+ valueSelector,
+ capabilities.toColumnType(),
+ ColumnType.DOUBLE
+ );
+ return new DoubleFirstVectorAggregator(timeSelector, objectSelector);
+ }
Review Comment:
This is the same code flow as StringFirstAggregatorFactory.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1670295363
Generally speaking, I don't believe that we should merge the code with the `GenericIndexed` method of generating the columns. That's going to create the need to always support that. We have a better way to store things (the way that the String versions do it) and we shoudl follow those instead. If we really want to do these in two PRs, I'd suggest leaving this PR as is and create another PR on top of it to switch the serde around and then merge them in quick succession.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1663369149
Thanks for resolving the merge conflicts and the comments. I'll test it from MSQ's perspective and check if it works fine with MSQ queries in a couple of days.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1262785892
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongDoubleComplexMetricSerde.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.aggregation;
+
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.data.ObjectStrategy;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Comparator;
+
+public class SerializablePairLongDoubleComplexMetricSerde extends AbstractSerializableLongObjectPairSerde<SerializablePairLongDouble>
+{
+ public static final String TYPE_NAME = "serializablePairLongDouble";
+
+ private static final Comparator<SerializablePair<Long, Double>> COMPARATOR = SerializablePair.createNullHandlingComparator(
+ Double::compare,
+ true
+ );
+
+ public SerializablePairLongDoubleComplexMetricSerde()
+ {
+ super(SerializablePairLongDouble.class);
+ }
+
+ @Override
+ public String getTypeName()
+ {
+ return TYPE_NAME;
+ }
+
+ @Override
+ public ObjectStrategy<SerializablePairLongDouble> getObjectStrategy()
+ {
+ return new ObjectStrategy<SerializablePairLongDouble>()
+ {
+ @Override
+ public int compare(SerializablePairLongDouble o1, SerializablePairLongDouble o2)
+ {
+ return COMPARATOR.compare(o1, o2);
+ }
+
+ @Override
+ public Class<? extends SerializablePairLongDouble> getClazz()
+ {
+ return SerializablePairLongDouble.class;
+ }
+
+ @Override
+ public SerializablePairLongDouble fromByteBuffer(ByteBuffer buffer, int numBytes)
+ {
+ final ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer();
+ long lhs = readOnlyBuffer.getLong();
+ boolean isNotNull = readOnlyBuffer.get() == NullHandling.IS_NOT_NULL_BYTE;
+ if (isNotNull) {
+ return new SerializablePairLongDouble(lhs, readOnlyBuffer.getDouble());
+ } else {
+ return new SerializablePairLongDouble(lhs, null);
+ }
+ }
+
+ @Override
+ public byte[] toBytes(@Nullable SerializablePairLongDouble inPair)
+ {
+ if (inPair == null) {
+ return new byte[]{};
+ }
+
+ ByteBuffer bbuf = ByteBuffer.allocate(Long.BYTES + Byte.BYTES + Double.BYTES);
Review Comment:
If it is a null do we need to allocate the byte buffer to an additional double as we are not adding anything there. While doing the get depending on the null byte we can access the double.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregator.java:
##########
@@ -19,30 +19,36 @@
package org.apache.druid.query.aggregation.first;
-import org.apache.druid.collections.SerializablePair;
-import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.query.aggregation.SerializablePairLongDouble;
import org.apache.druid.segment.BaseLongColumnValueSelector;
+import org.apache.druid.segment.ColumnValueSelector;
-public class DoubleFirstAggregator extends NumericFirstAggregator<BaseDoubleColumnValueSelector>
+public class DoubleFirstAggregator extends NumericFirstAggregator
{
double firstValue;
- public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, BaseDoubleColumnValueSelector valueSelector)
+ public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, ColumnValueSelector valueSelector, boolean needsFoldCheck)
{
- super(timeSelector, valueSelector);
+ super(timeSelector, valueSelector, needsFoldCheck);
firstValue = 0;
}
@Override
- void setCurrentValue()
+ void setFirstValue(ColumnValueSelector valueSelector)
Review Comment:
Do we need the valueSelector here as an arg ? Since it is already available
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -100,27 +106,37 @@ public DoubleFirstAggregatorFactory(
@Override
public Aggregator factorize(ColumnSelectorFactory metricFactory)
{
- final BaseDoubleColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
+ final ColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
if (valueSelector instanceof NilColumnValueSelector) {
return NIL_AGGREGATOR;
} else {
return new DoubleFirstAggregator(
metricFactory.makeColumnValueSelector(timeColumn),
- valueSelector
+ valueSelector,
+ StringFirstLastUtils.selectorNeedsFoldCheck(
+ valueSelector,
+ metricFactory.getColumnCapabilities(fieldName),
+ SerializablePairLongDouble.class
+ )
);
}
}
@Override
public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory)
{
- final BaseDoubleColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
+ final ColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName);
if (valueSelector instanceof NilColumnValueSelector) {
return NIL_BUFFER_AGGREGATOR;
} else {
return new DoubleFirstBufferAggregator(
metricFactory.makeColumnValueSelector(timeColumn),
- valueSelector
+ valueSelector,
+ StringFirstLastUtils.selectorNeedsFoldCheck(
Review Comment:
This this is dealing with numeric capabilities probably we should move away from StringFirstLastUtils and create a separate utils for numeric. Also the fold check can be avoided
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregator.java:
##########
@@ -19,30 +19,36 @@
package org.apache.druid.query.aggregation.first;
-import org.apache.druid.collections.SerializablePair;
-import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.query.aggregation.SerializablePairLongDouble;
import org.apache.druid.segment.BaseLongColumnValueSelector;
+import org.apache.druid.segment.ColumnValueSelector;
-public class DoubleFirstAggregator extends NumericFirstAggregator<BaseDoubleColumnValueSelector>
+public class DoubleFirstAggregator extends NumericFirstAggregator
{
double firstValue;
- public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, BaseDoubleColumnValueSelector valueSelector)
+ public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, ColumnValueSelector valueSelector, boolean needsFoldCheck)
Review Comment:
Does double first need a fold check ?
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1280848989
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/FloatFirstAggregatorFactory.java:
##########
@@ -281,7 +236,7 @@ public byte[] getCacheKey()
public ColumnType getIntermediateType()
{
// if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde
Review Comment:
This can be removed from all the numeric factories
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419951849
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectDeltaEncodedStagedSerde.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import com.google.common.primitives.Ints;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectDeltaEncodedStagedSerde<T extends SerializablePair<Long, ?>> implements
+ StagedSerde<T>
+{
+
+ final long minValue;
+ final boolean useIntegerDelta;
+ private final Class<?> pairClass;
+
+ AbstractSerializablePairLongObjectDeltaEncodedStagedSerde(
+ long minValue,
+ boolean useIntegerDelta,
+ Class<?> pairClass
+ )
+ {
+ this.minValue = minValue;
+ this.useIntegerDelta = useIntegerDelta;
+ this.pairClass = pairClass;
+ }
+
+ @Override
+ public StorableBuffer serializeDelayed(@Nullable T value)
+ {
+ if (value == null) {
+ return StorableBuffer.EMPTY;
+ }
+
+ Object rhsObject = value.getRhs();
+
+ return new StorableBuffer()
+ {
+ @Override
+ public void store(ByteBuffer byteBuffer)
+ {
+ Preconditions.checkNotNull(value.lhs, String.format(Locale.ENGLISH, "Long in %s must be non-null", pairClass.getSimpleName()));
+
+ long delta = value.lhs - minValue;
+
+ Preconditions.checkState(delta >= 0 || delta == value.lhs);
+
+ if (useIntegerDelta) {
+ byteBuffer.putInt(Ints.checkedCast(delta));
+ } else {
+ byteBuffer.putLong(delta);
+ }
+
+ if (rhsObject != null) {
+ byteBuffer.put(NullHandling.IS_NOT_NULL_BYTE);
+ if (pairClass.isAssignableFrom(SerializablePairLongLong.class)) {
+ byteBuffer.putLong((long) rhsObject);
+ } else if (pairClass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ byteBuffer.putDouble((double) rhsObject);
+ } else if (pairClass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ byteBuffer.putFloat((float) rhsObject);
+ }
+ } else {
+ byteBuffer.put(NullHandling.IS_NULL_BYTE);
+ }
+ }
+
+ @Override
+ public int getSerializedSize()
+ {
+ int rhsBytes = 0;
+
+ if (rhsObject != null) {
+ if (pairClass.isAssignableFrom(SerializablePairLongLong.class)) {
+ rhsBytes = Long.BYTES;
+ } else if (pairClass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ rhsBytes = Double.BYTES;
+ } else if (pairClass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ rhsBytes = Float.BYTES;
+ }
+ }
+
+ return (useIntegerDelta ? Integer.BYTES : Long.BYTES) + Byte.BYTES + rhsBytes;
Review Comment:
This should be conditional on whether the rhsObject is null or not.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1419951566
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectSimpleStagedSerde.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.aggregation;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.collections.SerializablePair;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.segment.serde.cell.StagedSerde;
+import org.apache.druid.segment.serde.cell.StorableBuffer;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+
+public abstract class AbstractSerializablePairLongObjectSimpleStagedSerde<T extends SerializablePair<Long, ?>> implements StagedSerde<T>
+{
+
+ private final Class<?> pairCLass;
+
+ AbstractSerializablePairLongObjectSimpleStagedSerde(Class<?> pairCLass)
+ {
+ this.pairCLass = pairCLass;
+ }
+
+ @Override
+ public StorableBuffer serializeDelayed(
+ @Nullable T value
+ )
+ {
+ if (value == null) {
+ return StorableBuffer.EMPTY;
+ }
+
+ Object rhsObject = value.getRhs();
+
+ return new StorableBuffer()
+ {
+ @Override
+ public void store(ByteBuffer byteBuffer)
+ {
+ Preconditions.checkNotNull(value.getLhs(), String.format(Locale.ENGLISH, "Long in %s must be non-null", pairCLass.getSimpleName()));
+ byteBuffer.putLong(value.getLhs());
+ if (rhsObject != null) {
+ byteBuffer.put(NullHandling.IS_NOT_NULL_BYTE);
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ byteBuffer.putLong((long) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ byteBuffer.putDouble((double) rhsObject);
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ byteBuffer.putFloat((float) rhsObject);
+ }
+ } else {
+ byteBuffer.put(NullHandling.IS_NULL_BYTE);
+ }
+ }
+
+ @Override
+ public int getSerializedSize()
+ {
+ int rhsBytes = 0;
+
+ if (rhsObject != null) {
+ if (pairCLass.isAssignableFrom(SerializablePairLongLong.class)) {
+ rhsBytes = Long.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongDouble.class)) {
+ rhsBytes = Double.BYTES;
+ } else if (pairCLass.isAssignableFrom(SerializablePairLongFloat.class)) {
+ rhsBytes = Float.BYTES;
+ }
+ }
+ return Long.BYTES + Byte.BYTES + rhsBytes;
Review Comment:
This computation is incorrect. For case when rhsObject is null, we'd only be storing `Long.BYTES + Byte.BYTES`, so this should be conditional.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1851357987
SInce we have identified the caveats, we can merge this and work on it in a follow up PR.
Thanks @ankit0811 and @LakshSingla for the PR and reviews.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #14462:
URL: https://github.com/apache/druid/pull/14462
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1393622850
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/StringFirstAggregatorFactory.java:
##########
Review Comment:
Can this also use the `GenericFirstAggregateCombiner` that you have created, and we can remove the `StringFirstAggregateCombiner`?
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/FirstLastUtils.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.aggregation.first;
+
+import org.apache.druid.segment.BaseObjectColumnValueSelector;
+import org.apache.druid.segment.NilColumnValueSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+
+public class FirstLastUtils
+{
+
+ /**
+ * Returns whether a given value selector *might* contain SerializablePairLongString objects.
Review Comment:
This comment is obsolete since this is not limited to the Pair<Long, String> object. Also, it would be good if Javadoc mentioned what the "fold" check means.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregator.java:
##########
@@ -19,30 +19,36 @@
package org.apache.druid.query.aggregation.first;
-import org.apache.druid.collections.SerializablePair;
-import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.query.aggregation.SerializablePairLongDouble;
import org.apache.druid.segment.BaseLongColumnValueSelector;
+import org.apache.druid.segment.ColumnValueSelector;
-public class DoubleFirstAggregator extends NumericFirstAggregator<BaseDoubleColumnValueSelector>
+public class DoubleFirstAggregator extends NumericFirstAggregator
{
double firstValue;
- public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, BaseDoubleColumnValueSelector valueSelector)
+ public DoubleFirstAggregator(BaseLongColumnValueSelector timeSelector, ColumnValueSelector valueSelector, boolean needsFoldCheck)
Review Comment:
Still confused as to why doubles wouldn't require a folds check.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/FirstLastUtils.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.aggregation.first;
+
+import org.apache.druid.segment.BaseObjectColumnValueSelector;
+import org.apache.druid.segment.NilColumnValueSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+
+public class FirstLastUtils
+{
+
+ /**
+ * Returns whether a given value selector *might* contain SerializablePairLongString objects.
+ */
+ public static boolean selectorNeedsFoldCheck(
+ final BaseObjectColumnValueSelector<?> valueSelector,
+ @Nullable final ColumnCapabilities valueSelectorCapabilities,
+ Class pairClass
+ )
+ {
+ if (valueSelectorCapabilities != null && !valueSelectorCapabilities.is(ValueType.COMPLEX)) {
+ // Known, non-complex type.
+ return false;
+ }
+
+ if (valueSelector instanceof NilColumnValueSelector) {
+ // Nil column, definitely no SerializablePairLongStrings.
+ return false;
+ }
+
+ // Check if the selector class could possibly be a SerializablePairLongString (either a superclass or subclass).
+ final Class<?> clazz = valueSelector.classOfObject();
+ return clazz.isAssignableFrom(pairClass)
+ || pairClass.isAssignableFrom(clazz);
+ }
+
+ /**
+ * Returns whether an object *might* contain SerializablePairLongString objects.
Review Comment:
Outdated javadoc
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/FirstLastUtils.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.aggregation.first;
+
+import org.apache.druid.segment.BaseObjectColumnValueSelector;
+import org.apache.druid.segment.NilColumnValueSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+
+public class FirstLastUtils
+{
+
+ /**
+ * Returns whether a given value selector *might* contain SerializablePairLongString objects.
+ */
+ public static boolean selectorNeedsFoldCheck(
+ final BaseObjectColumnValueSelector<?> valueSelector,
+ @Nullable final ColumnCapabilities valueSelectorCapabilities,
+ Class pairClass
+ )
+ {
+ if (valueSelectorCapabilities != null && !valueSelectorCapabilities.is(ValueType.COMPLEX)) {
+ // Known, non-complex type.
+ return false;
+ }
+
+ if (valueSelector instanceof NilColumnValueSelector) {
+ // Nil column, definitely no SerializablePairLongStrings.
+ return false;
+ }
+
+ // Check if the selector class could possibly be a SerializablePairLongString (either a superclass or subclass).
+ final Class<?> clazz = valueSelector.classOfObject();
+ return clazz.isAssignableFrom(pairClass)
+ || pairClass.isAssignableFrom(clazz);
+ }
+
+ /**
+ * Returns whether an object *might* contain SerializablePairLongString objects.
+ */
+ public static boolean objectNeedsFoldCheck(Object obj, Class pairClass)
+ {
+ if (obj == null) {
+ return false;
+ }
+ final Class<?> clazz = obj.getClass();
+ return clazz.isAssignableFrom(pairClass)
+ || pairClass.isAssignableFrom(clazz);
+ }
+
+
+ public static boolean[] getNullVector(Object[] objectVector)
Review Comment:
Please add the following annotation because it might not be intuitive that it can return null. Also, can you please add a javadoc describing the behaviour for the same.
```suggestion
@Nullable
public static boolean[] getNullVector(Object[] objectVector)
```
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongStringColumnSerializer.java:
##########
Review Comment:
This should also inherit from the `AbstractSerializablePairLongObjectColumnSerializer` class.
##########
processing/src/main/java/org/apache/druid/query/aggregation/AbstractSerializablePairLongObjectBufferStore.java:
##########
Review Comment:
The String methods could also inherit from this class I suppose.
--
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 support for first/last for double/float/long #10702 (druid)
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on PR #14462:
URL: https://github.com/apache/druid/pull/14462#issuecomment-1670262656
This PR doesn't actually solve hte problem it needs to solve as the columns that itproduces will be extremely large compared to the expected size. The complex column serde needs to follow the pattern followed by `SerializedPairLongStringComplexMetricSerde`. It's not okay to use the normal `GenericIndexed` for these columns. While I'm not against merging this just to get something in place, I am against this code existing in any release as it will mean that some people might actually create segments with incredibly large columns, not realizing why, and hten we have to carry forward the bad versions of the code to read those columns back out all because we just didn't implement it according to the already existing pattern the first time around.
--
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