mihaibudiu commented on code in PR #4587: URL: https://github.com/apache/calcite/pull/4587#discussion_r2442486491
########## core/src/test/resources/sql/hep.iq: ########## @@ -0,0 +1,108 @@ +# hep.iq - hep tests can customizable optimization rules for hep planner +# +# 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. +# + +# This documents the required format for .iq files containing hep planner rule configurations +# +# CORRECT SYNTAX: Review Comment: I think that "CORRECT" is redundant (unless you also plan to discuss incorrect versions) ########## testkit/src/main/java/org/apache/calcite/test/QuidemTest.java: ########## @@ -261,6 +266,39 @@ protected void checkRun(String path) throws Exception { })); } } + + if (propertyName.equals("hep-rules")) { + closer.add( + Hook.PROGRAM.addThread((Consumer<Holder<Program>>) + holder -> { + List<RelOptRule> hepRulesAdd = new ArrayList<>(); + List<RelOptRule> hepRulesRemove = new ArrayList<>(); Review Comment: this does not seem to be used anywhere ########## core/src/test/resources/sql/hep.iq: ########## @@ -0,0 +1,108 @@ +# hep.iq - hep tests can customizable optimization rules for hep planner +# +# 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. +# + +# This documents the required format for .iq files containing hep planner rule configurations +# +# CORRECT SYNTAX: +# --------------- +# !set hep-rules " +# +CoreRules.RULE_NAME1, +# -CoreRules.RULE_NAME2, +# +EnumerableRules.ENUMERABLE_RULE_NAME" +# +# FORMAT RULES: +# ------------- +# 1. Must begin with: !set hep-rules " (double quote) +# 2. Each rule must be on its own line. All lines except last must end with comma. +# Final rule line must close with " (double quote) on final line Review Comment: "final" is repeated ########## core/src/test/resources/sql/hep.iq: ########## @@ -0,0 +1,108 @@ +# hep.iq - hep tests can customizable optimization rules for hep planner +# +# 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. +# + +# This documents the required format for .iq files containing hep planner rule configurations +# +# CORRECT SYNTAX: +# --------------- +# !set hep-rules " +# +CoreRules.RULE_NAME1, +# -CoreRules.RULE_NAME2, +# +EnumerableRules.ENUMERABLE_RULE_NAME" +# +# FORMAT RULES: +# ------------- +# 1. Must begin with: !set hep-rules " (double quote) +# 2. Each rule must be on its own line. All lines except last must end with comma. +# Final rule line must close with " (double quote) on final line +# 3. Rules must start with - (remove) or + (add) +# 4. Rule prefix can be: +# - CoreRules.RULE_NAME -> added/removed from hep program +# - EnumerableRules.RULE_NAME -> added/removed from volcano program +# - RULE_NAME (no prefix) -> treated as CoreRules, added/removed from hep program +# 5. Duplicate operations on the same rule: last operation wins +# Example: "+Rule1,+Rule1,-Rule1" results in rule being removed Review Comment: I suspect it's a noop to remove a rule which does not exist ########## core/src/test/resources/sql/hep.iq: ########## @@ -0,0 +1,108 @@ +# hep.iq - hep tests can customizable optimization rules for hep planner +# +# 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. +# + +# This documents the required format for .iq files containing hep planner rule configurations +# +# CORRECT SYNTAX: +# --------------- +# !set hep-rules " +# +CoreRules.RULE_NAME1, +# -CoreRules.RULE_NAME2, +# +EnumerableRules.ENUMERABLE_RULE_NAME" +# +# FORMAT RULES: +# ------------- +# 1. Must begin with: !set hep-rules " (double quote) +# 2. Each rule must be on its own line. All lines except last must end with comma. +# Final rule line must close with " (double quote) on final line +# 3. Rules must start with - (remove) or + (add) +# 4. Rule prefix can be: +# - CoreRules.RULE_NAME -> added/removed from hep program +# - EnumerableRules.RULE_NAME -> added/removed from volcano program Review Comment: so there is both a volcano and a hep program? you have to describe the order in which the two optimizers are executed. ########## testkit/src/main/java/org/apache/calcite/test/QuidemTest.java: ########## @@ -261,6 +266,39 @@ protected void checkRun(String path) throws Exception { })); } } + + if (propertyName.equals("hep-rules")) { + closer.add( + Hook.PROGRAM.addThread((Consumer<Holder<Program>>) + holder -> { + List<RelOptRule> hepRulesAdd = new ArrayList<>(); + List<RelOptRule> hepRulesRemove = new ArrayList<>(); + List<RelOptRule> volcanoRulesAdd = new ArrayList<>(); + List<RelOptRule> volcanoRulesRemove = new ArrayList<>(); + parseRulesForProgram((String) value, hepRulesAdd, hepRulesRemove, + volcanoRulesAdd, volcanoRulesRemove); + + // Build hep rules: start empty, add as specified + Program hepProgram = Review Comment: the semantics of the optimization programs you are building does not seem to match the documented semantics. I understood from the documented semantics that rules +/- are applied in the order they are written. Here you remove first and apply next. The result is not the same. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
