This is an automated email from the ASF dual-hosted git repository. mblow pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/asterixdb.git
commit bbe9d0344cff8723d162c313ba42c19e72fd159d Author: Wail Alkowaileet <[email protected]> AuthorDate: Tue Jun 6 16:54:21 2023 -0700 [ASTERIXDB-3205][COMP] Avoid inlining non-pure functions in aggregate functions - user model changes: no - storage format changes: no - interface changes: no Details: Currently, the compiler blindly inlines all functions from assign operators into aggregate functions. However, non-pure functions should not be inlined. Change-Id: Ib3fbd684f1732a276d6033bf38507df41d33b843 Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17589 Reviewed-by: Wail Alkowaileet <[email protected]> Reviewed-by: Ali Alsuliman <[email protected]> Integration-Tests: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> --- .../non-pure-function.00.ddl.sqlpp | 30 +++++++++++++++++++++ .../non-pure-function.01.update.sqlpp | 24 +++++++++++++++++ .../non-pure-function.02.query.sqlpp | 26 ++++++++++++++++++ .../non-pure-function.03.query.sqlpp | 27 +++++++++++++++++++ .../non-pure-function.04.query.sqlpp | 28 +++++++++++++++++++ .../non-pure-function.05.query.sqlpp | 27 +++++++++++++++++++ .../non-pure-function.06.query.sqlpp | 28 +++++++++++++++++++ .../non-pure-function/non-pure-function.02.adm | 1 + .../non-pure-function/non-pure-function.03.adm | 1 + .../non-pure-function/non-pure-function.04.adm | 1 + .../non-pure-function/non-pure-function.05.adm | 1 + .../non-pure-function/non-pure-function.06.adm | 1 + .../test/resources/runtimets/testsuite_sqlpp.xml | 5 ++++ .../rules/InlineAssignIntoAggregateRule.java | 31 +++++++++++----------- 14 files changed, 215 insertions(+), 16 deletions(-) diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.00.ddl.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.00.ddl.sqlpp new file mode 100644 index 0000000000..99898f9b28 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.00.ddl.sqlpp @@ -0,0 +1,30 @@ +/* + * 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. + */ + +DROP DATAVERSE test IF EXISTS; +CREATE DATAVERSE test; + +USE test; + +CREATE TYPE OpenType AS { + id: int +}; + +CREATE DATASET MyDataset(OpenType) +PRIMARY KEY id; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.01.update.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.01.update.sqlpp new file mode 100644 index 0000000000..751aaba761 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.01.update.sqlpp @@ -0,0 +1,24 @@ +/* + * 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; + +INSERT INTO MyDataset( + SELECT x id, x num + FROM range(1, 10) x +); diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.02.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.02.query.sqlpp new file mode 100644 index 0000000000..116c61ae47 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.02.query.sqlpp @@ -0,0 +1,26 @@ +/* + * 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 : Do not inline random(1) in COUNT() +* Date : Jun 2nd 2023 +*/ +USE test; +SELECT COUNT(random(1)) randCount +FROM MyDataset md +GROUP BY 1 diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.03.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.03.query.sqlpp new file mode 100644 index 0000000000..1e02ae2426 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.03.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 : Do not inline random(1) in COUNT() +* Date : Jun 2nd 2023 +*/ +USE test; + +SELECT COUNT(random(1)) randCount, COUNT(md.num + 1) numCount +FROM MyDataset md +GROUP BY 1 \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.04.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.04.query.sqlpp new file mode 100644 index 0000000000..c5837237f4 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.04.query.sqlpp @@ -0,0 +1,28 @@ +/* + * 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 : Do not inline random(1) in COUNT() +* Date : Jun 2nd 2023 +* Notes : We can inline COUNT(md.num + 1) if ASTERIXDB-3206 is fixed +*/ +USE test; + +SELECT COUNT(md.num + 1) numCount, COUNT(random(1)) randCount +FROM MyDataset md +GROUP BY 1 \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.05.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.05.query.sqlpp new file mode 100644 index 0000000000..715b73f92d --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.05.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 : Do not inline random(1) in COUNT() +* Date : Jun 2nd 2023 +*/ +USE test; + +SELECT COUNT(random(1) + round(md.num / 3)) randCount, COUNT(md.num + 1) numCount +FROM MyDataset md +GROUP BY 1 \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.06.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.06.query.sqlpp new file mode 100644 index 0000000000..20e2c785a7 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/non-pure-function/non-pure-function.06.query.sqlpp @@ -0,0 +1,28 @@ +/* + * 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 : Do not inline random(1) in COUNT() +* Date : Jun 2nd 2023 +* Notes : We can inline COUNT(md.num + 1) if ASTERIXDB-3206 is fixed +*/ +USE test; + +SELECT COUNT(md.num + 1) numCount, COUNT(random(1) + round(md.num / 3)) randCount +FROM MyDataset md +GROUP BY 1 \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.02.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.02.adm new file mode 100644 index 0000000000..6390f8a0f6 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.02.adm @@ -0,0 +1 @@ +{ "randCount": 10 } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.03.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.03.adm new file mode 100644 index 0000000000..160683ea33 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.03.adm @@ -0,0 +1 @@ +{ "randCount": 10, "numCount": 10 } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.04.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.04.adm new file mode 100644 index 0000000000..a3724b3662 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.04.adm @@ -0,0 +1 @@ +{ "numCount": 10, "randCount": 10 } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.05.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.05.adm new file mode 100644 index 0000000000..160683ea33 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.05.adm @@ -0,0 +1 @@ +{ "randCount": 10, "numCount": 10 } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.06.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.06.adm new file mode 100644 index 0000000000..a3724b3662 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/non-pure-function/non-pure-function.06.adm @@ -0,0 +1 @@ +{ "numCount": 10, "randCount": 10 } 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 c0784c1dce..41c0424389 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml +++ b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml @@ -1612,6 +1612,11 @@ <output-dir compare="Text">min_max_arrays</output-dir> </compilation-unit> </test-case> + <test-case FilePath="aggregate"> + <compilation-unit name="non-pure-function"> + <output-dir compare="Text">non-pure-function</output-dir> + </compilation-unit> + </test-case> </test-group> <test-group name="aggregate-sql"> <test-case FilePath="aggregate-sql"> diff --git a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java index d966ed100d..ad7703db8a 100644 --- a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java +++ b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java @@ -84,11 +84,7 @@ public class InlineAssignIntoAggregateRule implements IAlgebraicRewriteRule { } AssignOperator assignOp = (AssignOperator) op2; VarExprSubstitution ves = new VarExprSubstitution(assignOp.getVariables(), assignOp.getExpressions()); - inlineVariables(aggOp, ves); - List<Mutable<ILogicalOperator>> op1InpList = aggOp.getInputs(); - op1InpList.clear(); - op1InpList.add(op2.getInputs().get(0)); - return true; + return inlineVariables(aggOp, ves); } private boolean inlineOuterInputAssignIntoAgg(AggregateOperator aggOp, @@ -107,18 +103,20 @@ public class InlineAssignIntoAggregateRule implements IAlgebraicRewriteRule { for (Mutable<ILogicalExpression> exprRef : aggOp.getExpressions()) { ILogicalExpression expr = exprRef.getValue(); Pair<Boolean, ILogicalExpression> p = expr.accept(ves, null); - if (p.first) { - exprRef.setValue(p.second); + ILogicalExpression originalExpr = p.second; + if (p.first & originalExpr.isFunctional()) { + exprRef.setValue(originalExpr); inlined = true; } } return inlined; } - private class VarExprSubstitution extends AbstractConstVarFunVisitor<Pair<Boolean, ILogicalExpression>, Void> { + private static class VarExprSubstitution + extends AbstractConstVarFunVisitor<Pair<Boolean, ILogicalExpression>, Void> { - private List<LogicalVariable> variables; - private List<Mutable<ILogicalExpression>> expressions; + private final List<LogicalVariable> variables; + private final List<Mutable<ILogicalExpression>> expressions; public VarExprSubstitution(List<LogicalVariable> variables, List<Mutable<ILogicalExpression>> expressions) { this.variables = variables; @@ -127,7 +125,7 @@ public class InlineAssignIntoAggregateRule implements IAlgebraicRewriteRule { @Override public Pair<Boolean, ILogicalExpression> visitConstantExpression(ConstantExpression expr, Void arg) { - return new Pair<Boolean, ILogicalExpression>(false, expr); + return new Pair<>(false, expr); } @Override @@ -137,12 +135,13 @@ public class InlineAssignIntoAggregateRule implements IAlgebraicRewriteRule { for (Mutable<ILogicalExpression> eRef : expr.getArguments()) { ILogicalExpression e = eRef.getValue(); Pair<Boolean, ILogicalExpression> p = e.accept(this, arg); - if (p.first) { - eRef.setValue(p.second.cloneExpression()); + ILogicalExpression originalExpr = p.second; + if (p.first & originalExpr.isFunctional()) { + eRef.setValue(originalExpr.cloneExpression()); changed = true; } } - return new Pair<Boolean, ILogicalExpression>(changed, expr); + return new Pair<>(changed, expr); } @Override @@ -151,9 +150,9 @@ public class InlineAssignIntoAggregateRule implements IAlgebraicRewriteRule { LogicalVariable v = expr.getVariableReference(); int idx = variables.indexOf(v); if (idx < 0) { - return new Pair<Boolean, ILogicalExpression>(false, expr); + return new Pair<>(false, expr); } else { - return new Pair<Boolean, ILogicalExpression>(true, expressions.get(idx).getValue()); + return new Pair<>(true, expressions.get(idx).getValue()); } }
