Dmitry Lychagin has submitted this change and it was merged. Change subject: [ASTERIXDB-2529][COMP] Incorrect result with MISSING field value ......................................................................
[ASTERIXDB-2529][COMP] Incorrect result with MISSING field value - user model changes: no - storage format changes: no - interface changes: no Details: - Fixed StaticTypeCastUtil.staticRecordTypeCast() to correctly handle MISSING fields in record constructors Change-Id: I5d3435274ebf0007fe7e63b86264337072fd8305 Reviewed-on: https://asterix-gerrit.ics.uci.edu/3269 Sonar-Qube: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Contrib: Jenkins <[email protected]> Reviewed-by: Ali Alsuliman <[email protected]> --- M asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/typecast/StaticTypeCastUtil.java M asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/ObjectsQueries.xml A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.query.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/results/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.adm 4 files changed, 99 insertions(+), 45 deletions(-) Approvals: Ali Alsuliman: Looks good to me, approved Jenkins: Verified; No violations found; ; Verified Objections: Anon. E. Moose #1000171: Violations found diff --git a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/typecast/StaticTypeCastUtil.java b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/typecast/StaticTypeCastUtil.java index 55b174b..eee53e2 100644 --- a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/typecast/StaticTypeCastUtil.java +++ b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/typecast/StaticTypeCastUtil.java @@ -21,6 +21,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.BitSet; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -41,6 +42,7 @@ import org.apache.asterix.om.types.AbstractCollectionType; import org.apache.asterix.om.types.BuiltinType; import org.apache.asterix.om.types.IAType; +import org.apache.asterix.om.utils.ConstantExpressionUtil; import org.apache.asterix.om.utils.NonTaggedFormatUtil; import org.apache.commons.lang3.mutable.Mutable; import org.apache.commons.lang3.mutable.MutableObject; @@ -234,11 +236,11 @@ switch (arg.getExpressionTag()) { case FUNCTION_CALL: ScalarFunctionCallExpression argFunc = (ScalarFunctionCallExpression) arg; - changed = rewriteFuncExpr(argFunc, requiredItemType, currentItemType, env) || changed; + changed |= rewriteFuncExpr(argFunc, requiredItemType, currentItemType, env); changed |= castItem(requiredItemType, currentItemType, argFunc, args.get(j)); break; case VARIABLE: - changed = injectCastToRelaxType(args.get(j), currentItemType, env) || changed; + changed |= injectCastToRelaxType(args.get(j), currentItemType, env); break; } } @@ -278,41 +280,44 @@ || func.getFunctionIdentifier() == BuiltinFunctions.CLOSED_RECORD_CONSTRUCTOR)) { return false; } + + List<Mutable<ILogicalExpression>> arguments = func.getArguments(); + int fieldArgumentCount = arguments.size() / 2; + IAType[] reqFieldTypes = reqType.getFieldTypes(); String[] reqFieldNames = reqType.getFieldNames(); IAType[] inputFieldTypes = inputType.getFieldTypes(); String[] inputFieldNames = inputType.getFieldNames(); int[] fieldPermutation = new int[reqFieldTypes.length]; - boolean[] nullFields = new boolean[reqFieldTypes.length]; - boolean[] openFields = new boolean[inputFieldTypes.length]; + BitSet nullFields = new BitSet(reqFieldTypes.length); + BitSet openFields = new BitSet(fieldArgumentCount); - Arrays.fill(nullFields, false); - Arrays.fill(openFields, true); Arrays.fill(fieldPermutation, -1); + openFields.set(0, fieldArgumentCount); // forward match: match from actual to required - boolean matched = false; for (int i = 0; i < inputFieldNames.length; i++) { String fieldName = inputFieldNames[i]; IAType fieldType = inputFieldTypes[i]; - if (2 * i + 1 > func.getArguments().size()) { - // it is not a record constructor function + int fieldNameArgumentIdx = findFieldNameArgumentIdx(arguments, fieldName); + if (fieldNameArgumentIdx < 0) { return false; } + int fieldValueArgumentIdx = fieldNameArgumentIdx + 1; + int fieldArgumentIdx = fieldNameArgumentIdx / 2; - // 2*i+1 is the index of field value expression - ILogicalExpression arg = func.getArguments().get(2 * i + 1).getValue(); - matched = false; + ILogicalExpression arg = arguments.get(fieldValueArgumentIdx).getValue(); + boolean matched = false; for (int j = 0; j < reqFieldNames.length; j++) { String reqFieldName = reqFieldNames[j]; IAType reqFieldType = reqFieldTypes[j]; if (fieldName.equals(reqFieldName)) { //type matched if (fieldType.equals(reqFieldType)) { - fieldPermutation[j] = i; - openFields[i] = false; + fieldPermutation[j] = fieldNameArgumentIdx; + openFields.clear(fieldArgumentIdx); matched = true; if (arg.getExpressionTag() == LogicalExpressionTag.FUNCTION_CALL) { @@ -327,8 +332,8 @@ IAType itemType = ((AUnionType) reqFieldType).getActualType(); reqFieldType = itemType; if (fieldType.equals(BuiltinType.AMISSING) || fieldType.equals(itemType)) { - fieldPermutation[j] = i; - openFields[i] = false; + fieldPermutation[j] = fieldNameArgumentIdx; + openFields.clear(fieldArgumentIdx); matched = true; // rewrite record expr @@ -345,15 +350,15 @@ if (NonTaggedFormatUtil.isOptional(fieldType)) { IAType itemType = ((AUnionType) fieldType).getActualType(); if (reqFieldType.equals(itemType)) { - fieldPermutation[j] = i; - openFields[i] = false; + fieldPermutation[j] = fieldNameArgumentIdx; + openFields.clear(fieldArgumentIdx); matched = true; ScalarFunctionCallExpression notNullFunc = new ScalarFunctionCallExpression( FunctionUtil.getFunctionInfo(BuiltinFunctions.CHECK_UNKNOWN)); - notNullFunc.getArguments().add(new MutableObject<ILogicalExpression>(arg)); + notNullFunc.getArguments().add(new MutableObject<>(arg)); //wrap the not null function to the original function - func.getArguments().get(2 * i + 1).setValue(notNullFunc); + arguments.get(fieldValueArgumentIdx).setValue(notNullFunc); break; } } @@ -362,8 +367,8 @@ if (arg.getExpressionTag() == LogicalExpressionTag.FUNCTION_CALL) { ScalarFunctionCallExpression scalarFunc = (ScalarFunctionCallExpression) arg; rewriteFuncExpr(scalarFunc, reqFieldType, fieldType, env); - fieldPermutation[j] = i; - openFields[i] = false; + fieldPermutation[j] = fieldNameArgumentIdx; + openFields.clear(fieldArgumentIdx); matched = true; break; } @@ -380,7 +385,7 @@ for (int i = 0; i < reqFieldNames.length; i++) { String reqFieldName = reqFieldNames[i]; IAType reqFieldType = reqFieldTypes[i]; - matched = false; + boolean matched = false; for (int j = 0; j < inputFieldNames.length; j++) { String fieldName = inputFieldNames[j]; IAType fieldType = inputFieldTypes[j]; @@ -388,11 +393,16 @@ continue; } // should check open field here - // because number of entries in fieldPermuations is the + // because number of entries in fieldPermutations is the // number of required schema fields // here we want to check if an input field is matched - // the entry index of fieldPermuatons is req field index - if (!openFields[j]) { + // the entry index of fieldPermutations is req field index + int fieldNameArgumentIdx = findFieldNameArgumentIdx(arguments, fieldName); + if (fieldNameArgumentIdx < 0) { + return false; + } + int fieldArgumentIdx = fieldNameArgumentIdx / 2; + if (!openFields.get(fieldArgumentIdx)) { matched = true; break; } @@ -413,7 +423,7 @@ if (NonTaggedFormatUtil.isOptional(reqFieldType)) { // add a null field - nullFields[i] = true; + nullFields.set(i); } else { // no matched field in the input for a required closed field if (inputType.isOpen()) { @@ -427,36 +437,47 @@ } } - List<Mutable<ILogicalExpression>> arguments = func.getArguments(); - List<Mutable<ILogicalExpression>> originalArguments = new ArrayList<Mutable<ILogicalExpression>>(); - originalArguments.addAll(arguments); - arguments.clear(); + List<Mutable<ILogicalExpression>> newArguments = new ArrayList<>(arguments.size()); + // re-order the closed part and fill in null fields - for (int i = 0; i < fieldPermutation.length; i++) { + for (int i = 0; i < reqFieldTypes.length; i++) { int pos = fieldPermutation[i]; if (pos >= 0) { - arguments.add(originalArguments.get(2 * pos)); - arguments.add(originalArguments.get(2 * pos + 1)); + newArguments.add(arguments.get(pos)); + newArguments.add(arguments.get(pos + 1)); } - if (nullFields[i]) { + if (nullFields.get(i)) { // add a null field - arguments.add(new MutableObject<ILogicalExpression>( + newArguments.add(new MutableObject<>( new ConstantExpression(new AsterixConstantValue(new AString(reqFieldNames[i]))))); - arguments.add(new MutableObject<ILogicalExpression>( - new ConstantExpression(new AsterixConstantValue(ANull.NULL)))); + newArguments.add(new MutableObject<>(new ConstantExpression(new AsterixConstantValue(ANull.NULL)))); } } // add the open part - for (int i = 0; i < openFields.length; i++) { - if (openFields[i]) { - arguments.add(originalArguments.get(2 * i)); - Mutable<ILogicalExpression> expRef = originalArguments.get(2 * i + 1); - injectCastToRelaxType(expRef, inputFieldTypes[i], env); - arguments.add(expRef); + for (int i = openFields.nextSetBit(0); i >= 0; i = openFields.nextSetBit(i + 1)) { + newArguments.add(arguments.get(i * 2)); + Mutable<ILogicalExpression> expRef = arguments.get(i * 2 + 1); + injectCastToRelaxType(expRef, inputFieldTypes[i], env); + newArguments.add(expRef); + } + + arguments.clear(); + arguments.addAll(newArguments); + + return true; + } + + private static int findFieldNameArgumentIdx(List<Mutable<ILogicalExpression>> arguments, String fieldName) { + for (int i = 0, ln = arguments.size(); i < ln; i += 2) { + String n = ConstantExpressionUtil.getStringConstant(arguments.get(i).getValue()); + if (n == null) { + break; + } else if (fieldName.equals(n)) { + return i; } } - return true; + return -1; } private static boolean injectCastToRelaxType(Mutable<ILogicalExpression> expRef, IAType inputFieldType, diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/ObjectsQueries.xml b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/ObjectsQueries.xml index cfd8aeb..b91a5ef 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/ObjectsQueries.xml +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/ObjectsQueries.xml @@ -201,4 +201,9 @@ <output-dir compare="Text">pairs</output-dir> </compilation-unit> </test-case> + <test-case FilePath="objects"> + <compilation-unit name="query-ASTERIXDB-2529"> + <output-dir compare="Text">query-ASTERIXDB-2529</output-dir> + </compilation-unit> + </test-case> </test-group> diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.query.sqlpp new file mode 100644 index 0000000..f8de25b --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.query.sqlpp @@ -0,0 +1,27 @@ +/* + * 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. + */ + +/* + * Description : Test record constructor with MISSING field value + * Expected Res : Success + */ + +FROM [{'a': 9, 'b': missing, 'c': 33}] v +SELECT v; + diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.adm new file mode 100644 index 0000000..0e58316 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/objects/query-ASTERIXDB-2529/query-ASTERIXDB-2529.1.adm @@ -0,0 +1 @@ +{ "v": { "a": 9, "c": 33 } } \ No newline at end of file -- To view, visit https://asterix-gerrit.ics.uci.edu/3269 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5d3435274ebf0007fe7e63b86264337072fd8305 Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Dmitry Lychagin <[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]>
