Ali Alsuliman has uploaded this change for review. (
https://asterix-gerrit.ics.uci.edu/3336
Change subject: [ASTERIXDB-2516][RT] Prevent passing null type to comparator
provider
......................................................................
[ASTERIXDB-2516][RT] Prevent passing null type to comparator provider
- user model changes: no
- storage format changes: no
- interface changes: no
Details:
This change is to disallow passing null as IAType to the comparator
provider when asking for a comparator.
- changed the generic comparator to check whether the tag of data
at runtime is a vlid tag.
- modified the comparator provider to return non-tagged comparator
for IAType SHORTWITHOUTINFOTYPE which is a short without tag.
SHORTWITHOUTINFOTYPE should not use the generic comparator since
the input data has no tag.
- fixed Dataset class to consider external dataset when getting
the IAType of the primary keys. The primary keys for external
datasets are different from regular datasets. They are not
part of the record type. Previously, null would be returned.
This would cause a failure when getting a comparator for the
primary keys of an external dataset since the type passed is null.
Change-Id: I37767a3f3d1e3b29597d2a4998c0b60005cadb09
---
M
asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java
M
asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparator.java
M
asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparatorFactory.java
M
asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/hash/AMurmurHash3BinaryHashFunctionFamily.java
M
asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/BinaryComparatorFactoryProvider.java
5 files changed, 67 insertions(+), 60 deletions(-)
git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb
refs/changes/36/3336/1
diff --git
a/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java
b/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java
index 855cf78..d207f7f 100644
---
a/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java
+++
b/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java
@@ -740,8 +740,10 @@
// Set the serde/traits for primary keys
for (int i = 0; i < numPrimaryKeys; i++) {
IAType keyType =
- (indicators == null || indicators.get(i) == 0) ?
itemType.getSubFieldType(partitioningKeys.get(i))
- :
metaType.getSubFieldType(partitioningKeys.get(i));
+ datasetType == DatasetType.EXTERNAL ?
IndexingConstants.getFieldType(i)
+ : (indicators == null || indicators.get(i) == 0)
+ ?
itemType.getSubFieldType(partitioningKeys.get(i))
+ :
metaType.getSubFieldType(partitioningKeys.get(i));
primaryRecFields[i] =
serdeProvider.getSerializerDeserializer(keyType);
primaryTypeTraits[i] =
TypeTraitProvider.INSTANCE.getTypeTrait(keyType);
}
@@ -778,8 +780,9 @@
indicators = ((InternalDatasetDetails)
getDatasetDetails()).getKeySourceIndicator();
}
for (int i = 0; i < numPrimaryKeys; i++) {
- IAType keyType =
- (indicators == null || indicators.get(i) == 0) ?
recordType.getSubFieldType(partitioningKeys.get(i))
+ IAType keyType = datasetType == DatasetType.EXTERNAL ?
IndexingConstants.getFieldType(i)
+ : (indicators == null || indicators.get(i) == 0)
+ ?
recordType.getSubFieldType(partitioningKeys.get(i))
:
metaType.getSubFieldType(partitioningKeys.get(i));
cmpFactories[i] =
cmpFactoryProvider.getBinaryComparatorFactory(keyType, true);
}
@@ -806,8 +809,9 @@
indicators = ((InternalDatasetDetails)
getDatasetDetails()).getKeySourceIndicator();
}
for (int i = 0; i < numPrimaryKeys; i++) {
- IAType keyType =
- (indicators == null || indicators.get(i) == 0) ?
recordType.getSubFieldType(partitioningKeys.get(i))
+ IAType keyType = datasetType == DatasetType.EXTERNAL ?
IndexingConstants.getFieldType(i)
+ : (indicators == null || indicators.get(i) == 0)
+ ?
recordType.getSubFieldType(partitioningKeys.get(i))
:
metaType.getSubFieldType(partitioningKeys.get(i));
hashFuncFactories[i] =
BinaryHashFunctionFactoryProvider.INSTANCE.getBinaryHashFunctionFactory(keyType);
}
diff --git
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparator.java
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparator.java
index 4510310..e907f75 100644
---
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparator.java
+++
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparator.java
@@ -62,7 +62,7 @@
new
PointableBinaryComparatorFactory(UTF8StringPointable.FACTORY).createBinaryComparator();
private final IBinaryComparator ascByteArrayComp =
new
PointableBinaryComparatorFactory(ByteArrayPointable.FACTORY).createBinaryComparator();
- // the type fields can be null
+
protected final IAType leftType;
protected final IAType rightType;
private final ListObjectPool<IMutableValueStorage, Void> storageAllocator
= new ListObjectPool<>(STORAGE_FACTORY);
@@ -70,28 +70,27 @@
private final ListObjectPool<SortedRecord, ARecordType> recordPool = new
ListObjectPool<>(RECORD_FACTORY);
AbstractAGenericBinaryComparator(IAType leftType, IAType rightType) {
- // factory should have already made sure to get the actual type
+ // factory should have already made sure to get the actual type (and
no null types)
this.leftType = leftType;
this.rightType = rightType;
}
protected final int compare(IAType leftType, byte[] b1, int s1, int l1,
IAType rightType, byte[] b2, int s2, int l2)
throws HyracksDataException {
- if (b1[s1] == ATypeTag.SERIALIZED_MISSING_TYPE_TAG) {
- return b2[s2] == ATypeTag.SERIALIZED_MISSING_TYPE_TAG ? 0 : -1;
- } else if (b2[s2] == ATypeTag.SERIALIZED_MISSING_TYPE_TAG) {
- return 1;
- }
- if (b1[s1] == ATypeTag.SERIALIZED_NULL_TYPE_TAG) {
- return b2[s2] == ATypeTag.SERIALIZED_NULL_TYPE_TAG ? 0 : -1;
- } else if (b2[s2] == ATypeTag.SERIALIZED_NULL_TYPE_TAG) {
- return 1;
- }
ATypeTag tag1 =
EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(b1[s1]);
ATypeTag tag2 =
EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(b2[s2]);
- // tag being null could mean several things among of which is that the
passed args are not tagged
if (tag1 == null || tag2 == null) {
- return RawBinaryComparatorFactory.compare(b1, s1, l1, b2, s2, l2);
+ throw new IllegalStateException("Could not recognize the type of
data.");
+ }
+ if (tag1 == ATypeTag.MISSING) {
+ return tag2 == ATypeTag.MISSING ? 0 : -1;
+ } else if (tag2 == ATypeTag.MISSING) {
+ return 1;
+ }
+ if (tag1 == ATypeTag.NULL) {
+ return tag2 == ATypeTag.NULL ? 0 : -1;
+ } else if (tag2 == ATypeTag.NULL) {
+ return 1;
}
if (ATypeHierarchy.isCompatible(tag1, tag2) &&
ATypeHierarchy.getTypeDomain(tag1) == Domain.NUMERIC) {
return ComparatorUtil.compareNumbers(tag1, b1, s1 + 1, tag2, b2,
s2 + 1);
@@ -142,9 +141,9 @@
case BINARY:
return ascByteArrayComp.compare(b1, s1 + 1, l1 - 1, b2, s2 +
1, l2 - 1);
case ARRAY:
- return compareArrays(leftType, b1, s1, l1, rightType, b2, s2,
l2);
+ return compareArrays(leftType, b1, s1, rightType, b2, s2);
case OBJECT:
- return compareRecords(leftType, b1, s1, l1, rightType, b2, s2,
l2);
+ return compareRecords(leftType, b1, s1, rightType, b2, s2);
default:
return RawBinaryComparatorFactory.compare(b1, s1, l1, b2, s2,
l2);
}
@@ -154,11 +153,8 @@
return AIntervalAscPartialBinaryComparatorFactory.compare(b1, s1 + 1,
l1 - 1, b2, s2 + 1, l2 - 1);
}
- private int compareArrays(IAType leftType, byte[] b1, int s1, int l1,
IAType rightType, byte[] b2, int s2, int l2)
+ private int compareArrays(IAType leftType, byte[] b1, int s1, IAType
rightType, byte[] b2, int s2)
throws HyracksDataException {
- if (leftType == null || rightType == null) {
- return RawBinaryComparatorFactory.compare(b1, s1, l1, b2, s2, l2);
- }
int leftNumItems = ListAccessorUtil.numberOfItems(b1, s1);
int rightNumItems = ListAccessorUtil.numberOfItems(b2, s2);
IAType leftArrayType = TypeComputeUtils.getActualTypeOrOpen(leftType,
ATypeTag.ARRAY);
@@ -195,11 +191,8 @@
}
}
- private int compareRecords(IAType leftType, byte[] b1, int s1, int l1,
IAType rightType, byte[] b2, int s2, int l2)
+ private int compareRecords(IAType leftType, byte[] b1, int s1, IAType
rightType, byte[] b2, int s2)
throws HyracksDataException {
- if (leftType == null || rightType == null) {
- return RawBinaryComparatorFactory.compare(b1, s1, l1, b2, s2, l2);
- }
ARecordType leftRecordType = (ARecordType)
TypeComputeUtils.getActualTypeOrOpen(leftType, ATypeTag.OBJECT);
ARecordType rightRecordType = (ARecordType)
TypeComputeUtils.getActualTypeOrOpen(rightType, ATypeTag.OBJECT);
SortedRecord leftRecord = recordPool.allocate(leftRecordType);
diff --git
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparatorFactory.java
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparatorFactory.java
index 140db22..c743e6e 100644
---
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparatorFactory.java
+++
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AbstractAGenericBinaryComparatorFactory.java
@@ -19,6 +19,7 @@
package org.apache.asterix.dataflow.data.nontagged.comparators;
import org.apache.asterix.om.typecomputer.impl.TypeComputeUtils;
+import org.apache.asterix.om.types.BuiltinType;
import org.apache.asterix.om.types.IAType;
import org.apache.hyracks.api.dataflow.value.IBinaryComparatorFactory;
import org.apache.hyracks.api.exceptions.HyracksDataException;
@@ -28,36 +29,32 @@
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
-public abstract class AbstractAGenericBinaryComparatorFactory implements
IBinaryComparatorFactory {
+abstract class AbstractAGenericBinaryComparatorFactory implements
IBinaryComparatorFactory {
private static final long serialVersionUID = 1L;
- // these fields can be null
- protected final IAType leftType;
- protected final IAType rightType;
+ final IAType leftType;
+ final IAType rightType;
AbstractAGenericBinaryComparatorFactory(IAType leftType, IAType rightType)
{
- this.leftType = leftType == null ? null :
TypeComputeUtils.getActualType(leftType);
- this.rightType = rightType == null ? null :
TypeComputeUtils.getActualType(rightType);
+ this.leftType = TypeComputeUtils.getActualType(leftType);
+ this.rightType = TypeComputeUtils.getActualType(rightType);
}
JsonNode convertToJson(IPersistedResourceRegistry registry, Class<?
extends IJsonSerializable> clazz, long version)
throws HyracksDataException {
ObjectNode jsonNode = registry.getClassIdentifier(clazz, version);
- if (leftType != null) {
- jsonNode.set("leftType", leftType.toJson(registry));
- }
- if (rightType != null) {
- jsonNode.set("rightType", rightType.toJson(registry));
- }
+ jsonNode.set("leftType", leftType.toJson(registry));
+ jsonNode.set("rightType", rightType.toJson(registry));
return jsonNode;
}
static IJsonSerializable convertToObject(IPersistedResourceRegistry
registry, JsonNode json, boolean asc)
throws HyracksDataException {
- JsonNode leftNode = json.get("leftType");
- JsonNode rightNode = json.get("rightType");
- IAType leftType = leftNode == null || leftNode.isNull() ? null :
(IAType) registry.deserialize(leftNode);
- IAType rightType = rightNode == null || rightNode.isNull() ? null :
(IAType) registry.deserialize(rightNode);
+ JsonNode left = json.get("leftType");
+ JsonNode right = json.get("rightType");
+ // default to ANY for comparators that didn't originally have the new
type fields
+ IAType leftType = left == null || left.isNull() ? BuiltinType.ANY :
(IAType) registry.deserialize(left);
+ IAType rightType = right == null || right.isNull() ? BuiltinType.ANY :
(IAType) registry.deserialize(right);
return asc ? new AGenericAscBinaryComparatorFactory(leftType,
rightType)
: new AGenericDescBinaryComparatorFactory(leftType, rightType);
}
diff --git
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/hash/AMurmurHash3BinaryHashFunctionFamily.java
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/hash/AMurmurHash3BinaryHashFunctionFamily.java
index 020fbd1..f02c764 100644
---
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/hash/AMurmurHash3BinaryHashFunctionFamily.java
+++
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/hash/AMurmurHash3BinaryHashFunctionFamily.java
@@ -122,22 +122,19 @@
valueBuffer.getLength(), seed);
case ARRAY:
try {
- return hashArray(type, bytes, offset, length);
+ return hashArray(type, bytes, offset);
} catch (IOException e) {
throw HyracksDataException.create(e);
}
case OBJECT:
- return hashRecord(type, bytes, offset, length);
+ return hashRecord(type, bytes, offset);
case DOUBLE:
default:
return MurmurHash3BinaryHash.hash(bytes, offset, length,
seed);
}
}
- private int hashArray(IAType type, byte[] bytes, int offset, int
length) throws IOException {
- if (type == null) {
- return MurmurHash3BinaryHash.hash(bytes, offset, length, seed);
- }
+ private int hashArray(IAType type, byte[] bytes, int offset) throws
IOException {
IAType arrayType = TypeComputeUtils.getActualTypeOrOpen(type,
ATypeTag.ARRAY);
IAType itemType = ((AbstractCollectionType)
arrayType).getItemType();
ATypeTag itemTag = itemType.getTypeTag();
@@ -158,10 +155,7 @@
return hash;
}
- private int hashRecord(IAType type, byte[] bytes, int offset, int
length) throws HyracksDataException {
- if (type == null) {
- return MurmurHash3BinaryHash.hash(bytes, offset, length, seed);
- }
+ private int hashRecord(IAType type, byte[] bytes, int offset) throws
HyracksDataException {
ARecordType recordType = (ARecordType)
TypeComputeUtils.getActualTypeOrOpen(type, ATypeTag.OBJECT);
SortedRecord record = recordPool.allocate(recordType);
IPointable fieldValue = voidPointableAllocator.allocate(null);
diff --git
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/BinaryComparatorFactoryProvider.java
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/BinaryComparatorFactoryProvider.java
index 7567e0c..b3a6d27 100644
---
a/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/BinaryComparatorFactoryProvider.java
+++
b/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/BinaryComparatorFactoryProvider.java
@@ -33,6 +33,7 @@
import
org.apache.asterix.dataflow.data.nontagged.comparators.ARectanglePartialBinaryComparatorFactory;
import
org.apache.asterix.dataflow.data.nontagged.comparators.AUUIDPartialBinaryComparatorFactory;
import org.apache.asterix.om.types.ATypeTag;
+import org.apache.asterix.om.types.BuiltinType;
import org.apache.asterix.om.types.IAType;
import org.apache.hyracks.algebricks.data.IBinaryComparatorFactoryProvider;
import org.apache.hyracks.api.dataflow.value.IBinaryComparatorFactory;
@@ -91,19 +92,23 @@
@Override
public IBinaryComparatorFactory getBinaryComparatorFactory(Object
leftType, Object rightType, boolean ascending,
boolean ignoreCase) {
- if (leftType == null || rightType == null) {
- return createGenericBinaryComparatorFactory(null, null, ascending);
- }
IAType left = (IAType) leftType;
IAType right = (IAType) rightType;
+ // TODO(ali): what if someone passed ignoreCase=true and type ANY (at
runtime it could be a string)?
if (left.getTypeTag() == ATypeTag.STRING && right.getTypeTag() ==
ATypeTag.STRING && ignoreCase) {
return addOffset(UTF8STRING_LOWERCASE_POINTABLE_INSTANCE,
ascending);
+ }
+ if (isShortWithoutTag(left, right)) {
+ return SHORT_POINTABLE_INSTANCE;
}
return createGenericBinaryComparatorFactory(left, right, ascending);
}
@Override
public IBinaryComparatorFactory getBinaryComparatorFactory(Object
leftType, Object rightType, boolean ascending) {
+ if (isShortWithoutTag((IAType) leftType, (IAType) rightType)) {
+ return SHORT_POINTABLE_INSTANCE;
+ }
// During a comparison, since proper type promotion among several
numeric types are required,
// we will use AGenericAscBinaryComparatorFactory, instead of using a
specific comparator
return createGenericBinaryComparatorFactory((IAType) leftType,
(IAType) rightType, ascending);
@@ -113,8 +118,9 @@
switch (type) {
case ANY:
case UNION:
+ // i think UNION shouldn't be allowed. the actual type could
be closed array or record. ANY would fail.
// we could do smth better for nullable fields
- return createGenericBinaryComparatorFactory(null, null,
ascending);
+ return createGenericBinaryComparatorFactory(BuiltinType.ANY,
BuiltinType.ANY, ascending);
case NULL:
case MISSING:
return new AnyBinaryComparatorFactory();
@@ -186,4 +192,17 @@
return AIntervalDescPartialBinaryComparatorFactory.INSTANCE;
}
}
+
+ private static boolean isShortWithoutTag(IAType left, IAType right) {
+ ATypeTag leftTag = left.getTypeTag();
+ ATypeTag rightTag = right.getTypeTag();
+ if (leftTag != ATypeTag.SHORTWITHOUTTYPEINFO && rightTag !=
ATypeTag.SHORTWITHOUTTYPEINFO) {
+ return false;
+ }
+ if (leftTag != rightTag) {
+ // this should not happen (i.e. comparing untagged (short without
tag) vs some tagged)
+ throw new IllegalStateException();
+ }
+ return true;
+ }
}
--
To view, visit https://asterix-gerrit.ics.uci.edu/3336
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I37767a3f3d1e3b29597d2a4998c0b60005cadb09
Gerrit-Change-Number: 3336
Gerrit-PatchSet: 1
Gerrit-Owner: Ali Alsuliman <[email protected]>
