You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/07/29 01:33:41 UTC
[GitHub] [druid] clintropolis opened a new pull request, #12834: add missing selectors for explicit null columns
clintropolis opened a new pull request, #12834:
URL: https://github.com/apache/druid/pull/12834
### Description
This PR adds some missing column selectors to `NullColumnPartSerde` used by explicit null columns (added in #12279). The `VectorObjectSelector` is used by many vector aggregators when they don't care to deal with dictionary id lookup, so this fixes issues when using these aggregators with explicit null columns. I'm not certain if the `VectorValueSelector` is actually used in practice, but went ahead and added it just in case.
This PR has:
- [x] been self-reviewed.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [x] 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
[GitHub] [druid] rohangarg commented on a diff in pull request #12834: add missing selectors for explicit null columns
Posted by GitBox <gi...@apache.org>.
rohangarg commented on code in PR #12834:
URL: https://github.com/apache/druid/pull/12834#discussion_r932939379
##########
processing/src/test/java/org/apache/druid/segment/serde/NullColumnPartSerdeTest.java:
##########
@@ -61,4 +76,128 @@ public void testDeserializer()
Assert.assertTrue(columnCapabilities.areDictionaryValuesSorted().isTrue());
Assert.assertTrue(columnCapabilities.areDictionaryValuesUnique().isTrue());
}
+
+ @Test
+ public void testDimensionSelector()
+ {
+ final NullColumnPartSerde partSerde = new NullColumnPartSerde(10, new RoaringBitmapSerdeFactory(null));
+ final ColumnBuilder builder = new ColumnBuilder().setType(ValueType.STRING);
+ partSerde.getDeserializer().read(Mockito.mock(ByteBuffer.class), builder, Mockito.mock(ColumnConfig.class));
+ ColumnHolder holder = builder.build();
+
+ BaseColumn theColumn = holder.getColumn();
+ Assert.assertTrue(theColumn instanceof DictionaryEncodedColumn);
+ DictionaryEncodedColumn dictionaryEncodedColumn = (DictionaryEncodedColumn) theColumn;
+
+ ReadableOffset offset = new SimpleAscendingOffset(10);
+ DimensionSelector dimensionSelector = dictionaryEncodedColumn.makeDimensionSelector(
+ offset,
+ null
+ );
+ Assert.assertNull(dimensionSelector.getObject());
+ Assert.assertEquals(1, dimensionSelector.getRow().size());
+ Assert.assertEquals(0, dimensionSelector.getRow().get(0));
+ }
+
+ @Test
+ public void testDimensionVectorSelector()
+ {
+ final NullColumnPartSerde partSerde = new NullColumnPartSerde(10, new RoaringBitmapSerdeFactory(null));
+ final ColumnBuilder builder = new ColumnBuilder().setType(ValueType.STRING);
+ partSerde.getDeserializer().read(Mockito.mock(ByteBuffer.class), builder, Mockito.mock(ColumnConfig.class));
+ ColumnHolder holder = builder.build();
+
+ BaseColumn theColumn = holder.getColumn();
+ Assert.assertTrue(theColumn instanceof DictionaryEncodedColumn);
+ DictionaryEncodedColumn dictionaryEncodedColumn = (DictionaryEncodedColumn) theColumn;
+
+ ReadableVectorOffset vectorOffset = new NoFilterVectorOffset(8, 0, 10);
+
+ SingleValueDimensionVectorSelector vectorSelector =
+ dictionaryEncodedColumn.makeSingleValueDimensionVectorSelector(vectorOffset);
+
+ int[] rowVector = vectorSelector.getRowVector();
+ for (int i = 0; i < vectorOffset.getCurrentVectorSize(); i++) {
+ Assert.assertEquals(0, rowVector[i]);
+ Assert.assertNull(vectorSelector.lookupName(rowVector[i]));
+ }
+
+ Assert.assertThrows(UnsupportedOperationException.class, () -> {
+ dictionaryEncodedColumn.makeMultiValueDimensionVectorSelector(vectorOffset);
Review Comment:
is there any specific reason to not support multi-value selector?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #12834: add missing selectors for explicit null columns
Posted by GitBox <gi...@apache.org>.
clintropolis merged PR #12834:
URL: https://github.com/apache/druid/pull/12834
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12834: add missing selectors for explicit null columns
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12834:
URL: https://github.com/apache/druid/pull/12834#discussion_r932943344
##########
processing/src/test/java/org/apache/druid/segment/serde/NullColumnPartSerdeTest.java:
##########
@@ -61,4 +76,128 @@ public void testDeserializer()
Assert.assertTrue(columnCapabilities.areDictionaryValuesSorted().isTrue());
Assert.assertTrue(columnCapabilities.areDictionaryValuesUnique().isTrue());
}
+
+ @Test
+ public void testDimensionSelector()
+ {
+ final NullColumnPartSerde partSerde = new NullColumnPartSerde(10, new RoaringBitmapSerdeFactory(null));
+ final ColumnBuilder builder = new ColumnBuilder().setType(ValueType.STRING);
+ partSerde.getDeserializer().read(Mockito.mock(ByteBuffer.class), builder, Mockito.mock(ColumnConfig.class));
+ ColumnHolder holder = builder.build();
+
+ BaseColumn theColumn = holder.getColumn();
+ Assert.assertTrue(theColumn instanceof DictionaryEncodedColumn);
+ DictionaryEncodedColumn dictionaryEncodedColumn = (DictionaryEncodedColumn) theColumn;
+
+ ReadableOffset offset = new SimpleAscendingOffset(10);
+ DimensionSelector dimensionSelector = dictionaryEncodedColumn.makeDimensionSelector(
+ offset,
+ null
+ );
+ Assert.assertNull(dimensionSelector.getObject());
+ Assert.assertEquals(1, dimensionSelector.getRow().size());
+ Assert.assertEquals(0, dimensionSelector.getRow().get(0));
+ }
+
+ @Test
+ public void testDimensionVectorSelector()
+ {
+ final NullColumnPartSerde partSerde = new NullColumnPartSerde(10, new RoaringBitmapSerdeFactory(null));
+ final ColumnBuilder builder = new ColumnBuilder().setType(ValueType.STRING);
+ partSerde.getDeserializer().read(Mockito.mock(ByteBuffer.class), builder, Mockito.mock(ColumnConfig.class));
+ ColumnHolder holder = builder.build();
+
+ BaseColumn theColumn = holder.getColumn();
+ Assert.assertTrue(theColumn instanceof DictionaryEncodedColumn);
+ DictionaryEncodedColumn dictionaryEncodedColumn = (DictionaryEncodedColumn) theColumn;
+
+ ReadableVectorOffset vectorOffset = new NoFilterVectorOffset(8, 0, 10);
+
+ SingleValueDimensionVectorSelector vectorSelector =
+ dictionaryEncodedColumn.makeSingleValueDimensionVectorSelector(vectorOffset);
+
+ int[] rowVector = vectorSelector.getRowVector();
+ for (int i = 0; i < vectorOffset.getCurrentVectorSize(); i++) {
+ Assert.assertEquals(0, rowVector[i]);
+ Assert.assertNull(vectorSelector.lookupName(rowVector[i]));
+ }
+
+ Assert.assertThrows(UnsupportedOperationException.class, () -> {
+ dictionaryEncodedColumn.makeMultiValueDimensionVectorSelector(vectorOffset);
Review Comment:
yeah, we shouldn't use this selector because they cannot be multi-valued because there is only a null value for every row. The single value selectors are more optimized, this is mimicking how actually missing columns behave https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/ColumnProcessors.java#L329
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] rohangarg commented on a diff in pull request #12834: add missing selectors for explicit null columns
Posted by GitBox <gi...@apache.org>.
rohangarg commented on code in PR #12834:
URL: https://github.com/apache/druid/pull/12834#discussion_r932950545
##########
processing/src/test/java/org/apache/druid/segment/serde/NullColumnPartSerdeTest.java:
##########
@@ -61,4 +76,128 @@ public void testDeserializer()
Assert.assertTrue(columnCapabilities.areDictionaryValuesSorted().isTrue());
Assert.assertTrue(columnCapabilities.areDictionaryValuesUnique().isTrue());
}
+
+ @Test
+ public void testDimensionSelector()
+ {
+ final NullColumnPartSerde partSerde = new NullColumnPartSerde(10, new RoaringBitmapSerdeFactory(null));
+ final ColumnBuilder builder = new ColumnBuilder().setType(ValueType.STRING);
+ partSerde.getDeserializer().read(Mockito.mock(ByteBuffer.class), builder, Mockito.mock(ColumnConfig.class));
+ ColumnHolder holder = builder.build();
+
+ BaseColumn theColumn = holder.getColumn();
+ Assert.assertTrue(theColumn instanceof DictionaryEncodedColumn);
+ DictionaryEncodedColumn dictionaryEncodedColumn = (DictionaryEncodedColumn) theColumn;
+
+ ReadableOffset offset = new SimpleAscendingOffset(10);
+ DimensionSelector dimensionSelector = dictionaryEncodedColumn.makeDimensionSelector(
+ offset,
+ null
+ );
+ Assert.assertNull(dimensionSelector.getObject());
+ Assert.assertEquals(1, dimensionSelector.getRow().size());
+ Assert.assertEquals(0, dimensionSelector.getRow().get(0));
+ }
+
+ @Test
+ public void testDimensionVectorSelector()
+ {
+ final NullColumnPartSerde partSerde = new NullColumnPartSerde(10, new RoaringBitmapSerdeFactory(null));
+ final ColumnBuilder builder = new ColumnBuilder().setType(ValueType.STRING);
+ partSerde.getDeserializer().read(Mockito.mock(ByteBuffer.class), builder, Mockito.mock(ColumnConfig.class));
+ ColumnHolder holder = builder.build();
+
+ BaseColumn theColumn = holder.getColumn();
+ Assert.assertTrue(theColumn instanceof DictionaryEncodedColumn);
+ DictionaryEncodedColumn dictionaryEncodedColumn = (DictionaryEncodedColumn) theColumn;
+
+ ReadableVectorOffset vectorOffset = new NoFilterVectorOffset(8, 0, 10);
+
+ SingleValueDimensionVectorSelector vectorSelector =
+ dictionaryEncodedColumn.makeSingleValueDimensionVectorSelector(vectorOffset);
+
+ int[] rowVector = vectorSelector.getRowVector();
+ for (int i = 0; i < vectorOffset.getCurrentVectorSize(); i++) {
+ Assert.assertEquals(0, rowVector[i]);
+ Assert.assertNull(vectorSelector.lookupName(rowVector[i]));
+ }
+
+ Assert.assertThrows(UnsupportedOperationException.class, () -> {
+ dictionaryEncodedColumn.makeMultiValueDimensionVectorSelector(vectorOffset);
Review Comment:
Thanks for the clarification, makes sense
Also, just checked the `NullColumnPartSerde#getDeserializer` sets `setHasMultipleValues` as `false` which should never invoke the creation of MVD selector.
--
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