Ali Alsuliman has submitted this change and it was merged. ( https://asterix-gerrit.ics.uci.edu/3404 )
Change subject: [ASTERIXDB-2547][COMP] Disallow passing UNION tag to get comparator ...................................................................... [ASTERIXDB-2547][COMP] Disallow passing UNION tag to get comparator - user model changes: no - storage format changes: no - interface changes: no Details: UNION should not be allowed when getting a comparator by tag. The comparator provider returns a generic comparator with types ANY when UNION is passed as a tag. This could cause problems if the actual type is a complex type. UNION should not be allowed. Change-Id: Id8816a0dc5584f0a27410c512f3a44ccfc6c3151 Reviewed-on: https://asterix-gerrit.ics.uci.edu/3404 Contrib: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Reviewed-by: Dmitry Lychagin <[email protected]> --- M asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java M asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/RangeMapBuilder.java M asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/BinaryComparatorFactoryProvider.java 3 files changed, 23 insertions(+), 12 deletions(-) Approvals: Jenkins: Verified; ; Verified Anon. E. Moose (1000171): Dmitry Lychagin: Looks good to me, approved Objections: Jenkins: Violations found diff --git a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java index b8bfffd..c6c8a2f 100644 --- a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java +++ b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java @@ -1252,7 +1252,7 @@ if (oc.getRangeMap() != null) { Iterator<OrderModifier> orderModifIter = oc.getModifierList().iterator(); boolean ascending = orderModifIter.next() == OrderModifier.ASC; - RangeMapBuilder.verifyRangeOrder(oc.getRangeMap(), ascending); + RangeMapBuilder.verifyRangeOrder(oc.getRangeMap(), ascending, sourceLoc); ord.getAnnotations().put(OperatorAnnotations.USE_STATIC_RANGE, oc.getRangeMap()); } return new Pair<>(ord, null); diff --git a/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/RangeMapBuilder.java b/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/RangeMapBuilder.java index c505c1c..a26c94d 100644 --- a/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/RangeMapBuilder.java +++ b/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/RangeMapBuilder.java @@ -23,6 +23,7 @@ import org.apache.asterix.common.exceptions.CompilationException; import org.apache.asterix.common.exceptions.ErrorCode; +import org.apache.asterix.common.exceptions.RuntimeDataException; import org.apache.asterix.formats.nontagged.BinaryComparatorFactoryProvider; import org.apache.asterix.formats.nontagged.SerializerDeserializerProvider; import org.apache.asterix.lang.common.base.Expression; @@ -50,6 +51,7 @@ import org.apache.hyracks.api.dataflow.value.IBinaryComparatorFactory; import org.apache.hyracks.api.dataflow.value.ISerializerDeserializer; import org.apache.hyracks.api.exceptions.HyracksDataException; +import org.apache.hyracks.api.exceptions.SourceLocation; import org.apache.hyracks.data.std.util.ArrayBackedValueStorage; import org.apache.hyracks.dataflow.common.data.partition.range.RangeMap; @@ -145,19 +147,25 @@ } } - public static void verifyRangeOrder(RangeMap rangeMap, boolean ascending) throws CompilationException { + public static void verifyRangeOrder(RangeMap rangeMap, boolean ascending, SourceLocation sourceLoc) + throws CompilationException { // TODO Add support for composite fields. int fieldIndex = 0; int fieldType = rangeMap.getTag(0, 0); BinaryComparatorFactoryProvider comparatorFactory = BinaryComparatorFactoryProvider.INSTANCE; - IBinaryComparatorFactory bcf = - comparatorFactory.getBinaryComparatorFactory(ATypeTag.VALUE_TYPE_MAPPING[fieldType], ascending); + IBinaryComparatorFactory bcf; + try { + bcf = comparatorFactory.getBinaryComparatorFactory(ATypeTag.VALUE_TYPE_MAPPING[fieldType], ascending); + } catch (RuntimeDataException e) { + throw new CompilationException(ErrorCode.COMPILATION_ERROR, sourceLoc, e.getMessage()); + } IBinaryComparator comparator = bcf.createBinaryComparator(); int c = 0; for (int split = 1; split < rangeMap.getSplitCount(); ++split) { if (fieldType != rangeMap.getTag(fieldIndex, split)) { - throw new CompilationException("Range field contains more than a single type of items (" + fieldType - + " and " + rangeMap.getTag(fieldIndex, split) + ")."); + throw new CompilationException(ErrorCode.COMPILATION_ERROR, sourceLoc, + "Range field contains more than a single type of items (" + fieldType + " and " + + rangeMap.getTag(fieldIndex, split) + ")."); } int previousSplit = split - 1; try { @@ -165,10 +173,11 @@ rangeMap.getLength(fieldIndex, previousSplit), rangeMap.getByteArray(), rangeMap.getStartOffset(fieldIndex, split), rangeMap.getLength(fieldIndex, split)); } catch (HyracksDataException e) { - throw new CompilationException(e); + throw new CompilationException(ErrorCode.COMPILATION_ERROR, sourceLoc, e.getMessage()); } if (c >= 0) { - throw new CompilationException("Range fields are not in sorted order."); + throw new CompilationException(ErrorCode.COMPILATION_ERROR, sourceLoc, + "Range fields are not in sorted order."); } } } 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 396bf3b..eea9fed 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 @@ -20,6 +20,8 @@ import java.io.Serializable; +import org.apache.asterix.common.exceptions.ErrorCode; +import org.apache.asterix.common.exceptions.RuntimeDataException; import org.apache.asterix.dataflow.data.nontagged.comparators.ACirclePartialBinaryComparatorFactory; import org.apache.asterix.dataflow.data.nontagged.comparators.ADurationPartialBinaryComparatorFactory; import org.apache.asterix.dataflow.data.nontagged.comparators.AGenericAscBinaryComparatorFactory; @@ -112,13 +114,13 @@ return createGenericBinaryComparatorFactory((IAType) leftType, (IAType) rightType, ascending); } - public IBinaryComparatorFactory getBinaryComparatorFactory(ATypeTag type, boolean ascending) { + public IBinaryComparatorFactory getBinaryComparatorFactory(ATypeTag type, boolean ascending) + throws RuntimeDataException { 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(BuiltinType.ANY, BuiltinType.ANY, ascending); + case UNION: + throw new RuntimeDataException(ErrorCode.TYPE_UNSUPPORTED, "Comparator", type); case NULL: case MISSING: return new AnyBinaryComparatorFactory(); -- To view, visit https://asterix-gerrit.ics.uci.edu/3404 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id8816a0dc5584f0a27410c512f3a44ccfc6c3151 Gerrit-Change-Number: 3404 Gerrit-PatchSet: 4 Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose (1000171) Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]>
