You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org> on 2023/05/04 14:29:14 UTC
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #14139: fix issues with filtering nulls on values coerced to numeric types
github-code-scanning[bot] commented on code in PR #14139:
URL: https://github.com/apache/druid/pull/14139#discussion_r1185103894
##########
processing/src/main/java/org/apache/druid/segment/virtual/NestedFieldVirtualColumn.java:
##########
@@ -601,15 +607,138 @@
if (!(theColumn instanceof NestedDataComplexColumn)) {
if (parts.isEmpty()) {
- ColumnCapabilities capabilities = holder.getCapabilities();
if (theColumn instanceof DictionaryEncodedColumn) {
- return ExpressionVectorSelectors.castObjectSelectorToNumeric(
- offset,
- this.columnName,
- theColumn.makeVectorObjectSelector(offset),
- capabilities.toColumnType(),
- expectedType
- );
+ final VectorObjectSelector delegate = theColumn.makeVectorObjectSelector(offset);
+ if (expectedType.is(ValueType.LONG)) {
Review Comment:
## Dereferenced variable may be null
Variable [expectedType](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/4920)
##########
processing/src/main/java/org/apache/druid/segment/nested/VariantArrayColumn.java:
##########
@@ -327,17 +707,17 @@
encodedValueColumn.get(vector, offset.getOffsets(), offset.getCurrentVectorSize());
}
for (int i = 0; i < offset.getCurrentVectorSize(); i++) {
- final int globalId = vector[i];
- if (globalId < adjustArrayId) {
- objects[i] = lookupScalarValue(globalId);
+ final int id = vector[i];
Review Comment:
## Possible confusion of local and field
Confusing name: method [getObjectVector](1) also refers to field [id](2) (without qualifying it with 'this').
[Show more details](https://github.com/apache/druid/security/code-scanning/4916)
##########
processing/src/test/java/org/apache/druid/segment/nested/VariantArrayColumnSupplierTest.java:
##########
@@ -88,41 +94,137 @@
Arrays.asList(null, null)
);
- Closer closer = Closer.create();
+ static List<List<Double>> DOUBLE_ARRAY = Arrays.asList(
+ Collections.emptyList(),
+ Arrays.asList(1.1, null, 2.2),
+ null,
+ Collections.singletonList(null),
+ Arrays.asList(3.3, 4.4),
+ Arrays.asList(null, null)
+ );
- SmooshedFileMapper fileMapper;
+ static List<List<String>> STRING_ARRAY = Arrays.asList(
+ Collections.emptyList(),
+ Arrays.asList("a", null, "b"),
+ null,
+ Collections.singletonList(null),
+ Arrays.asList("c", "d"),
+ Arrays.asList(null, null)
+ );
+
+ static List<Object> VARIANT_NUMERIC = Arrays.asList(
+ 1L,
+ 2.2,
+ null,
+ 3.3,
+ 4L,
+ null
+ );
+
+ static List<Object> VARIANT_SCALAR = Arrays.asList(
+ null,
+ 1L,
+ null,
+ "b",
+ 3.3,
+ 4L
+ );
+
+ static List<Object> VARIANT_SCALAR_AND_ARRAY = Arrays.asList(
+ Collections.emptyList(),
+ 2L,
+ null,
+ Collections.singletonList(null),
+ Arrays.asList(3L, 4L),
+ Arrays.asList(null, "a"),
+ 5.5,
+ "b"
+ );
+
+ static List<List<Object>> VARIANT_ARRAY = Arrays.asList(
+ Collections.emptyList(),
+ Arrays.asList("a", null, "b"),
+ null,
+ Collections.singletonList(null),
+ Arrays.asList(3L, 4L),
+ Arrays.asList(null, 3.3)
+ );
- ByteBuffer baseBuffer;
@BeforeClass
public static void staticSetup()
{
NestedDataModule.registerHandlersAndSerde();
}
+ @Parameterized.Parameters(name = "data = {0}")
+ public static Collection<?> constructorFeeder()
+ {
+ IndexSpec fancy = IndexSpec.builder()
+ .withLongEncoding(CompressionFactory.LongEncodingStrategy.AUTO)
+ .withStringDictionaryEncoding(
+ new StringEncodingStrategy.FrontCoded(16, FrontCodedIndexed.V1)
+ )
+ .build();
+ final List<Object[]> constructors = ImmutableList.of(
+ new Object[]{"ARRAY<LONG>", LONG_ARRAY, IndexSpec.DEFAULT},
+ new Object[]{"ARRAY<LONG>", LONG_ARRAY, fancy},
+ new Object[]{"ARRAY<DOUBLE>", DOUBLE_ARRAY, IndexSpec.DEFAULT},
+ new Object[]{"ARRAY<DOUBLE>", DOUBLE_ARRAY, fancy},
+ new Object[]{"ARRAY<STRING>", STRING_ARRAY, IndexSpec.DEFAULT},
+ new Object[]{"ARRAY<STRING>", STRING_ARRAY, fancy},
+ new Object[]{"DOUBLE,LONG", VARIANT_NUMERIC, IndexSpec.DEFAULT},
+ new Object[]{"DOUBLE,LONG", VARIANT_NUMERIC, fancy},
+ new Object[]{"DOUBLE,LONG,STRING", VARIANT_SCALAR, IndexSpec.DEFAULT},
+ new Object[]{"DOUBLE,LONG,STRING", VARIANT_SCALAR, fancy},
+ new Object[]{"ARRAY<LONG>,ARRAY<STRING>,DOUBLE,LONG,STRING", VARIANT_SCALAR_AND_ARRAY, IndexSpec.DEFAULT},
+ new Object[]{"ARRAY<LONG>,ARRAY<STRING>,DOUBLE,LONG,STRING", VARIANT_SCALAR_AND_ARRAY, fancy},
+ new Object[]{"ARRAY<DOUBLE>,ARRAY<LONG>,ARRAY<STRING>", VARIANT_ARRAY, IndexSpec.DEFAULT},
+ new Object[]{"ARRAY<DOUBLE>,ARRAY<LONG>,ARRAY<STRING>", VARIANT_ARRAY, fancy}
+ );
+
+ return constructors;
+ }
+
+ Closer closer = Closer.create();
+
+ SmooshedFileMapper fileMapper;
+
+ ByteBuffer baseBuffer;
+
+ FieldTypeInfo.MutableTypeSet expectedTypes;
+
+ ColumnType expectedLogicalType = null;
+
+ private final List<?> data;
+ private final IndexSpec indexSpec;
+
+ public VariantArrayColumnSupplierTest(
+ @SuppressWarnings("unused") String name,
Review Comment:
## Useless parameter
The parameter 'name' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/4919)
##########
processing/src/test/java/org/apache/druid/segment/nested/VariantArrayColumnSupplierTest.java:
##########
@@ -245,22 +367,32 @@
SortedMap<String, FieldTypeInfo.MutableTypeSet> fields = column.getFieldTypeInfo();
Assert.assertEquals(
- ImmutableMap.of(NestedPathFinder.JSON_PATH_ROOT, new FieldTypeInfo.MutableTypeSet().add(ColumnType.LONG_ARRAY)),
+ ImmutableMap.of(NestedPathFinder.JSON_PATH_ROOT, expectedType),
fields
);
for (int i = 0; i < data.size(); i++) {
- List<Long> row = data.get(i);
+ Object row = data.get(i);
// in default value mode, even though the input row had an empty string, the selector spits out null, so we want
// to take the null checking path
if (row != null) {
- Assert.assertArrayEquals(row.toArray(), (Object[]) valueSelector.getObject());
+ if (row instanceof List) {
+ Assert.assertArrayEquals(((List) row).toArray(), (Object[]) valueSelector.getObject());
+ } else {
+ Assert.assertEquals(row, valueSelector.getObject());
+ if (expectedLogicalType.isPrimitive()) {
+ Assert.assertEquals(String.valueOf(row), dimensionSelector.lookupName(dimensionSelector.getRow().get(0)));
Review Comment:
## Dereferenced variable may be null
Variable [dimensionSelector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/4921)
##########
processing/src/test/java/org/apache/druid/segment/nested/VariantArrayColumnSupplierTest.java:
##########
@@ -245,22 +367,32 @@
SortedMap<String, FieldTypeInfo.MutableTypeSet> fields = column.getFieldTypeInfo();
Assert.assertEquals(
- ImmutableMap.of(NestedPathFinder.JSON_PATH_ROOT, new FieldTypeInfo.MutableTypeSet().add(ColumnType.LONG_ARRAY)),
+ ImmutableMap.of(NestedPathFinder.JSON_PATH_ROOT, expectedType),
fields
);
for (int i = 0; i < data.size(); i++) {
- List<Long> row = data.get(i);
+ Object row = data.get(i);
// in default value mode, even though the input row had an empty string, the selector spits out null, so we want
// to take the null checking path
if (row != null) {
- Assert.assertArrayEquals(row.toArray(), (Object[]) valueSelector.getObject());
+ if (row instanceof List) {
+ Assert.assertArrayEquals(((List) row).toArray(), (Object[]) valueSelector.getObject());
+ } else {
+ Assert.assertEquals(row, valueSelector.getObject());
+ if (expectedLogicalType.isPrimitive()) {
+ Assert.assertEquals(String.valueOf(row), dimensionSelector.lookupName(dimensionSelector.getRow().get(0)));
+ }
+ }
Assert.assertFalse(nullValueIndex.forNull().computeBitmapResult(resultFactory).get(i));
} else {
Assert.assertNull(valueSelector.getObject());
+ if (expectedLogicalType.isPrimitive()) {
+ Assert.assertNull(dimensionSelector.lookupName(dimensionSelector.getRow().get(0)));
Review Comment:
## Dereferenced variable may be null
Variable [dimensionSelector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/4922)
##########
processing/src/main/java/org/apache/druid/segment/nested/VariantArrayColumnSerializer.java:
##########
@@ -276,10 +282,35 @@
(id) -> indexSpec.getBitmapSerdeFactory().getBitmapFactory().makeEmptyMutableBitmap()
).add(rowCount);
}
+ final int dictId = globalIds == null ? 0 : dictionaryIdLookup.lookupArray(globalIds);
Review Comment:
## Useless null check
This check is useless. [globalIds](1) cannot be null at this check, since [new int\[\]](2) always is non-null.
[Show more details](https://github.com/apache/druid/security/code-scanning/4917)
--
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