Dmitry Lychagin has submitted this change and it was merged. Change subject: [ASTERIXDB-2428][COMP] Incorrect result with limit if offset is negative ......................................................................
[ASTERIXDB-2428][COMP] Incorrect result with limit if offset is negative - user model changes: no - storage format changes: no - interface changes: no Details: - Guarantee non negative limit and offset value during plan generation, so it is correct to add them in CopyLimitDownRule Change-Id: I2238cc5d8f48e14aa2b74d120248a4848dd35691 Reviewed-on: https://asterix-gerrit.ics.uci.edu/2821 Sonar-Qube: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Contrib: Jenkins <[email protected]> Reviewed-by: Taewoo Kim <[email protected]> --- M asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.1.ddl.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.2.update.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.3.query.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.4.query.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.3.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.4.adm M asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml 8 files changed, 203 insertions(+), 7 deletions(-) Approvals: Anon. E. Moose #1000171: Taewoo Kim: Looks good to me, approved Jenkins: Verified; No violations found; ; Verified 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 5410a94..aa4fb75 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 @@ -84,8 +84,10 @@ import org.apache.asterix.metadata.functions.ExternalFunctionCompilerUtil; import org.apache.asterix.metadata.utils.DatasetUtil; import org.apache.asterix.om.base.ABoolean; +import org.apache.asterix.om.base.AInt32; import org.apache.asterix.om.base.AInt64; import org.apache.asterix.om.base.AString; +import org.apache.asterix.om.base.IAObject; import org.apache.asterix.om.constants.AsterixConstantValue; import org.apache.asterix.om.functions.BuiltinFunctions; import org.apache.asterix.om.functions.FunctionInfo; @@ -1351,16 +1353,13 @@ LimitOperator opLim; Pair<ILogicalExpression, Mutable<ILogicalOperator>> p1 = langExprToAlgExpression(lc.getLimitExpr(), tupSource); - AbstractFunctionCallExpression maxObjectsExpr = - createFunctionCallExpression(BuiltinFunctions.TREAT_AS_INTEGER, lc.getLimitExpr().getSourceLocation()); - maxObjectsExpr.getArguments().add(new MutableObject<>(p1.first)); - + ILogicalExpression maxObjectsExpr = + createLimitOffsetValueExpression(p1.first, lc.getLimitExpr().getSourceLocation()); Expression offset = lc.getOffset(); if (offset != null) { Pair<ILogicalExpression, Mutable<ILogicalOperator>> p2 = langExprToAlgExpression(offset, p1.second); - AbstractFunctionCallExpression offsetExpr = - createFunctionCallExpression(BuiltinFunctions.TREAT_AS_INTEGER, lc.getOffset().getSourceLocation()); - offsetExpr.getArguments().add(new MutableObject<>(p2.first)); + ILogicalExpression offsetExpr = + createLimitOffsetValueExpression(p2.first, lc.getOffset().getSourceLocation()); opLim = new LimitOperator(maxObjectsExpr, offsetExpr); opLim.getInputs().add(p2.second); opLim.setSourceLocation(sourceLoc); @@ -1370,6 +1369,37 @@ opLim.setSourceLocation(sourceLoc); } return new Pair<>(opLim, null); + } + + private ILogicalExpression createLimitOffsetValueExpression(ILogicalExpression inputExpr, SourceLocation sourceLoc) + throws CompilationException { + // generates expression for limit and offset value: + // + // switch-case(treat-as-integer(user_value_expr) > 0, true, treat-as-integer(user_value_expr), 0) + // + // this guarantees that the value is always an integer and greater or equals to 0, + // so CopyLimitDownRule works correctly when computing the total limit, + // and other rules which assume integer type + + AInt32 zero = new AInt32(0); + + AbstractFunctionCallExpression valueExpr = + createFunctionCallExpression(BuiltinFunctions.TREAT_AS_INTEGER, sourceLoc); + valueExpr.getArguments().add(new MutableObject<>(inputExpr)); + + AbstractFunctionCallExpression cmpExpr = + createFunctionCallExpressionForBuiltinOperator(OperatorType.GT, sourceLoc); + cmpExpr.getArguments().add(new MutableObject<>(valueExpr)); + cmpExpr.getArguments().add(new MutableObject<>(createConstantExpression(zero, sourceLoc))); + + AbstractFunctionCallExpression switchExpr = + createFunctionCallExpression(BuiltinFunctions.SWITCH_CASE, sourceLoc); + switchExpr.getArguments().add(new MutableObject<>(cmpExpr)); + switchExpr.getArguments().add(new MutableObject<>(createConstantExpression(ABoolean.TRUE, sourceLoc))); + switchExpr.getArguments().add(new MutableObject<>(valueExpr.cloneExpression())); + switchExpr.getArguments().add(new MutableObject<>(createConstantExpression(zero, sourceLoc))); + + return switchExpr; } private static AbstractFunctionCallExpression createFunctionCallExpressionForBuiltinOperator(OperatorType t, @@ -1878,4 +1908,10 @@ } return new Pair<>(topUnionAllOp, topUnionVar); } + + private ConstantExpression createConstantExpression(IAObject value, SourceLocation sourceLoc) { + ConstantExpression constExpr = new ConstantExpression(new AsterixConstantValue(value)); + constExpr.setSourceLocation(sourceLoc); + return constExpr; + } } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.1.ddl.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.1.ddl.sqlpp new file mode 100644 index 0000000..33a9c58 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.1.ddl.sqlpp @@ -0,0 +1,39 @@ +/* + * 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 push down limit into the primary index scan operator + * Expected Result : Success + */ + +drop dataverse test if exists; +create dataverse test; + +use test; + +create type test.DBLPType as +{ + id : bigint, + dblpid : string, + title : string, + authors : string, + misc : string +}; + +create dataset DBLP1(DBLPType) primary key id; + diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.2.update.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.2.update.sqlpp new file mode 100644 index 0000000..6be85ee --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.2.update.sqlpp @@ -0,0 +1,21 @@ +/* + * 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. + */ +use test; + +load dataset DBLP1 using localfs ((`path`=`asterix_nc1://data/dblp-small/dblp-small-id.txt`),(`format`=`delimited-text`),(`delimiter`=`:`)); diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.3.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.3.query.sqlpp new file mode 100644 index 0000000..194ad19 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.3.query.sqlpp @@ -0,0 +1,67 @@ +/* + * 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 negative limit and offset values + * Expected Result : Success + */ + +{ + "t1": array_count(( + select value t + from [6,5,4,3,2,1] t + order by t + limit -2 + )), + + "t2": array_count(( + select value t + from [6,5,4,3,2,1] t + order by t + limit -get_year(current_date()) + )), + + "t3": ( + select value t + from [6,5,4,3,2,1] t + order by t + limit 2 offset -4 + ), + + "t4": ( + select value t + from [6,5,4,3,2,1] t + order by t + limit 2 offset -get_year(current_date()) + ), + + "t5": array_count(( + select value t + from [6,5,4,3,2,1] t + order by t + limit -2 offset -2 + )), + + "t6": array_count(( + select value t + from [6,5,4,3,2,1] t + order by t + limit -get_year(current_date()) offset -get_year(current_datetime()) + )) +} diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.4.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.4.query.sqlpp new file mode 100644 index 0000000..df89aca --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/limit/limit_negative_value/limit_negative_value.4.query.sqlpp @@ -0,0 +1,25 @@ +/* + * 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. + */ + +use test; + +select value paper +from DBLP1 as paper +order by dblpid +limit 2 offset -get_year(current_date()); diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.3.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.3.adm new file mode 100644 index 0000000..af2cdfd --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.3.adm @@ -0,0 +1 @@ +{ "t1": 0, "t2": 0, "t3": [ 1, 2 ], "t4": [ 1, 2 ], "t5": 0, "t6": 0 } \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.4.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.4.adm new file mode 100644 index 0000000..6e7b664 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/limit_negative_value/limit_negative_value.4.adm @@ -0,0 +1,2 @@ +{ "id": 34, "dblpid": "books/acm/Kim95", "title": "Modern Database Systems The Object Model, Interoperability, and Beyond.", "authors": "", "misc": "2004-03-08 Won Kim Modern Database Systems ACM Press and Addison-Wesley 1995 0-201-59098-0 db/books/collections/kim95.html" } +{ "id": 1, "dblpid": "books/acm/kim95/AnnevelinkACFHK95", "title": "Object SQL - A Language for the Design and Implementation of Object Databases.", "authors": "Jurgen Annevelink Rafiul Ahad Amelia Carlson Daniel H. Fishman Michael L. Heytens William Kent", "misc": "2002-01-03 42-68 1995 Modern Database Systems db/books/collections/kim95.html#AnnevelinkACFHK95" } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml index 49bed4b..ada210e 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml +++ b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml @@ -10161,6 +10161,11 @@ </test-group> <test-group name="limit"> <test-case FilePath="limit"> + <compilation-unit name="limit_negative_value"> + <output-dir compare="Text">limit_negative_value</output-dir> + </compilation-unit> + </test-case> + <test-case FilePath="limit"> <compilation-unit name="limit_type_01"> <output-dir compare="Text">limit_type_01</output-dir> </compilation-unit> -- To view, visit https://asterix-gerrit.ics.uci.edu/2821 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: merged Gerrit-Change-Id: I2238cc5d8f48e14aa2b74d120248a4848dd35691 Gerrit-PatchSet: 3 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]>
