You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "LakshSingla (via GitHub)" <gi...@apache.org> on 2023/08/23 18:30:44 UTC
[PR] Field writers for numerical arrays (druid)
LakshSingla opened a new pull request, #14900:
URL: https://github.com/apache/druid/pull/14900
### Description
This PR introduces the `FieldWriters` to write numerical arrays into Frames. This will allow the arrays to be written into row-based frames, which are currently being used by the MSQ engine.
The end goal is to extend this to `FrameColumnWriters` as well in follow-up patches so that the broker that can use Frame (column-based) to materialize the subquery results can also write and read numerical array columns.
The PR refactors the existing `long`, `double`, and `float` - `FieldWriters` and `FieldReaders` - to subclass from `NumericalFieldWriter` and `NumericalFieldReader`.
The new `NumericalArrayFieldWriter` and `NumericalArrayFieldReader` reuse these classes to write the numerical array.
The format of the array value is written into the field as:
1. 1st byte is reserved for whether the array is null or not.
2. If the array is not null, we write the individual values into the memory consecutively using the preexisting field writers. There's one caveat, the null value is denoted with a 0x01 flag instead of 0x00 (which is used in normal numeric field writers), and the non-null value is denoted with a 0x02 flag instead of 0x01 (which is used in normal numeric field writers). This is done because it allows the field comparison per byte.
3. We terminate the array with 0x00.
For example:
1. The getObject() returns `null`
| Bytes | Value | Interpretation |
|--------|--------|--------|
| 1 | 0x00 | Denotes that the array null |
2. The getObject returns `[5L, null, 6L]`
| Bytes | Value | Interpretation |
|--------|--------|--------|
| 1 | 0x01 | Denotes that the array is not null |
| 2 | 0x02 | Denotes that the next element is not null |
| 3-10 | transform(5) | Representation of 5 |
| 11 | 0x01 | Denotes that the next element is null |
| 12-19 | transform(0) | Representation of 0 (default value, the reader will ignore it if SqlCompatible mode is on |
| 20 | 0x02 | Denotes that the next element is not null |
| 21-28 | transform(6) | Representation of 6 |
| 29 | 0x00 | End of array |
#### Fixed the bug ...
#### Renamed the class ...
#### Added a forbidden-apis entry ...
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Field writers for numerical arrays (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1341495434
##########
processing/src/main/java/org/apache/druid/frame/write/FrameWriterUtils.java:
##########
@@ -172,6 +174,40 @@ public static List<ByteBuffer> getUtf8ByteBuffersFromStringArraySelector(
return retVal;
}
+ @Nullable
+ public static List<? extends Number> getNumericArrayFromNumericArray(Object row)
Review Comment:
This method needs some javadoc, because the name is unclear. ("Numeric array from numeric array")
Also, reading through the impl I think that `numericArrayAsList` would be a better name.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344098524
##########
processing/src/main/java/org/apache/druid/frame/field/DoubleArrayFieldReader.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reader for fields written by {@link NumericArrayFieldWriter#getDoubleArrayFieldWriter}
+ */
+public class DoubleArrayFieldReader extends NumericArrayFieldReader
+{
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer
+ )
+ {
+ return new Selector<Double>(memory, fieldPointer)
+ {
+
+ @Nullable
+ @Override
+ public Double getIndividualValueAtMemory(Memory memory, long position)
+ {
+ ColumnValueSelector<?> columnValueSelector =
+ DoubleFieldReader.forArray()
+ .makeColumnValueSelector(memory, new ConstantFieldPointer(position));
Review Comment:
Lemme check if I can reduce these object creations post addressing the other review comments.. If it requires a minor refactor, I'll update it 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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14900:
URL: https://github.com/apache/druid/pull/14900#issuecomment-1746984808
Verified that it works on a Druid cluster. The IT failures look unrelated
Thanks for the reviews @clintropolis @cryptoe @gianm
<img width="1206" alt="Screenshot 2023-10-04 at 7 52 31 PM" src="https://github.com/apache/druid/assets/30999375/cae20550-f770-4c15-8cf1-7b25230ef24b">
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344788699
##########
processing/src/test/java/org/apache/druid/frame/field/FloatArrayFieldReaderTest.java:
##########
@@ -0,0 +1,224 @@
+/*
+ * 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.frame.field;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.testing.InitializedNullHandlingTest;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class FloatArrayFieldReaderTest extends InitializedNullHandlingTest
+{
+ private static final long MEMORY_POSITION = 1;
+
+ @Rule
+ public MockitoRule mockitoRule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
+
+ @Mock
+ public ColumnValueSelector writeSelector;
+
+ private WritableMemory memory;
+ private FieldWriter fieldWriter;
+
+ //CHECKSTYLE.OFF: Regexp
+ private static final Object[] FLOATS_ARRAY_1 = new Object[]{
Review Comment:
Do you mean something like `[[1,2],[3,4]]`? It'd make little sense to do that since the numeric fields cannot be arbitrarily nested.
Therefore the fact that we have created a `FloatArrayFieldReader` means that we are reading something written by `FloatArrayFieldWriter`which ensures that we get an array of type [1,2,3,4,....].
If we'd had nested array like `[[1,2],[3,4]]`, we wouldn't have created the writer and the reader in the first place, since the signature would have been `Array<Array<Long>>, and we wouldn't be in the methods tested by this suite.
--
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] Field writers for numerical arrays (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1343861521
##########
processing/src/main/java/org/apache/druid/frame/field/FieldReaders.java:
##########
@@ -61,8 +60,22 @@ public static FieldReader create(final String columnName, final ColumnType colum
return ComplexFieldReader.createFromType(columnType);
case ARRAY:
- if (columnType.getElementType().getType() == ValueType.STRING) {
- return new StringFieldReader(true);
+ switch (Preconditions.checkNotNull(columnType.getElementType().getType(), "array elementType")) {
+ case STRING:
+ return new StringFieldReader(true);
Review Comment:
unrelated to this PR side note, i wish we split string array field reader out of string field reader, even if its only a vanity name for the same class
##########
processing/src/main/java/org/apache/druid/frame/field/FieldWriters.java:
##########
@@ -64,23 +64,38 @@ public static FieldWriter create(
}
switch (columnType.getType()) {
+
Review Comment:
nit: why all the extra spaces?
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Reader class for the fields written by {@link NumericArrayFieldWriter}. See the Javadoc for the writer for more
+ * information on the format
+ *
+ * The numeric array fields are byte comparable
+ */
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ throw DruidException.defensive("Cannot call makeDimensionSelector on field of type ARRAY");
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract static class Selector<T extends Number> implements ColumnValueSelector
+ {
+ private final Memory memory;
+ private final ReadableFieldPointer fieldPointer;
+
+ private long currentFieldPosition = -1;
+
+ private final List<T> currentRow = new ArrayList<>();
+ private boolean currentRowIsNull;
+
+ public Selector(final Memory memory, final ReadableFieldPointer fieldPointer)
+ {
+ this.memory = memory;
+ this.fieldPointer = fieldPointer;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ final List<T> currentArray = computeCurrentArray();
+
+ if (currentArray == null) {
+ return null;
+ }
+
+ return currentArray.toArray();
+ }
+
+ @Override
+ public Class classOfObject()
+ {
+ return Object.class;
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return 0;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return 0;
+ }
+
+ @Override
+ public long getLong()
+ {
+ return 0;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ long position = fieldPointer.position();
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
Review Comment:
nit: i don't think you technically need to implement this since it is only used in practice with `getDouble`/`getFloat`/`getLong`. Things that call `getObject` just call `getObject` and check if the result is null.
That said, since you are using it internally to check for nulls, i suppose it doesn't really hurt
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Reader class for the fields written by {@link NumericArrayFieldWriter}. See the Javadoc for the writer for more
+ * information on the format
+ *
+ * The numeric array fields are byte comparable
+ */
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ throw DruidException.defensive("Cannot call makeDimensionSelector on field of type ARRAY");
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract static class Selector<T extends Number> implements ColumnValueSelector
Review Comment:
+1, also maybe a more descriptive name for the class, and maybe name for the generic parameter, `TElement` or something perhaps?
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
+ * <p>
+ * Examples:
+ * 1. null
+ * | Bytes | Value | Interpretation |
+ * |--------|-------|-----------------------------|
+ * | 1 | 0x00 | Denotes that the array null |
+ * <p>
+ * 2. [] (empty array)
+ * | Bytes | Value | Interpretation |
+ * |--------|----- -|------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x00 | End of the array |
+ * <p>
+ * 3. [5L, null, 6L]
+ * | Bytes | Value | Interpretation |
+ * |---------|--------------|-----------------------------------------------------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x02 | Denotes that the next element is not null |
+ * | 3-10 | transform(5) | Representation of 5 |
+ * | 11 | 0x01 | Denotes that the next element is null |
+ * | 12-19 | transform(0) | Representation of 0 (default value, the reader will ignore it if SqlCompatible mode is on |
+ * | 20 | 0x02 | Denotes that the next element is not null |
+ * | 21-28 | transform(6) | Representation of 6 |
+ * | 29 | 0x00 | End of array |
+ */
+public class NumericArrayFieldWriter implements FieldWriter
+{
+
+ /**
+ * Denotes that the array itself is null
+ */
+ public static final byte NULL_ROW = 0x00;
+
+ /**
+ * Denotes that the array is non null
+ */
+ public static final byte NON_NULL_ROW = 0x01;
+
+ /**
+ * Marks the end of the array. Since {@link #NULL_ROW} and {@link #ARRAY_TERMINATOR} will only occur at different
+ * locations, therefore there is no clash in keeping both's values at 0x00
+ */
+ public static final byte ARRAY_TERMINATOR = 0x00;
+
+ private final ColumnValueSelector selector;
+ private final NumericFieldWriterFactory writerFactory;
+
+ /**
+ * Returns the writer for ARRAY<LONG>
+ */
+ public static NumericArrayFieldWriter getLongArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, LongFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<FLOAT>
+ */
+ public static NumericArrayFieldWriter getFloatArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, FloatFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<DOUBLE>
+ */
+ public static NumericArrayFieldWriter getDoubleArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, DoubleFieldWriter::forArray);
+ }
+
+ public NumericArrayFieldWriter(final ColumnValueSelector selector, NumericFieldWriterFactory writerFactory)
+ {
+ this.selector = selector;
+ this.writerFactory = writerFactory;
+ }
+
+ @Override
+ public long writeTo(WritableMemory memory, long position, long maxSize)
+ {
+ Object row = selector.getObject();
+ if (row == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ } else {
+
+ List<? extends Number> list = FrameWriterUtils.getNumericArrayFromNumericArray(row);
+
+ if (list == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ }
+
+ // Create a columnValueSelector to write the individual elements re-using the NumericFieldWriter
+ AtomicInteger index = new AtomicInteger(0);
+ ColumnValueSelector<Number> columnValueSelector = new ColumnValueSelector<Number>()
+ {
+ @Override
+ public double getDouble()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.doubleValue() : 0d;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.floatValue() : 0f;
+ }
+
+ @Override
+ public long getLong()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.longValue() : 0L;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ // Arrays preserve the individual element's nullity when they are written and read.
+ // Therefore, when working with SQL incompatible mode, [7, null] won't change to [7, 0] when written to and
+ // read from the underlying serialization (as compared with the primitives). Therefore,
+ // even when NullHandling.replaceWithDefault() is true we need to write null as is, and not convert it to their
+ // default value when writing the array. Therefore, the check is `getObject() == null` ignoring the value of
+ // `NullHandling.replaceWithDefaul()`.
+ return getObject() == null;
Review Comment:
`NullHandling.replaceWithDefault` is dead to me, but this is true that array columns and expressions do not coerce elements to zeros.
We could also consider only supporting arrays in sql compatible mode :p
##########
processing/src/main/java/org/apache/druid/frame/field/DoubleArrayFieldReader.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reader for fields written by {@link NumericArrayFieldWriter#getDoubleArrayFieldWriter}
+ */
+public class DoubleArrayFieldReader extends NumericArrayFieldReader
+{
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer
+ )
+ {
+ return new Selector<Double>(memory, fieldPointer)
+ {
+
+ @Nullable
+ @Override
+ public Double getIndividualValueAtMemory(Memory memory, long position)
+ {
+ ColumnValueSelector<?> columnValueSelector =
+ DoubleFieldReader.forArray()
+ .makeColumnValueSelector(memory, new ConstantFieldPointer(position));
Review Comment:
this seems kind of expensive, like we make a column value selector and a constant pointer for every element of every row? surely there is a less garbage intense way to read the array elements, though doesn't necessarily need to be resolved in this PR...
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldWriter.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+/**
+ * FieldWriter for numeric datatypes. The parent class does the null handling for the underlying data, while
+ * the individual subclasses write the individual element (long, float or double type). This also allows for a clean
+ * reuse of the readers and writers between the numeric types and also allowing the array writers ({@link NumericArrayFieldWriter})
+ * to use these methods directly without duplication
+ *
+ * Format:
+ * - 1 byte: Whether the following value is null or not. Take a look at the note on the indicator bytes.
+ * - X bytes: Encoded value of the selector, or the default value if it is null. X denotes the size of the numeric value
+ *
+ * Indicator bytes for denoting whether the element is null or not null changes depending on whether the writer is used
+ * to write the data for individual value (like LONG) or for an element of an array (like ARRAY<LONG>). This is because
+ * array support for the numeric types was added later and by then the field writers for individual fields were using
+ * 0x00 to denote the null byte, which is reserved for denoting the array end when we are writing the elements as part
+ * of the array instead. (0x00 is used for array end because it helps in preserving the byte comparison property of the
+ * numeric array field writers).
+ *
+ * Therefore, to preserve backward and forward compatibility, the individual element's writers were left unchanged,
+ * while the array's element's writers used 0x01 and 0x02 to denote null and non-null byte respectively
+ *
+ * Values produced by the writer are sortable without decoding
+ */
+public abstract class NumericFieldWriter implements FieldWriter
+{
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is null. This is used in the primitive
+ * writers. NULL_BYTE < NOT_NULL_BYTE to preserve the ordering while doing byte comparison
+ */
+ public static final byte NULL_BYTE = 0x00;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is not null. This is used in the primitive
+ * writers
+ */
+ public static final byte NOT_NULL_BYTE = 0x01;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is null. This is used while writing the individual
+ * elements writers of an array. ARRAY_ELEMENT_NULL_BYTE < ARRAY_ELEMENT_NOT_NULL_BYTE to preserve the ordering
+ * while doing byte comparison
+ */
+ public static final byte ARRAY_ELEMENT_NULL_BYTE = 0x01;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is not null. This is used while writing the individual
+ * elements writers of an array
+ */
+ public static final byte ARRAY_ELEMENT_NOT_NULL_BYTE = 0x02;
Review Comment:
any reason not to make this the array terminator then can just use the same null/not null bytes for elements as for regular numbers?
--
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] Field writers for numerical arrays (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1341495434
##########
processing/src/main/java/org/apache/druid/frame/write/FrameWriterUtils.java:
##########
@@ -172,6 +174,40 @@ public static List<ByteBuffer> getUtf8ByteBuffersFromStringArraySelector(
return retVal;
}
+ @Nullable
+ public static List<? extends Number> getNumericArrayFromNumericArray(Object row)
Review Comment:
This method needs some javadoc, because the name is unclear. ("Numeric array from numeric array"? Sounds like it doesn't do anything)
Also, reading through the impl I think that `numericArrayAsList` would be a better name.
##########
processing/src/test/java/org/apache/druid/frame/field/FloatArrayFieldReaderTest.java:
##########
@@ -0,0 +1,224 @@
+/*
+ * 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.frame.field;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.testing.InitializedNullHandlingTest;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class FloatArrayFieldReaderTest extends InitializedNullHandlingTest
Review Comment:
Please include a test case with `Float.NaN`, and a test case with with a `null` mixed in with nonnull doubles. (I see one for `null` by itself, but I don't see a mixture of null and nonnull.) Only include the `null` if `NullHandling.sqlCompatible()` though.
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReader.java:
##########
@@ -96,11 +115,28 @@ public static FrameReader create(final RowSignature signature)
signature.getColumnName(columnNumber)
);
- columnReaders.add(FrameColumnReaders.create(columnNumber, columnType));
fieldReaders.add(FieldReaders.create(signature.getColumnName(columnNumber), columnType));
+
+ // If we encounter a numeric array type, then don't throw the error immediately since the reader can be used to
+ // read only the ROW_BASED frames. Rather, set the optional, and throw the appropriate error message when the reader
+ // tries to read COLUMNAR frame. This should go away once the COLUMNAR frames also start supporting the numeric
+ // array
+ if (columnType.getType() == ValueType.ARRAY
Review Comment:
nit: the conditional would be more readable if the element type was extracted from the condition.
##########
processing/src/test/java/org/apache/druid/frame/field/DoubleArrayFieldReaderTest.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.frame.field;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.testing.InitializedNullHandlingTest;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class DoubleArrayFieldReaderTest extends InitializedNullHandlingTest
Review Comment:
Please include a test case with `Double.NaN`, and a test case with with a `null` mixed in with nonnull doubles. (I see one for `null` by itself, but I don't see a mixture of null and nonnull.) Only include the `null` if `NullHandling.sqlCompatible()` though.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldWriter.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+/**
+ * FieldWriter for numeric datatypes. The parent class does the null handling for the underlying data, while
+ * the individual subclasses write the individual element (long, float or double type). This also allows for a clean
+ * reuse of the readers and writers between the numeric types and also allowing the array writers ({@link NumericArrayFieldWriter})
+ * to use these methods directly without duplication
+ *
+ * Format:
+ * - 1 byte: Whether the following value is null or not. Take a look at the note on the indicator bytes.
+ * - X bytes: Encoded value of the selector, or the default value if it is null. X denotes the size of the numeric value
+ *
+ * Indicator bytes for denoting whether the element is null or not null changes depending on whether the writer is used
+ * to write the data for individual value (like LONG) or for an element of an array (like ARRAY<LONG>). This is because
+ * array support for the numeric types was added later and by then the field writers for individual fields were using
+ * 0x00 to denote the null byte, which is reserved for denoting the array end when we are writing the elements as part
+ * of the array instead. (0x00 is used for array end because it helps in preserving the byte comparison property of the
+ * numeric array field writers).
+ *
+ * Therefore, to preserve backward and forward compatibility, the individual element's writers were left unchanged,
+ * while the array's element's writers used 0x01 and 0x02 to denote null and non-null byte respectively
+ *
+ * Values produced by the writer are sortable without decoding
+ */
Review Comment:
Please include `@see NumericFieldWriter for format details` on `DoubleFieldReader`, `DoubleFieldWriter`, etc, so people can find it easily.
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReader.java:
##########
@@ -57,15 +61,28 @@ public class FrameReader
// Field readers, for row-based frames.
private final List<FieldReader> fieldReaders;
+ /**
+ * Currently, only ROW_BASED frames support numerical array columns, while the COLUMNAR frames donot. While creating
+ * a FrameReader, for types unsupported by COLUMNAR frames, we populate this field to denote that the FrameReader is
+ * "incomplete" and can't be used to read the columnar frame. However, the FrameReader performs as expected for the
+ * row-based frames.
+ * In short, this is a temporary measure till columnar frames support the numerical array types to punt the unsupported
+ * type check for the numerical arrays (for COLUMNAR frames only) at the usage time, rather than the creation time
+ */
+ private final Optional<Pair<String, ColumnType>> unsupportedColumnAndType;
Review Comment:
Instead of this structure, IMO it'd keep `FrameReader` cleaner if we create a dummy column reader for the numeric array columns, which throws exceptions on `readColumn` and `readRACColumn`. Then the errors would happen only when we attempt to access those columns.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Reader class for the fields written by {@link NumericArrayFieldWriter}. See the Javadoc for the writer for more
+ * information on the format
+ *
+ * The numeric array fields are byte comparable
+ */
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ throw DruidException.defensive("Cannot call makeDimensionSelector on field of type ARRAY");
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract static class Selector<T extends Number> implements ColumnValueSelector
Review Comment:
Please add some javadoc explaining the purpose of this class, including javadoc for the methods `getIndividualValueAtMemory` and `getIndividualFieldSize`. It's pretty twisty-turny to read through the code due to the inheritance stack, so additional javadoc helps the reader figure out what is going on.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344124503
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReader.java:
##########
@@ -96,11 +115,28 @@ public static FrameReader create(final RowSignature signature)
signature.getColumnName(columnNumber)
);
- columnReaders.add(FrameColumnReaders.create(columnNumber, columnType));
fieldReaders.add(FieldReaders.create(signature.getColumnName(columnNumber), columnType));
+
+ // If we encounter a numeric array type, then don't throw the error immediately since the reader can be used to
+ // read only the ROW_BASED frames. Rather, set the optional, and throw the appropriate error message when the reader
+ // tries to read COLUMNAR frame. This should go away once the COLUMNAR frames also start supporting the numeric
+ // array
+ if (columnType.getType() == ValueType.ARRAY
Review Comment:
Will remove the conditional altogether, and add a dummy column reader.
--
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] Field writers for numerical arrays (druid)
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1303445579
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ // TODO: Should I throw an exception here
+ return DimensionSelector.nilSelector();
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract static class Selector<T extends Number> implements ColumnValueSelector
+ {
+ private final Memory memory;
+ private final ReadableFieldPointer fieldPointer;
+
+ private long currentFieldPosition = -1;
+
+ private final List<T> currentRow = new ArrayList<>();
+ private boolean currentRowIsNull;
+
+ public Selector(final Memory memory, final ReadableFieldPointer fieldPointer)
+ {
+ this.memory = memory;
+ this.fieldPointer = fieldPointer;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ final List<T> currentArray = computeCurrentArray();
+
+ if (currentArray == null) {
+ return null;
+ }
+
+ return currentArray.toArray();
+ }
+
+ @Override
+ public Class classOfObject()
+ {
+ return Object.class;
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return 0;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return 0;
+ }
+
+ @Override
+ public long getLong()
+ {
+ return 0;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ long position = fieldPointer.position();
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ public abstract T getIndividualValueAtMemory(Memory memory, long position);
+
+ public abstract int getIndividualFieldSize();
+
+ @Nullable
+ private List<T> computeCurrentArray()
+ {
+ final long fieldPosition = fieldPointer.position();
+
+ if (fieldPosition != currentFieldPosition) {
+ updateCurrentArray(fieldPosition);
+ }
+
+ this.currentFieldPosition = fieldPosition;
+
+ if (currentRowIsNull) {
+ return null;
+ }
+ return currentRow;
+
+ }
+
+ private void updateCurrentArray(final long fieldPosition)
+ {
+ currentRow.clear();
+ currentRowIsNull = false;
+
+ long position = fieldPosition;
+ long limit = memory.getCapacity();
+
+ if (isNull()) {
+ currentRowIsNull = true;
+ return;
+ }
+
+ position++;
Review Comment:
## User-controlled data in arithmetic expression
This arithmetic expression depends on a [user-provided value](1), potentially causing an overflow.
This arithmetic expression depends on a [user-provided value](2), potentially causing an overflow.
This arithmetic expression depends on a [user-provided value](3), potentially causing an overflow.
This arithmetic expression depends on a [user-provided value](4), potentially causing an overflow.
This arithmetic expression depends on a [user-provided value](5), potentially causing an overflow.
[Show more details](https://github.com/apache/druid/security/code-scanning/5729)
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla merged PR #14900:
URL: https://github.com/apache/druid/pull/14900
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344071256
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldWriter.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+/**
+ * FieldWriter for numeric datatypes. The parent class does the null handling for the underlying data, while
+ * the individual subclasses write the individual element (long, float or double type). This also allows for a clean
+ * reuse of the readers and writers between the numeric types and also allowing the array writers ({@link NumericArrayFieldWriter})
+ * to use these methods directly without duplication
+ *
+ * Format:
+ * - 1 byte: Whether the following value is null or not. Take a look at the note on the indicator bytes.
+ * - X bytes: Encoded value of the selector, or the default value if it is null. X denotes the size of the numeric value
+ *
+ * Indicator bytes for denoting whether the element is null or not null changes depending on whether the writer is used
+ * to write the data for individual value (like LONG) or for an element of an array (like ARRAY<LONG>). This is because
+ * array support for the numeric types was added later and by then the field writers for individual fields were using
+ * 0x00 to denote the null byte, which is reserved for denoting the array end when we are writing the elements as part
+ * of the array instead. (0x00 is used for array end because it helps in preserving the byte comparison property of the
+ * numeric array field writers).
+ *
+ * Therefore, to preserve backward and forward compatibility, the individual element's writers were left unchanged,
+ * while the array's element's writers used 0x01 and 0x02 to denote null and non-null byte respectively
+ *
+ * Values produced by the writer are sortable without decoding
+ */
+public abstract class NumericFieldWriter implements FieldWriter
+{
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is null. This is used in the primitive
+ * writers. NULL_BYTE < NOT_NULL_BYTE to preserve the ordering while doing byte comparison
+ */
+ public static final byte NULL_BYTE = 0x00;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is not null. This is used in the primitive
+ * writers
+ */
+ public static final byte NOT_NULL_BYTE = 0x01;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is null. This is used while writing the individual
+ * elements writers of an array. ARRAY_ELEMENT_NULL_BYTE < ARRAY_ELEMENT_NOT_NULL_BYTE to preserve the ordering
+ * while doing byte comparison
+ */
+ public static final byte ARRAY_ELEMENT_NULL_BYTE = 0x01;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is not null. This is used while writing the individual
+ * elements writers of an array
+ */
+ public static final byte ARRAY_ELEMENT_NOT_NULL_BYTE = 0x02;
Review Comment:
Checkout the comment here: https://github.com/apache/druid/pull/14900/files#diff-49d317f8e9b064164fbad1b4424e78acf2ff7c96757744918ad7a965dc296d6cR35-R42
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14900:
URL: https://github.com/apache/druid/pull/14900#issuecomment-1746986907
There are some static check failures, that I was about to miss. Fixing them
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1345484923
##########
sql/src/test/java/org/apache/druid/sql/calcite/QueryTestRunner.java:
##########
@@ -667,7 +674,9 @@ public QueryTestRunner(QueryTestBuilder builder)
verifySteps.add(new VerifyNativeQueries(finalExecStep));
}
if (builder.expectedResultsVerifier != null) {
- verifySteps.add(new VerifyResults(finalExecStep));
+ // Don't verify the row signature when MSQ is running, since the broker receives the task id, and the signature
+ // would be {TASK:STRING} instead of the expected results signature
+ verifySteps.add(new VerifyResults(finalExecStep, !config.isRunningMSQ()));
Review Comment:
Yep, I wanted to separate out the row signature check and the results check, though chose not to keep it in this PR.
--
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] Field writers for numerical arrays (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1345648332
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQInsertTest.java:
##########
@@ -1463,6 +1464,193 @@ public void testInsertArraysAutoType() throws IOException
.verifyResults();
}
+ @Test
+ public void testInsertArrays() throws IOException
+ {
+ List<Object[]> expectedRows = Arrays.asList(
+ new Object[]{
+ 1672531200000L,
+ null,
+ null,
+ new Object[]{1L, 2L, 3L},
+ new Object[]{},
+ new Object[]{1.1d, 2.2d, 3.3d},
+ null
+ },
+ new Object[]{
+ 1672531200000L,
+ null,
+ Arrays.asList("a", "b"),
+ null,
+ new Object[]{2L, 3L},
+ null,
+ new Object[]{null}
+ },
+ new Object[]{
+ 1672531200000L,
+ Arrays.asList("a", "b"),
+ null,
+ null,
+ new Object[]{null, 2L, 9L},
+ null,
+ new Object[]{999.0d, 5.5d, null}
+ },
+ new Object[]{
+ 1672531200000L,
+ Arrays.asList("a", "b"),
+ Arrays.asList("a", "b"),
+ new Object[]{1L, 2L, 3L},
+ new Object[]{1L, null, 3L},
+ new Object[]{1.1d, 2.2d, 3.3d},
+ new Object[]{1.1d, 2.2d, null}
+ },
+ new Object[]{
+ 1672531200000L,
+ Arrays.asList("a", "b", "c"),
+ Arrays.asList(null, "b"),
+ new Object[]{2L, 3L},
+ null,
+ new Object[]{3.3d, 4.4d, 5.5d},
+ new Object[]{999.0d, null, 5.5d}
+ },
+ new Object[]{
+ 1672531200000L,
+ Arrays.asList("b", "c"),
+ Arrays.asList("d", null, "b"),
+ new Object[]{1L, 2L, 3L, 4L},
+ new Object[]{1L, 2L, 3L},
+ new Object[]{1.1d, 3.3d},
+ new Object[]{null, 2.2d, null}
+ },
+ new Object[]{
+ 1672531200000L,
+ Arrays.asList("d", "e"),
+ Arrays.asList("b", "b"),
+ new Object[]{1L, 4L},
+ new Object[]{1L},
+ new Object[]{2.2d, 3.3d, 4.0d},
+ null
+ },
+ new Object[]{
+ 1672617600000L,
+ null,
+ null,
+ new Object[]{1L, 2L, 3L},
+ null,
+ new Object[]{1.1d, 2.2d, 3.3d},
+ new Object[]{}
+ },
+ new Object[]{
+ 1672617600000L,
+ null,
+ Arrays.asList("a", "b"),
+ null,
+ new Object[]{2L, 3L},
+ null,
+ new Object[]{null, 1.1d}
+ },
+ new Object[]{
+ 1672617600000L,
+ Arrays.asList("a", "b"),
+ null,
+ null,
+ new Object[]{null, 2L, 9L},
+ null,
+ new Object[]{999.0d, 5.5d, null}
+ },
+ new Object[]{
+ 1672617600000L,
+ Arrays.asList("a", "b"),
+ Collections.emptyList(),
+ new Object[]{1L, 2L, 3L},
+ new Object[]{1L, null, 3L},
+ new Object[]{1.1d, 2.2d, 3.3d},
+ new Object[]{1.1d, 2.2d, null}
+ },
+ new Object[]{
+ 1672617600000L,
+ Arrays.asList("a", "b", "c"),
+ Arrays.asList(null, "b"),
+ new Object[]{2L, 3L},
+ null,
+ new Object[]{3.3d, 4.4d, 5.5d},
+ new Object[]{999.0d, null, 5.5d}
+ },
+ new Object[]{
+ 1672617600000L,
+ Arrays.asList("b", "c"),
+ Arrays.asList("d", null, "b"),
+ new Object[]{1L, 2L, 3L, 4L},
+ new Object[]{1L, 2L, 3L},
+ new Object[]{1.1d, 3.3d},
+ new Object[]{null, 2.2d, null}
+ },
+ new Object[]{
+ 1672617600000L,
+ Arrays.asList("d", "e"),
+ Arrays.asList("b", "b"),
+ new Object[]{1L, 4L},
+ new Object[]{null},
+ new Object[]{2.2d, 3.3d, 4.0},
+ null
+ }
+ );
+
+ RowSignature rowSignatureWithoutTimeAndStringColumns =
+ RowSignature.builder()
+ .add("arrayLong", ColumnType.LONG_ARRAY)
+ .add("arrayLongNulls", ColumnType.LONG_ARRAY)
+ .add("arrayDouble", ColumnType.DOUBLE_ARRAY)
+ .add("arrayDoubleNulls", ColumnType.DOUBLE_ARRAY)
+ .build();
+
+
+ RowSignature fileSignature = RowSignature.builder()
+ .add("timestamp", ColumnType.STRING)
+ .add("arrayString", ColumnType.STRING_ARRAY)
+ .add("arrayStringNulls", ColumnType.STRING_ARRAY)
+ .addAll(rowSignatureWithoutTimeAndStringColumns)
+ .build();
+
+ // MSQ writes strings instead of string arrays
+ RowSignature rowSignature = RowSignature.builder()
+ .add("__time", ColumnType.LONG)
+ .add("arrayString", ColumnType.STRING)
+ .add("arrayStringNulls", ColumnType.STRING)
+ .addAll(rowSignatureWithoutTimeAndStringColumns)
+ .build();
+
+ final Map<String, Object> adjustedContext = new HashMap<>(context);
+ final File tmpFile = temporaryFolder.newFile();
+ final InputStream resourceStream = NestedDataTestUtils.class.getClassLoader().getResourceAsStream(NestedDataTestUtils.ARRAY_TYPES_DATA_FILE);
Review Comment:
Since this method will run multiple times due to parametrized tests, we can consider writing this file only once. Maybe in a static method but that can be done in a follow up PR.
Thank you for adding this test though.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1317853814
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldReader.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.segment.column.ValueTypes;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+public abstract class NumericFieldReader<T extends Number> implements FieldReader
+{
+
+ private final byte nullIndicatorByte;
+
+ public NumericFieldReader(boolean forArray)
+ {
+ if (!forArray) {
+ this.nullIndicatorByte = NumericFieldWriter.NULL_BYTE;
+ } else {
+ this.nullIndicatorByte = NumericFieldWriter.ARRAY_NULL_BYTE;
+ }
+ }
+
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(Memory memory, ReadableFieldPointer fieldPointer)
+ {
+ return new Selector(memory, fieldPointer);
+ }
+
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ return ValueTypes.makeNumericWrappingDimensionSelector(
+ getValueType(),
+ makeColumnValueSelector(memory, fieldPointer),
+ extractionFn
+ );
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ return memory.getByte(position) == nullIndicatorByte;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract ValueType getValueType();
+
+ public abstract Class<? extends T> getClassOfObject();
+
+ public abstract T getValueFromMemory(Memory memory, long position);
Review Comment:
Great catch! Denormalized the readers a bit further to remove this additional allocation cost.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344823632
##########
processing/src/main/java/org/apache/druid/frame/field/DoubleArrayFieldReader.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reader for fields written by {@link NumericArrayFieldWriter#getDoubleArrayFieldWriter}
+ */
+public class DoubleArrayFieldReader extends NumericArrayFieldReader
+{
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer
+ )
+ {
+ return new Selector<Double>(memory, fieldPointer)
+ {
+
+ @Nullable
+ @Override
+ public Double getIndividualValueAtMemory(Memory memory, long position)
+ {
+ ColumnValueSelector<?> columnValueSelector =
+ DoubleFieldReader.forArray()
+ .makeColumnValueSelector(memory, new ConstantFieldPointer(position));
Review Comment:
Refactored, now we are just changing the position, and creating each object exactly once per selector
(which should probably be once per Frame i think, or slightly higher, if the frame was comprised of multiple memory locations)
--
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] Field writers for numerical arrays (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1343704788
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
Review Comment:
I did not understand the sentence
`though that would have been more convenient to keep the written array value byte comparable`
Even if we had lengths we could not make the bytes comparable since we sort arrays like
`[a,a,a] < [b]`
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldReader.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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.segment.column.ValueTypes;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reads the fields created by the {@link NumericFieldWriter}. See the Javadoc for the writer for format details
+ */
+public abstract class NumericFieldReader implements FieldReader
+{
+
+ /**
+ * The indicator byte which denotes that the following value is null.
+ */
+ private final byte nullIndicatorByte;
+
+ public NumericFieldReader(boolean forArray)
+ {
+ if (!forArray) {
+ this.nullIndicatorByte = NumericFieldWriter.NULL_BYTE;
+ } else {
+ this.nullIndicatorByte = NumericFieldWriter.ARRAY_ELEMENT_NULL_BYTE;
+ }
+ }
+
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(Memory memory, ReadableFieldPointer fieldPointer)
+ {
+ return getColumnValueSelector(memory, fieldPointer, nullIndicatorByte);
+ }
+
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ return ValueTypes.makeNumericWrappingDimensionSelector(
+ getValueType(),
+ makeColumnValueSelector(memory, fieldPointer),
+ extractionFn
+ );
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ return memory.getByte(position) == nullIndicatorByte;
+ }
+
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract ColumnValueSelector<?> getColumnValueSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ byte nullIndicatorByte
+ );
+
+ public abstract ValueType getValueType();
+
+ public abstract static class Selector
Review Comment:
Is there a reason why this is not a class of its own?
Where is this selector being used in the abstract class `NumericFieldReader` ?
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
+ * <p>
+ * Examples:
+ * 1. null
+ * | Bytes | Value | Interpretation |
+ * |--------|-------|-----------------------------|
+ * | 1 | 0x00 | Denotes that the array null |
+ * <p>
+ * 2. [] (empty array)
+ * | Bytes | Value | Interpretation |
+ * |--------|----- -|------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x00 | End of the array |
+ * <p>
+ * 3. [5L, null, 6L]
+ * | Bytes | Value | Interpretation |
+ * |---------|--------------|-----------------------------------------------------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x02 | Denotes that the next element is not null |
+ * | 3-10 | transform(5) | Representation of 5 |
+ * | 11 | 0x01 | Denotes that the next element is null |
+ * | 12-19 | transform(0) | Representation of 0 (default value, the reader will ignore it if SqlCompatible mode is on |
+ * | 20 | 0x02 | Denotes that the next element is not null |
+ * | 21-28 | transform(6) | Representation of 6 |
+ * | 29 | 0x00 | End of array |
+ */
+public class NumericArrayFieldWriter implements FieldWriter
+{
+
+ /**
+ * Denotes that the array itself is null
+ */
+ public static final byte NULL_ROW = 0x00;
+
+ /**
+ * Denotes that the array is non null
+ */
+ public static final byte NON_NULL_ROW = 0x01;
+
+ /**
+ * Marks the end of the array. Since {@link #NULL_ROW} and {@link #ARRAY_TERMINATOR} will only occur at different
+ * locations, therefore there is no clash in keeping both's values at 0x00
+ */
+ public static final byte ARRAY_TERMINATOR = 0x00;
+
+ private final ColumnValueSelector selector;
+ private final NumericFieldWriterFactory writerFactory;
+
+ /**
+ * Returns the writer for ARRAY<LONG>
+ */
+ public static NumericArrayFieldWriter getLongArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, LongFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<FLOAT>
+ */
+ public static NumericArrayFieldWriter getFloatArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, FloatFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<DOUBLE>
+ */
+ public static NumericArrayFieldWriter getDoubleArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, DoubleFieldWriter::forArray);
+ }
+
+ public NumericArrayFieldWriter(final ColumnValueSelector selector, NumericFieldWriterFactory writerFactory)
+ {
+ this.selector = selector;
+ this.writerFactory = writerFactory;
+ }
+
+ @Override
+ public long writeTo(WritableMemory memory, long position, long maxSize)
+ {
+ Object row = selector.getObject();
+ if (row == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ } else {
+
+ List<? extends Number> list = FrameWriterUtils.getNumericArrayFromNumericArray(row);
+
+ if (list == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ }
+
+ // Create a columnValueSelector to write the individual elements re-using the NumericFieldWriter
+ AtomicInteger index = new AtomicInteger(0);
+ ColumnValueSelector<Number> columnValueSelector = new ColumnValueSelector<Number>()
Review Comment:
Can we push column value selector to another class or a subclass. That would make the code more readable.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
+ * <p>
+ * Examples:
+ * 1. null
+ * | Bytes | Value | Interpretation |
+ * |--------|-------|-----------------------------|
+ * | 1 | 0x00 | Denotes that the array null |
+ * <p>
+ * 2. [] (empty array)
+ * | Bytes | Value | Interpretation |
+ * |--------|----- -|------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x00 | End of the array |
+ * <p>
+ * 3. [5L, null, 6L]
+ * | Bytes | Value | Interpretation |
+ * |---------|--------------|-----------------------------------------------------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x02 | Denotes that the next element is not null |
+ * | 3-10 | transform(5) | Representation of 5 |
+ * | 11 | 0x01 | Denotes that the next element is null |
+ * | 12-19 | transform(0) | Representation of 0 (default value, the reader will ignore it if SqlCompatible mode is on |
+ * | 20 | 0x02 | Denotes that the next element is not null |
+ * | 21-28 | transform(6) | Representation of 6 |
+ * | 29 | 0x00 | End of array |
+ */
+public class NumericArrayFieldWriter implements FieldWriter
+{
+
+ /**
+ * Denotes that the array itself is null
+ */
+ public static final byte NULL_ROW = 0x00;
+
+ /**
+ * Denotes that the array is non null
+ */
+ public static final byte NON_NULL_ROW = 0x01;
+
+ /**
+ * Marks the end of the array. Since {@link #NULL_ROW} and {@link #ARRAY_TERMINATOR} will only occur at different
+ * locations, therefore there is no clash in keeping both's values at 0x00
+ */
+ public static final byte ARRAY_TERMINATOR = 0x00;
+
+ private final ColumnValueSelector selector;
+ private final NumericFieldWriterFactory writerFactory;
+
+ /**
+ * Returns the writer for ARRAY<LONG>
+ */
+ public static NumericArrayFieldWriter getLongArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, LongFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<FLOAT>
+ */
+ public static NumericArrayFieldWriter getFloatArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, FloatFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<DOUBLE>
+ */
+ public static NumericArrayFieldWriter getDoubleArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, DoubleFieldWriter::forArray);
+ }
+
+ public NumericArrayFieldWriter(final ColumnValueSelector selector, NumericFieldWriterFactory writerFactory)
+ {
+ this.selector = selector;
+ this.writerFactory = writerFactory;
+ }
+
+ @Override
+ public long writeTo(WritableMemory memory, long position, long maxSize)
+ {
+ Object row = selector.getObject();
+ if (row == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ } else {
+
+ List<? extends Number> list = FrameWriterUtils.getNumericArrayFromNumericArray(row);
+
+ if (list == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ }
+
+ // Create a columnValueSelector to write the individual elements re-using the NumericFieldWriter
+ AtomicInteger index = new AtomicInteger(0);
+ ColumnValueSelector<Number> columnValueSelector = new ColumnValueSelector<Number>()
+ {
+ @Override
+ public double getDouble()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.doubleValue() : 0d;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.floatValue() : 0f;
+ }
+
+ @Override
+ public long getLong()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.longValue() : 0L;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ // Arrays preserve the individual element's nullity when they are written and read.
+ // Therefore, when working with SQL incompatible mode, [7, null] won't change to [7, 0] when written to and
+ // read from the underlying serialization (as compared with the primitives). Therefore,
+ // even when NullHandling.replaceWithDefault() is true we need to write null as is, and not convert it to their
+ // default value when writing the array. Therefore, the check is `getObject() == null` ignoring the value of
+ // `NullHandling.replaceWithDefaul()`.
+ return getObject() == null;
Review Comment:
@clintropolis Could you please review this.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldWriter.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+/**
+ * FieldWriter for numeric datatypes. The parent class does the null handling for the underlying data, while
+ * the individual subclasses write the individual element (long, float or double type). This also allows for a clean
+ * reuse of the readers and writers between the numeric types and also allowing the array writers ({@link NumericArrayFieldWriter})
+ * to use these methods directly without duplication
+ *
+ * Format:
+ * - 1 byte: Whether the following value is null or not. Take a look at the note on the indicator bytes.
+ * - X bytes: Encoded value of the selector, or the default value if it is null. X denotes the size of the numeric value
+ *
+ * Indicator bytes for denoting whether the element is null or not null changes depending on whether the writer is used
+ * to write the data for individual value (like LONG) or for an element of an array (like ARRAY<LONG>). This is because
+ * array support for the numeric types was added later and by then the field writers for individual fields were using
+ * 0x00 to denote the null byte, which is reserved for denoting the array end when we are writing the elements as part
+ * of the array instead. (0x00 is used for array end because it helps in preserving the byte comparison property of the
+ * numeric array field writers).
+ *
+ * Therefore, to preserve backward and forward compatibility, the individual element's writers were left unchanged,
Review Comment:
Please provide and example of the above. This will help user reason about the choice of bytes.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldReader.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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.segment.column.ValueTypes;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reads the fields created by the {@link NumericFieldWriter}. See the Javadoc for the writer for format details
+ */
+public abstract class NumericFieldReader implements FieldReader
+{
+
+ /**
+ * The indicator byte which denotes that the following value is null.
+ */
+ private final byte nullIndicatorByte;
+
+ public NumericFieldReader(boolean forArray)
+ {
+ if (!forArray) {
+ this.nullIndicatorByte = NumericFieldWriter.NULL_BYTE;
+ } else {
+ this.nullIndicatorByte = NumericFieldWriter.ARRAY_ELEMENT_NULL_BYTE;
+ }
+ }
+
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(Memory memory, ReadableFieldPointer fieldPointer)
+ {
+ return getColumnValueSelector(memory, fieldPointer, nullIndicatorByte);
+ }
+
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ return ValueTypes.makeNumericWrappingDimensionSelector(
+ getValueType(),
+ makeColumnValueSelector(memory, fieldPointer),
+ extractionFn
+ );
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ return memory.getByte(position) == nullIndicatorByte;
+ }
+
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract ColumnValueSelector<?> getColumnValueSelector(
Review Comment:
Could you please java doc the abstract class methods.
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReader.java:
##########
@@ -57,15 +61,28 @@ public class FrameReader
// Field readers, for row-based frames.
private final List<FieldReader> fieldReaders;
+ /**
+ * Currently, only ROW_BASED frames support numerical array columns, while the COLUMNAR frames donot. While creating
+ * a FrameReader, for types unsupported by COLUMNAR frames, we populate this field to denote that the FrameReader is
+ * "incomplete" and can't be used to read the columnar frame. However, the FrameReader performs as expected for the
+ * row-based frames.
+ * In short, this is a temporary measure till columnar frames support the numerical array types to punt the unsupported
+ * type check for the numerical arrays (for COLUMNAR frames only) at the usage time, rather than the creation time
+ */
+ private final Optional<Pair<String, ColumnType>> unsupportedColumnAndType;
Review Comment:
+1 to @gianm comment. I think frameReader should not track the incompatible column types. That logic should be pushed to the `fieldReaders`and they should just throw impl not found.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
+ * <p>
+ * Examples:
+ * 1. null
+ * | Bytes | Value | Interpretation |
+ * |--------|-------|-----------------------------|
+ * | 1 | 0x00 | Denotes that the array null |
+ * <p>
+ * 2. [] (empty array)
+ * | Bytes | Value | Interpretation |
+ * |--------|----- -|------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x00 | End of the array |
+ * <p>
+ * 3. [5L, null, 6L]
+ * | Bytes | Value | Interpretation |
+ * |---------|--------------|-----------------------------------------------------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x02 | Denotes that the next element is not null |
+ * | 3-10 | transform(5) | Representation of 5 |
+ * | 11 | 0x01 | Denotes that the next element is null |
+ * | 12-19 | transform(0) | Representation of 0 (default value, the reader will ignore it if SqlCompatible mode is on |
+ * | 20 | 0x02 | Denotes that the next element is not null |
+ * | 21-28 | transform(6) | Representation of 6 |
+ * | 29 | 0x00 | End of array |
+ */
+public class NumericArrayFieldWriter implements FieldWriter
+{
+
+ /**
+ * Denotes that the array itself is null
+ */
+ public static final byte NULL_ROW = 0x00;
+
+ /**
+ * Denotes that the array is non null
+ */
+ public static final byte NON_NULL_ROW = 0x01;
+
+ /**
+ * Marks the end of the array. Since {@link #NULL_ROW} and {@link #ARRAY_TERMINATOR} will only occur at different
+ * locations, therefore there is no clash in keeping both's values at 0x00
+ */
+ public static final byte ARRAY_TERMINATOR = 0x00;
+
+ private final ColumnValueSelector selector;
+ private final NumericFieldWriterFactory writerFactory;
+
+ /**
+ * Returns the writer for ARRAY<LONG>
+ */
+ public static NumericArrayFieldWriter getLongArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, LongFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<FLOAT>
+ */
+ public static NumericArrayFieldWriter getFloatArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, FloatFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<DOUBLE>
+ */
+ public static NumericArrayFieldWriter getDoubleArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, DoubleFieldWriter::forArray);
+ }
+
+ public NumericArrayFieldWriter(final ColumnValueSelector selector, NumericFieldWriterFactory writerFactory)
+ {
+ this.selector = selector;
+ this.writerFactory = writerFactory;
+ }
+
+ @Override
+ public long writeTo(WritableMemory memory, long position, long maxSize)
+ {
+ Object row = selector.getObject();
+ if (row == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ } else {
+
+ List<? extends Number> list = FrameWriterUtils.getNumericArrayFromNumericArray(row);
+
+ if (list == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ }
+
+ // Create a columnValueSelector to write the individual elements re-using the NumericFieldWriter
+ AtomicInteger index = new AtomicInteger(0);
+ ColumnValueSelector<Number> columnValueSelector = new ColumnValueSelector<Number>()
+ {
+ @Override
+ public double getDouble()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.doubleValue() : 0d;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.floatValue() : 0f;
+ }
+
+ @Override
+ public long getLong()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.longValue() : 0L;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ // Arrays preserve the individual element's nullity when they are written and read.
+ // Therefore, when working with SQL incompatible mode, [7, null] won't change to [7, 0] when written to and
+ // read from the underlying serialization (as compared with the primitives). Therefore,
+ // even when NullHandling.replaceWithDefault() is true we need to write null as is, and not convert it to their
+ // default value when writing the array. Therefore, the check is `getObject() == null` ignoring the value of
+ // `NullHandling.replaceWithDefaul()`.
+ return getObject() == null;
+ }
+
+ @Nullable
+ @Override
+ public Number getObject()
+ {
+ return list.get(index.get());
+ }
+
+ @Override
+ public Class<? extends Number> classOfObject()
+ {
+ return Number.class;
+ }
+ };
+
+ NumericFieldWriter writer = writerFactory.get(columnValueSelector);
+
+ int requiredSize = Byte.BYTES + (writer.getNumericSizeBytes() + Byte.BYTES) * list.size() + Byte.BYTES;
Review Comment:
Could you please a java comment here.
I think its [startMarker, fields ..... end Marker]
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
+ * <p>
+ * Examples:
+ * 1. null
+ * | Bytes | Value | Interpretation |
+ * |--------|-------|-----------------------------|
+ * | 1 | 0x00 | Denotes that the array null |
+ * <p>
+ * 2. [] (empty array)
+ * | Bytes | Value | Interpretation |
+ * |--------|----- -|------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x00 | End of the array |
+ * <p>
+ * 3. [5L, null, 6L]
+ * | Bytes | Value | Interpretation |
+ * |---------|--------------|-----------------------------------------------------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x02 | Denotes that the next element is not null |
+ * | 3-10 | transform(5) | Representation of 5 |
+ * | 11 | 0x01 | Denotes that the next element is null |
+ * | 12-19 | transform(0) | Representation of 0 (default value, the reader will ignore it if SqlCompatible mode is on |
+ * | 20 | 0x02 | Denotes that the next element is not null |
+ * | 21-28 | transform(6) | Representation of 6 |
+ * | 29 | 0x00 | End of array |
+ */
+public class NumericArrayFieldWriter implements FieldWriter
+{
+
+ /**
+ * Denotes that the array itself is null
+ */
+ public static final byte NULL_ROW = 0x00;
+
+ /**
+ * Denotes that the array is non null
+ */
+ public static final byte NON_NULL_ROW = 0x01;
+
+ /**
+ * Marks the end of the array. Since {@link #NULL_ROW} and {@link #ARRAY_TERMINATOR} will only occur at different
+ * locations, therefore there is no clash in keeping both's values at 0x00
+ */
+ public static final byte ARRAY_TERMINATOR = 0x00;
+
+ private final ColumnValueSelector selector;
+ private final NumericFieldWriterFactory writerFactory;
+
+ /**
+ * Returns the writer for ARRAY<LONG>
+ */
+ public static NumericArrayFieldWriter getLongArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, LongFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<FLOAT>
+ */
+ public static NumericArrayFieldWriter getFloatArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, FloatFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<DOUBLE>
+ */
+ public static NumericArrayFieldWriter getDoubleArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, DoubleFieldWriter::forArray);
+ }
+
+ public NumericArrayFieldWriter(final ColumnValueSelector selector, NumericFieldWriterFactory writerFactory)
+ {
+ this.selector = selector;
+ this.writerFactory = writerFactory;
+ }
+
+ @Override
+ public long writeTo(WritableMemory memory, long position, long maxSize)
+ {
+ Object row = selector.getObject();
+ if (row == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ } else {
+
+ List<? extends Number> list = FrameWriterUtils.getNumericArrayFromNumericArray(row);
+
+ if (list == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ }
+
+ // Create a columnValueSelector to write the individual elements re-using the NumericFieldWriter
+ AtomicInteger index = new AtomicInteger(0);
+ ColumnValueSelector<Number> columnValueSelector = new ColumnValueSelector<Number>()
+ {
+ @Override
+ public double getDouble()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.doubleValue() : 0d;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.floatValue() : 0f;
+ }
+
+ @Override
+ public long getLong()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.longValue() : 0L;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ // Arrays preserve the individual element's nullity when they are written and read.
+ // Therefore, when working with SQL incompatible mode, [7, null] won't change to [7, 0] when written to and
+ // read from the underlying serialization (as compared with the primitives). Therefore,
+ // even when NullHandling.replaceWithDefault() is true we need to write null as is, and not convert it to their
+ // default value when writing the array. Therefore, the check is `getObject() == null` ignoring the value of
+ // `NullHandling.replaceWithDefaul()`.
+ return getObject() == null;
+ }
+
+ @Nullable
+ @Override
+ public Number getObject()
+ {
+ return list.get(index.get());
+ }
+
+ @Override
+ public Class<? extends Number> classOfObject()
+ {
+ return Number.class;
+ }
+ };
+
+ NumericFieldWriter writer = writerFactory.get(columnValueSelector);
+
+ int requiredSize = Byte.BYTES + (writer.getNumericSizeBytes() + Byte.BYTES) * list.size() + Byte.BYTES;
+
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+
+ long offset = 0;
+ memory.putByte(position + offset, NON_NULL_ROW);
+ offset += Byte.BYTES;
+
+ for (; index.get() < list.size(); index.incrementAndGet()) {
Review Comment:
Can we iterate the list 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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344123026
##########
processing/src/test/java/org/apache/druid/frame/field/DoubleArrayFieldReaderTest.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.frame.field;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.testing.InitializedNullHandlingTest;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class DoubleArrayFieldReaderTest extends InitializedNullHandlingTest
Review Comment:
The test case should work in both compatible and incompatible mode, since arrays don't coerce elements to null. https://github.com/apache/druid/pull/14900#discussion_r1343948446
--
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] Field writers for numerical arrays (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1307884483
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ // TODO: Should I throw an exception here
Review Comment:
You should do the same thing that numeric array columns do in regular segments. (I don't know what that is, but you should find out and then do the same thing.)
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldReader.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.segment.column.ValueTypes;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+public abstract class NumericFieldReader<T extends Number> implements FieldReader
+{
+
+ private final byte nullIndicatorByte;
+
+ public NumericFieldReader(boolean forArray)
+ {
+ if (!forArray) {
+ this.nullIndicatorByte = NumericFieldWriter.NULL_BYTE;
+ } else {
+ this.nullIndicatorByte = NumericFieldWriter.ARRAY_NULL_BYTE;
+ }
+ }
+
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(Memory memory, ReadableFieldPointer fieldPointer)
+ {
+ return new Selector(memory, fieldPointer);
+ }
+
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ return ValueTypes.makeNumericWrappingDimensionSelector(
+ getValueType(),
+ makeColumnValueSelector(memory, fieldPointer),
+ extractionFn
+ );
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ return memory.getByte(position) == nullIndicatorByte;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract ValueType getValueType();
+
+ public abstract Class<? extends T> getClassOfObject();
+
+ public abstract T getValueFromMemory(Memory memory, long position);
Review Comment:
Unfortunately this design requires boxing, whereas the previous design didn't. Is it possible to update this design to avoid boxing and therefore avoid the cost of the additional allocations?
--
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] Field writers for numerical arrays (druid)
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1318313895
##########
processing/src/main/java/org/apache/druid/frame/field/FieldReaders.java:
##########
@@ -61,8 +60,19 @@
return ComplexFieldReader.createFromType(columnType);
case ARRAY:
- if (columnType.getElementType().getType() == ValueType.STRING) {
- return new StringFieldReader(true);
+ switch (Preconditions.checkNotNull(columnType.getElementType().getType(), "array elementType")) {
Review Comment:
## Missing enum case in switch
Switch statement does not have a case for [ARRAY](1).
Switch statement does not have a case for [COMPLEX](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/5769)
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1345497227
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldSelector.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Base implementation of the column value selector that the concrete numeric field reader implementations inherit from.
+ * The selector contains the logic to construct an array written by {@link NumericArrayFieldWriter}, and present it as
+ * a column value selector.
+ *
+ * The inheritors of this class are expected to implement
+ * 1. {@link #getIndividualValueAtMemory} Which extracts the element from the field where it was written to. Returns
+ * null if the value at that location represents a null element
+ * 2. {@link #getIndividualFieldSize} Which informs the method about the field size corresponding to each element in
+ * the numeric array's serialized representation
+ *
+ * @param <ElementType> Type of the individual array elements
+ */
+public abstract class NumericArrayFieldSelector<ElementType extends Number> implements ColumnValueSelector
+{
+ /**
+ * Memory containing the serialized values of the array
+ */
+ protected final Memory memory;
+
+ /**
+ * Pointer to location in the memory. The callers are expected to update the pointer's position to the start of the
+ * array that they wish to get prior to {@link #getObject()} call.
+ *
+ * Frames read and written using {@link org.apache.druid.frame.write.FrameWriter} and
+ * {@link org.apache.druid.frame.read.FrameReader} shouldn't worry about this detail, since they automatically update
+ * and handle the start location
+ */
+ private final ReadableFieldPointer fieldPointer;
+
+ /**
+ * Position last read, for caching the last fetched result
+ */
+ private long currentFieldPosition = -1;
+
+ /**
+ * Value of the row at the location beginning at {@link #currentFieldPosition}
+ */
+ private final List<ElementType> currentRow = new ArrayList<>();
+
+ /**
+ * Nullity of the row at the location beginning at {@link #currentFieldPosition}
+ */
+ private boolean currentRowIsNull;
+
+ public NumericArrayFieldSelector(final Memory memory, final ReadableFieldPointer fieldPointer)
+ {
+ this.memory = memory;
+ this.fieldPointer = fieldPointer;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+ // Do nothing
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ final List<ElementType> currentArray = computeCurrentArray();
Review Comment:
> the array length after the null byte and wanting to spit out Object[].
So you are saying the format should be something like:
Begin array indicator -> Array Elements -> Array terminator -> Size of array
The size of the array will be helpful in preventing extra converts of `List <-> Object` while reading since we can directly allocate that much memory upfront. I never considered it, and it also doesn't mess with the comparison aspect of the fields, since we are going to end the comparison before that itself.
However, since the array end is not known, we'd need to have a two-pass, where we find the rowTerminator first, and then figure out the size, allocate an array, and then re-read the elements from the bytes.
So it's b/w - Two passes through the byte array, however without any List <-> Object[] conversion
Or Single pass, requiring List<-> Object[] conversion
--
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] Field writers for numerical arrays (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1345480513
##########
sql/src/test/java/org/apache/druid/sql/calcite/QueryTestRunner.java:
##########
@@ -667,7 +674,9 @@ public QueryTestRunner(QueryTestBuilder builder)
verifySteps.add(new VerifyNativeQueries(finalExecStep));
}
if (builder.expectedResultsVerifier != null) {
- verifySteps.add(new VerifyResults(finalExecStep));
+ // Don't verify the row signature when MSQ is running, since the broker receives the task id, and the signature
+ // would be {TASK:STRING} instead of the expected results signature
+ verifySteps.add(new VerifyResults(finalExecStep, !config.isRunningMSQ()));
Review Comment:
it might be nice to have something that functions equivalently for msq based tests (if there isn't already) but this doesn't seem like a blocker.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldSelector.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Base implementation of the column value selector that the concrete numeric field reader implementations inherit from.
+ * The selector contains the logic to construct an array written by {@link NumericArrayFieldWriter}, and present it as
+ * a column value selector.
+ *
+ * The inheritors of this class are expected to implement
+ * 1. {@link #getIndividualValueAtMemory} Which extracts the element from the field where it was written to. Returns
+ * null if the value at that location represents a null element
+ * 2. {@link #getIndividualFieldSize} Which informs the method about the field size corresponding to each element in
+ * the numeric array's serialized representation
+ *
+ * @param <ElementType> Type of the individual array elements
+ */
+public abstract class NumericArrayFieldSelector<ElementType extends Number> implements ColumnValueSelector
+{
+ /**
+ * Memory containing the serialized values of the array
+ */
+ protected final Memory memory;
+
+ /**
+ * Pointer to location in the memory. The callers are expected to update the pointer's position to the start of the
+ * array that they wish to get prior to {@link #getObject()} call.
+ *
+ * Frames read and written using {@link org.apache.druid.frame.write.FrameWriter} and
+ * {@link org.apache.druid.frame.read.FrameReader} shouldn't worry about this detail, since they automatically update
+ * and handle the start location
+ */
+ private final ReadableFieldPointer fieldPointer;
+
+ /**
+ * Position last read, for caching the last fetched result
+ */
+ private long currentFieldPosition = -1;
+
+ /**
+ * Value of the row at the location beginning at {@link #currentFieldPosition}
+ */
+ private final List<ElementType> currentRow = new ArrayList<>();
+
+ /**
+ * Nullity of the row at the location beginning at {@link #currentFieldPosition}
+ */
+ private boolean currentRowIsNull;
+
+ public NumericArrayFieldSelector(final Memory memory, final ReadableFieldPointer fieldPointer)
+ {
+ this.memory = memory;
+ this.fieldPointer = fieldPointer;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+ // Do nothing
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ final List<ElementType> currentArray = computeCurrentArray();
Review Comment:
i suppose this is the only real disadvantage of not storing the array length after the null byte and wanting to spit out `Object[]`. Basically that since we don't know the length up front we have to make a list and then convert it to `Object[]` after (though realistically right now it seems like almost everything that handles arrays handles Object[], List, and some group by stuff, so its not strictly required, but is better to be consistent).
I'm not sure that the extra bytes per value (could use `VByte.writeInt`/`VByte.readInt`, well add methods that work on `Memory` instead of `ByteBuffer` to fit in a little as a byte) are worth the savings in reads, might be worth measuring someday, though this doesn't really seem like a blocker to me
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344087198
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Reader class for the fields written by {@link NumericArrayFieldWriter}. See the Javadoc for the writer for more
+ * information on the format
+ *
+ * The numeric array fields are byte comparable
+ */
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ throw DruidException.defensive("Cannot call makeDimensionSelector on field of type ARRAY");
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract static class Selector<T extends Number> implements ColumnValueSelector
+ {
+ private final Memory memory;
+ private final ReadableFieldPointer fieldPointer;
+
+ private long currentFieldPosition = -1;
+
+ private final List<T> currentRow = new ArrayList<>();
+ private boolean currentRowIsNull;
+
+ public Selector(final Memory memory, final ReadableFieldPointer fieldPointer)
+ {
+ this.memory = memory;
+ this.fieldPointer = fieldPointer;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ final List<T> currentArray = computeCurrentArray();
+
+ if (currentArray == null) {
+ return null;
+ }
+
+ return currentArray.toArray();
+ }
+
+ @Override
+ public Class classOfObject()
+ {
+ return Object.class;
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return 0;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return 0;
+ }
+
+ @Override
+ public long getLong()
+ {
+ return 0;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ long position = fieldPointer.position();
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
Review Comment:
Yep, need a check for nullity while fetching the element.
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Reader class for the fields written by {@link NumericArrayFieldWriter}. See the Javadoc for the writer for more
+ * information on the format
+ *
+ * The numeric array fields are byte comparable
+ */
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ throw DruidException.defensive("Cannot call makeDimensionSelector on field of type ARRAY");
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract static class Selector<T extends Number> implements ColumnValueSelector
+ {
+ private final Memory memory;
+ private final ReadableFieldPointer fieldPointer;
+
+ private long currentFieldPosition = -1;
+
+ private final List<T> currentRow = new ArrayList<>();
+ private boolean currentRowIsNull;
+
+ public Selector(final Memory memory, final ReadableFieldPointer fieldPointer)
+ {
+ this.memory = memory;
+ this.fieldPointer = fieldPointer;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ final List<T> currentArray = computeCurrentArray();
+
+ if (currentArray == null) {
+ return null;
+ }
+
+ return currentArray.toArray();
+ }
+
+ @Override
+ public Class classOfObject()
+ {
+ return Object.class;
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return 0;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return 0;
+ }
+
+ @Override
+ public long getLong()
+ {
+ return 0;
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ long position = fieldPointer.position();
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
Review Comment:
Yep, need a check for nullity while fetching the object.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344805703
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldReader.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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.segment.column.ValueTypes;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reads the fields created by the {@link NumericFieldWriter}. See the Javadoc for the writer for format details
+ */
+public abstract class NumericFieldReader implements FieldReader
+{
+
+ /**
+ * The indicator byte which denotes that the following value is null.
+ */
+ private final byte nullIndicatorByte;
+
+ public NumericFieldReader(boolean forArray)
+ {
+ if (!forArray) {
+ this.nullIndicatorByte = NumericFieldWriter.NULL_BYTE;
+ } else {
+ this.nullIndicatorByte = NumericFieldWriter.ARRAY_ELEMENT_NULL_BYTE;
+ }
+ }
+
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(Memory memory, ReadableFieldPointer fieldPointer)
+ {
+ return getColumnValueSelector(memory, fieldPointer, nullIndicatorByte);
+ }
+
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ return ValueTypes.makeNumericWrappingDimensionSelector(
+ getValueType(),
+ makeColumnValueSelector(memory, fieldPointer),
+ extractionFn
+ );
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ return memory.getByte(position) == nullIndicatorByte;
+ }
+
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract ColumnValueSelector<?> getColumnValueSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ byte nullIndicatorByte
+ );
+
+ public abstract ValueType getValueType();
+
+ public abstract static class Selector
Review Comment:
Keeping it as is, since this is used for nullity check only. I'll add the relevant comments though.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #14900:
URL: https://github.com/apache/druid/pull/14900#issuecomment-1747368154
```
Unit & Integration tests CI / revised-its / it (8, HighAvailability, middleManager) / HighAvailability integration test (Compile=jdk8, Run=jdk8, Indexer=middleManager) (pull_request) Failing after 65m
```
HA Integration tests are failing, which this PR doesn't touch upon and seems unrelated. Also, it passed in the previous run.
Going ahead with the merge.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1343497853
##########
processing/src/main/java/org/apache/druid/frame/write/FrameWriterUtils.java:
##########
@@ -172,6 +174,40 @@ public static List<ByteBuffer> getUtf8ByteBuffersFromStringArraySelector(
return retVal;
}
+ @Nullable
+ public static List<? extends Number> getNumericArrayFromNumericArray(Object row)
Review Comment:
I was absent-minded while writing the name. Added javadocs and fixed the method name
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344752145
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldReader.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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.segment.column.ValueTypes;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reads the fields created by the {@link NumericFieldWriter}. See the Javadoc for the writer for format details
+ */
+public abstract class NumericFieldReader implements FieldReader
+{
+
+ /**
+ * The indicator byte which denotes that the following value is null.
+ */
+ private final byte nullIndicatorByte;
+
+ public NumericFieldReader(boolean forArray)
+ {
+ if (!forArray) {
+ this.nullIndicatorByte = NumericFieldWriter.NULL_BYTE;
+ } else {
+ this.nullIndicatorByte = NumericFieldWriter.ARRAY_ELEMENT_NULL_BYTE;
+ }
+ }
+
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(Memory memory, ReadableFieldPointer fieldPointer)
+ {
+ return getColumnValueSelector(memory, fieldPointer, nullIndicatorByte);
+ }
+
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ return ValueTypes.makeNumericWrappingDimensionSelector(
+ getValueType(),
+ makeColumnValueSelector(memory, fieldPointer),
+ extractionFn
+ );
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ return memory.getByte(position) == nullIndicatorByte;
+ }
+
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract ColumnValueSelector<?> getColumnValueSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ byte nullIndicatorByte
+ );
+
+ public abstract ValueType getValueType();
+
+ public abstract static class Selector
Review Comment:
All of the previous implementations of the field readers kept the selector as a private class inside their code, therefore I kept the selectors inside the parent class. Since this one is getting inherited, I'll move this out.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344069760
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldWriter.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+/**
+ * FieldWriter for numeric datatypes. The parent class does the null handling for the underlying data, while
+ * the individual subclasses write the individual element (long, float or double type). This also allows for a clean
+ * reuse of the readers and writers between the numeric types and also allowing the array writers ({@link NumericArrayFieldWriter})
+ * to use these methods directly without duplication
+ *
+ * Format:
+ * - 1 byte: Whether the following value is null or not. Take a look at the note on the indicator bytes.
+ * - X bytes: Encoded value of the selector, or the default value if it is null. X denotes the size of the numeric value
+ *
+ * Indicator bytes for denoting whether the element is null or not null changes depending on whether the writer is used
+ * to write the data for individual value (like LONG) or for an element of an array (like ARRAY<LONG>). This is because
+ * array support for the numeric types was added later and by then the field writers for individual fields were using
+ * 0x00 to denote the null byte, which is reserved for denoting the array end when we are writing the elements as part
+ * of the array instead. (0x00 is used for array end because it helps in preserving the byte comparison property of the
+ * numeric array field writers).
+ *
+ * Therefore, to preserve backward and forward compatibility, the individual element's writers were left unchanged,
+ * while the array's element's writers used 0x01 and 0x02 to denote null and non-null byte respectively
+ *
+ * Values produced by the writer are sortable without decoding
+ */
+public abstract class NumericFieldWriter implements FieldWriter
+{
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is null. This is used in the primitive
+ * writers. NULL_BYTE < NOT_NULL_BYTE to preserve the ordering while doing byte comparison
+ */
+ public static final byte NULL_BYTE = 0x00;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is not null. This is used in the primitive
+ * writers
+ */
+ public static final byte NOT_NULL_BYTE = 0x01;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is null. This is used while writing the individual
+ * elements writers of an array. ARRAY_ELEMENT_NULL_BYTE < ARRAY_ELEMENT_NOT_NULL_BYTE to preserve the ordering
+ * while doing byte comparison
+ */
+ public static final byte ARRAY_ELEMENT_NULL_BYTE = 0x01;
+
+ /**
+ * Indicator byte denoting that the numeric value succeeding it is not null. This is used while writing the individual
+ * elements writers of an array
+ */
+ public static final byte ARRAY_ELEMENT_NOT_NULL_BYTE = 0x02;
Review Comment:
Byte-by-byte comparison won't work if we change the order.
Comparing 2 arrays [1] and [1, 2].
Semantically, [1] < [1, 2]
When we convert using the scheme you mentioned, we'd encode them as
```
[1]
0x01 (non-null array)
0x01 (non-null byte)
transform(1)
0x02 (array end) (*)
```
```
[1, 2]
0x01 (non-null array)
0x01 (non-null byte)
transform(1)
0x01 (non-null byte) (*)
transform(2)
0x02 (array end)
```
However, on comparing byte-by-byte, the second array < first array, and the first point of difference would be the (*) marked location.
Therefore the array terminator is the smallest marker of them all.
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344607747
##########
processing/src/main/java/org/apache/druid/frame/field/FieldReaders.java:
##########
@@ -61,8 +60,22 @@ public static FieldReader create(final String columnName, final ColumnType colum
return ComplexFieldReader.createFromType(columnType);
case ARRAY:
- if (columnType.getElementType().getType() == ValueType.STRING) {
- return new StringFieldReader(true);
+ switch (Preconditions.checkNotNull(columnType.getElementType().getType(), "array elementType")) {
+ case STRING:
+ return new StringFieldReader(true);
Review Comment:
I have added a new namesake reader for this.
So, to create new methods would be:
1. `new StringFieldReader()` -> for strings
2. `new StringArrayFieldReader()` -> for string arrays
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344810029
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldReader.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.extraction.ExtractionFn;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.DimensionSelector;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Reader class for the fields written by {@link NumericArrayFieldWriter}. See the Javadoc for the writer for more
+ * information on the format
+ *
+ * The numeric array fields are byte comparable
+ */
+public abstract class NumericArrayFieldReader implements FieldReader
+{
+ @Override
+ public DimensionSelector makeDimensionSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer,
+ @Nullable ExtractionFn extractionFn
+ )
+ {
+ throw DruidException.defensive("Cannot call makeDimensionSelector on field of type ARRAY");
+ }
+
+ @Override
+ public boolean isNull(Memory memory, long position)
+ {
+ final byte firstByte = memory.getByte(position);
+ return firstByte == NumericArrayFieldWriter.NULL_ROW;
+ }
+
+ @Override
+ public boolean isComparable()
+ {
+ return true;
+ }
+
+ public abstract static class Selector<T extends Number> implements ColumnValueSelector
Review Comment:
Updated comments and docs
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344823632
##########
processing/src/main/java/org/apache/druid/frame/field/DoubleArrayFieldReader.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.Memory;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+
+/**
+ * Reader for fields written by {@link NumericArrayFieldWriter#getDoubleArrayFieldWriter}
+ */
+public class DoubleArrayFieldReader extends NumericArrayFieldReader
+{
+ @Override
+ public ColumnValueSelector<?> makeColumnValueSelector(
+ Memory memory,
+ ReadableFieldPointer fieldPointer
+ )
+ {
+ return new Selector<Double>(memory, fieldPointer)
+ {
+
+ @Nullable
+ @Override
+ public Double getIndividualValueAtMemory(Memory memory, long position)
+ {
+ ColumnValueSelector<?> columnValueSelector =
+ DoubleFieldReader.forArray()
+ .makeColumnValueSelector(memory, new ConstantFieldPointer(position));
Review Comment:
Refactored, now we are just changing the position, and creating each object (reader, column value selector, field pointer) exactly once per selector.
(which should probably be once per Frame I think, or slightly higher, if the frame was comprised of multiple memory locations)
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344092351
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
+ * <p>
+ * Examples:
+ * 1. null
+ * | Bytes | Value | Interpretation |
+ * |--------|-------|-----------------------------|
+ * | 1 | 0x00 | Denotes that the array null |
+ * <p>
+ * 2. [] (empty array)
+ * | Bytes | Value | Interpretation |
+ * |--------|----- -|------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x00 | End of the array |
+ * <p>
+ * 3. [5L, null, 6L]
+ * | Bytes | Value | Interpretation |
+ * |---------|--------------|-----------------------------------------------------------------------------------|
+ * | 1 | 0x01 | Denotes that the array is not null |
+ * | 2 | 0x02 | Denotes that the next element is not null |
+ * | 3-10 | transform(5) | Representation of 5 |
+ * | 11 | 0x01 | Denotes that the next element is null |
+ * | 12-19 | transform(0) | Representation of 0 (default value, the reader will ignore it if SqlCompatible mode is on |
+ * | 20 | 0x02 | Denotes that the next element is not null |
+ * | 21-28 | transform(6) | Representation of 6 |
+ * | 29 | 0x00 | End of array |
+ */
+public class NumericArrayFieldWriter implements FieldWriter
+{
+
+ /**
+ * Denotes that the array itself is null
+ */
+ public static final byte NULL_ROW = 0x00;
+
+ /**
+ * Denotes that the array is non null
+ */
+ public static final byte NON_NULL_ROW = 0x01;
+
+ /**
+ * Marks the end of the array. Since {@link #NULL_ROW} and {@link #ARRAY_TERMINATOR} will only occur at different
+ * locations, therefore there is no clash in keeping both's values at 0x00
+ */
+ public static final byte ARRAY_TERMINATOR = 0x00;
+
+ private final ColumnValueSelector selector;
+ private final NumericFieldWriterFactory writerFactory;
+
+ /**
+ * Returns the writer for ARRAY<LONG>
+ */
+ public static NumericArrayFieldWriter getLongArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, LongFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<FLOAT>
+ */
+ public static NumericArrayFieldWriter getFloatArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, FloatFieldWriter::forArray);
+ }
+
+ /**
+ * Returns the writer for ARRAY<DOUBLE>
+ */
+ public static NumericArrayFieldWriter getDoubleArrayFieldWriter(final ColumnValueSelector selector)
+ {
+ return new NumericArrayFieldWriter(selector, DoubleFieldWriter::forArray);
+ }
+
+ public NumericArrayFieldWriter(final ColumnValueSelector selector, NumericFieldWriterFactory writerFactory)
+ {
+ this.selector = selector;
+ this.writerFactory = writerFactory;
+ }
+
+ @Override
+ public long writeTo(WritableMemory memory, long position, long maxSize)
+ {
+ Object row = selector.getObject();
+ if (row == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ } else {
+
+ List<? extends Number> list = FrameWriterUtils.getNumericArrayFromNumericArray(row);
+
+ if (list == null) {
+ int requiredSize = Byte.BYTES;
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+ memory.putByte(position, NULL_ROW);
+ return requiredSize;
+ }
+
+ // Create a columnValueSelector to write the individual elements re-using the NumericFieldWriter
+ AtomicInteger index = new AtomicInteger(0);
+ ColumnValueSelector<Number> columnValueSelector = new ColumnValueSelector<Number>()
+ {
+ @Override
+ public double getDouble()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.doubleValue() : 0d;
+ }
+
+ @Override
+ public float getFloat()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.floatValue() : 0f;
+ }
+
+ @Override
+ public long getLong()
+ {
+ final Number n = getObject();
+ assert NullHandling.replaceWithDefault() || n != null;
+ return n != null ? n.longValue() : 0L;
+ }
+
+ @Override
+ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+ {
+
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ // Arrays preserve the individual element's nullity when they are written and read.
+ // Therefore, when working with SQL incompatible mode, [7, null] won't change to [7, 0] when written to and
+ // read from the underlying serialization (as compared with the primitives). Therefore,
+ // even when NullHandling.replaceWithDefault() is true we need to write null as is, and not convert it to their
+ // default value when writing the array. Therefore, the check is `getObject() == null` ignoring the value of
+ // `NullHandling.replaceWithDefaul()`.
+ return getObject() == null;
+ }
+
+ @Nullable
+ @Override
+ public Number getObject()
+ {
+ return list.get(index.get());
+ }
+
+ @Override
+ public Class<? extends Number> classOfObject()
+ {
+ return Number.class;
+ }
+ };
+
+ NumericFieldWriter writer = writerFactory.get(columnValueSelector);
+
+ int requiredSize = Byte.BYTES + (writer.getNumericSizeBytes() + Byte.BYTES) * list.size() + Byte.BYTES;
+
+ if (requiredSize > maxSize) {
+ return -1;
+ }
+
+ long offset = 0;
+ memory.putByte(position + offset, NON_NULL_ROW);
+ offset += Byte.BYTES;
+
+ for (; index.get() < list.size(); index.incrementAndGet()) {
Review Comment:
Yes, but we'll ignore the element of the list we will be iterating over, so I have kept it as is
```
for ( Number ignored : list) {
// processing
}
```
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1343766061
##########
processing/src/main/java/org/apache/druid/frame/field/NumericArrayFieldWriter.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.frame.write.FrameWriterUtils;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.ColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Writes the values of the type ARRAY<X> where X is a numeric type to row based frames.
+ * The format of the array written is as follows:
+ * <p>
+ * Format:
+ * - 1 Byte - {@link #NULL_ROW} or {@link #NON_NULL_ROW} denoting whether the array itself is null
+ * - If the array is null, then the writer stops here
+ * - If the array is not null, then it proceeds to the following steps
+ * <p>
+ * For each value in the non-null array:
+ * - 1 Byte - {@link NumericFieldWriter#ARRAY_ELEMENT_NULL_BYTE} or {@link NumericFieldWriter#ARRAY_ELEMENT_NOT_NULL_BYTE}
+ * denothing whether the proceeding value is null or not.
+ * - ElementSize Bytes - The encoded value of the element
+ * <p>
+ * Once all the values in the non-null arrays are over, writes {@link #ARRAY_TERMINATOR}. This is to aid the byte
+ * comparison, and also let the reader know that the number of elements in the array are over.
+ * <p>
+ * The format doesn't add the number of elements in the array at the beginning, though that would have been more
+ * convenient to keep the written array value byte comparable
Review Comment:
Incorrectly worded statement. I'll change this.
--
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] Field writers for numerical arrays (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #14900:
URL: https://github.com/apache/druid/pull/14900#issuecomment-1744598395
Please also enable the `CalciteArraysQueryTests` with this PR.
--
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] Field writers for numerical arrays (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1343831760
##########
processing/src/test/java/org/apache/druid/frame/field/FloatArrayFieldReaderTest.java:
##########
@@ -0,0 +1,224 @@
+/*
+ * 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.frame.field;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.testing.InitializedNullHandlingTest;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class FloatArrayFieldReaderTest extends InitializedNullHandlingTest
+{
+ private static final long MEMORY_POSITION = 1;
+
+ @Rule
+ public MockitoRule mockitoRule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
+
+ @Mock
+ public ColumnValueSelector writeSelector;
+
+ private WritableMemory memory;
+ private FieldWriter fieldWriter;
+
+ //CHECKSTYLE.OFF: Regexp
+ private static final Object[] FLOATS_ARRAY_1 = new Object[]{
Review Comment:
Can we add a nested array test case as well to all numeric types ?
--
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] Field writers for numerical arrays (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14900:
URL: https://github.com/apache/druid/pull/14900#discussion_r1344741588
##########
processing/src/main/java/org/apache/druid/frame/field/NumericFieldWriter.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.frame.field;
+
+import org.apache.datasketches.memory.WritableMemory;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+/**
+ * FieldWriter for numeric datatypes. The parent class does the null handling for the underlying data, while
+ * the individual subclasses write the individual element (long, float or double type). This also allows for a clean
+ * reuse of the readers and writers between the numeric types and also allowing the array writers ({@link NumericArrayFieldWriter})
+ * to use these methods directly without duplication
+ *
+ * Format:
+ * - 1 byte: Whether the following value is null or not. Take a look at the note on the indicator bytes.
+ * - X bytes: Encoded value of the selector, or the default value if it is null. X denotes the size of the numeric value
+ *
+ * Indicator bytes for denoting whether the element is null or not null changes depending on whether the writer is used
+ * to write the data for individual value (like LONG) or for an element of an array (like ARRAY<LONG>). This is because
+ * array support for the numeric types was added later and by then the field writers for individual fields were using
+ * 0x00 to denote the null byte, which is reserved for denoting the array end when we are writing the elements as part
+ * of the array instead. (0x00 is used for array end because it helps in preserving the byte comparison property of the
+ * numeric array field writers).
+ *
+ * Therefore, to preserve backward and forward compatibility, the individual element's writers were left unchanged,
Review Comment:
1. There is format in the JavaDoc above
2. There is an explanation about the choice of the bytes chosen here
3. Added @see to the `NumericArrayFieldWriter` which contains the info on how this field writer is used for array elements.
Is there anything specific that I am missing here, that would help with clarification?
--
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