alex-plekhanov commented on code in PR #12595:
URL: https://github.com/apache/ignite/pull/12595#discussion_r2640215107
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/DistributedCalciteConfiguration.java:
##########
@@ -17,14 +17,70 @@
package org.apache.ignite.internal.processors.query.calcite;
+import java.util.stream.Stream;
import org.apache.ignite.IgniteLogger;
import org.apache.ignite.internal.GridKernalContext;
+import
org.apache.ignite.internal.processors.configuration.distributed.DistributedChangeableProperty;
+import
org.apache.ignite.internal.processors.configuration.distributed.DistributedPropertyDispatcher;
+import
org.apache.ignite.internal.processors.configuration.distributed.SimpleDistributedProperty;
import org.apache.ignite.internal.processors.query.DistributedSqlConfiguration;
+import static
org.apache.ignite.internal.cluster.DistributedConfigurationUtils.setDefaultValue;
+
/** Distributed Calcite-engine configuration. */
public class DistributedCalciteConfiguration extends
DistributedSqlConfiguration {
+ /** Globally disabled rules property name. */
+ public static final String DISABLED_RULES_PROPERTY_NAME =
"sql.calcite.disabledRules";
+
+ /** Default value of the disabled rules. */
+ public static final String[] DFLT_DISABLED_RULES = new String[0];
+
+ /** Query timeout. */
+ private volatile DistributedChangeableProperty<String[]> disabledRules;
+
/** */
public DistributedCalciteConfiguration(GridKernalContext ctx, IgniteLogger
log) {
super(ctx, log);
}
+
+ /**
+ * @return Globally disabled planning rules.
+ * @see #DISABLED_RULES_PROPERTY_NAME
+ */
+ public String[] disabledRules() {
+ DistributedChangeableProperty<String[]> disabledRules =
this.disabledRules;
+
+ String[] res = disabledRules == null ? DFLT_DISABLED_RULES :
disabledRules.get();
+
+ return res != null ? res : DFLT_DISABLED_RULES;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void onReadyToRegister(DistributedPropertyDispatcher
dispatcher) {
+ super.onReadyToRegister(dispatcher);
+
+ registerProperty(
+ dispatcher,
+ DISABLED_RULES_PROPERTY_NAME,
+ prop -> disabledRules = prop,
+ () -> new SimpleDistributedProperty<>(
+ DISABLED_RULES_PROPERTY_NAME,
+ str -> Stream.of(str.split(",")).map(String::trim).filter(s ->
!s.isBlank()).toArray(String[]::new),
+ "Comma-separated list of Calcite's disabled planning rules.
NOTE: cleans the planning cache!"
Review Comment:
cleans the planning cache on change
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessor.java:
##########
@@ -315,6 +319,28 @@ public CalciteQueryProcessor(GridKernalContext ctx) {
}
distrCfg = new DistributedCalciteConfiguration(ctx, log);
+
+ // A listener to clean the plans cache if a rule was disabled.
+
ctx.internalSubscriptionProcessor().registerDistributedConfigurationListener(new
DistributedConfigurationLifecycleListener() {
+ @Override public void
onReadyToRegister(DistributedPropertyDispatcher dispatcher) {
+ // No-op.
+ }
+
+ @Override public void onReadyToWrite() {
+ assert distrCfg.disabledRulesProperty() != null;
+
+ distrCfg.disabledRulesProperty().addListener(new
DistributePropertyListener<>() {
+ @Override public void onUpdate(String name, String[]
oldVal, String[] newVal) {
+ if (oldVal != null && F.compareArrays(oldVal, newVal)
!= 0) {
+ log.warning("Cleaning Calcite's cache plan by
setting changing of the property '"
+ + distrCfg.disabledRulesProperty().getName() +
"'.");
+
+ qryPlanCache.clear();
+ }
+ }
+ });
Review Comment:
I think it worth to make this inside `DistributedCalciteConfiguration`
class, to not overload `CalciteQueryProcessor`.
Perhaps we can also make `DistributedCalciteConfiguration` implements
`LifecycleAware` and store query processor in `onStart` method (see
ExchangeServiceImpl/ExecutionServiceImpl/etc as example).
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/DistributedCalciteConfiguration.java:
##########
@@ -17,14 +17,70 @@
package org.apache.ignite.internal.processors.query.calcite;
+import java.util.stream.Stream;
import org.apache.ignite.IgniteLogger;
import org.apache.ignite.internal.GridKernalContext;
+import
org.apache.ignite.internal.processors.configuration.distributed.DistributedChangeableProperty;
+import
org.apache.ignite.internal.processors.configuration.distributed.DistributedPropertyDispatcher;
+import
org.apache.ignite.internal.processors.configuration.distributed.SimpleDistributedProperty;
import org.apache.ignite.internal.processors.query.DistributedSqlConfiguration;
+import static
org.apache.ignite.internal.cluster.DistributedConfigurationUtils.setDefaultValue;
+
/** Distributed Calcite-engine configuration. */
public class DistributedCalciteConfiguration extends
DistributedSqlConfiguration {
+ /** Globally disabled rules property name. */
+ public static final String DISABLED_RULES_PROPERTY_NAME =
"sql.calcite.disabledRules";
+
+ /** Default value of the disabled rules. */
+ public static final String[] DFLT_DISABLED_RULES = new String[0];
+
+ /** Query timeout. */
Review Comment:
Wrong comment
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessorTest.java:
##########
@@ -1167,6 +1172,154 @@ private static List<String>
deriveColumnNamesFromCursor(FieldsQueryCursor cursor
return names;
}
+ /** */
+ @Test
+ public void testDisableRuleDistributedPropertyCommand() throws Exception {
+ String propName =
DistributedCalciteConfiguration.DISABLED_RULES_PROPERTY_NAME;
+
+ for (Ignite ig : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop =
((IgniteEx)ig).context().distributedConfiguration().property(propName);
+
+ assertNotNull(prop);
+ assertNotNull(prop.get());
+ assertTrue(prop.get().length == 0);
+ }
+
+ DynamicMBean mbean = getMxBean(
+ client.context().igniteInstanceName(),
+ "management",
+ Collections.singletonList("Property"),
+ "Set",
+ DynamicMBean.class
+ );
+
+ // Check simple value setting.
+ mbean.invoke(
+ CommandMBean.INVOKE,
+ new Object[] {propName, "ExposeIndexRule"},
+ new String[] {String.class.getName(), String.class.getName()}
+ );
Review Comment:
Move this call to the method?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessorTest.java:
##########
@@ -1167,6 +1172,154 @@ private static List<String>
deriveColumnNamesFromCursor(FieldsQueryCursor cursor
return names;
}
+ /** */
+ @Test
+ public void testDisableRuleDistributedPropertyCommand() throws Exception {
+ String propName =
DistributedCalciteConfiguration.DISABLED_RULES_PROPERTY_NAME;
+
+ for (Ignite ig : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop =
((IgniteEx)ig).context().distributedConfiguration().property(propName);
Review Comment:
Move this call to the method? Maybe together with .get()?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessorTest.java:
##########
@@ -1167,6 +1172,154 @@ private static List<String>
deriveColumnNamesFromCursor(FieldsQueryCursor cursor
return names;
}
+ /** */
+ @Test
+ public void testDisableRuleDistributedPropertyCommand() throws Exception {
Review Comment:
CalciteQueryProcessorTest is already to large, let's move these tests to the
new dedicated class
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessor.java:
##########
@@ -756,6 +782,11 @@ private <T> T processQuery(
fldsQry != null ? fldsQry.getQueryInitiatorId() : null
);
+ String[] disbledRules = distrCfg.disabledRules();
+
+ if (!F.isEmpty(disbledRules))
+ qry.planningContext().addRulesFilter(new
IgnitePlanner.DisabledRuleFilter(disbledRules));
Review Comment:
This code will be executed on every query, even if planning is not required.
Redundant `planningContext` will be created. Actually we need this only before
planning (before or inside `prepareSingle` call)
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessorTest.java:
##########
@@ -1167,6 +1172,154 @@ private static List<String>
deriveColumnNamesFromCursor(FieldsQueryCursor cursor
return names;
}
+ /** */
+ @Test
+ public void testDisableRuleDistributedPropertyCommand() throws Exception {
+ String propName =
DistributedCalciteConfiguration.DISABLED_RULES_PROPERTY_NAME;
+
+ for (Ignite ig : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop =
((IgniteEx)ig).context().distributedConfiguration().property(propName);
+
+ assertNotNull(prop);
+ assertNotNull(prop.get());
+ assertTrue(prop.get().length == 0);
+ }
+
+ DynamicMBean mbean = getMxBean(
+ client.context().igniteInstanceName(),
+ "management",
+ Collections.singletonList("Property"),
+ "Set",
+ DynamicMBean.class
+ );
+
+ // Check simple value setting.
+ mbean.invoke(
+ CommandMBean.INVOKE,
+ new Object[] {propName, "ExposeIndexRule"},
+ new String[] {String.class.getName(), String.class.getName()}
+ );
+
+ assertTrue(waitForCondition(
+ () -> {
+ String[] expectedVal = new String[] {"ExposeIndexRule"};
+
+ for (Ignite g : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop0 =
((IgniteEx)g).context().distributedConfiguration().property(propName);
+
+ if (F.compareArrays(expectedVal, prop0.get()) != 0)
+ return false;
+ }
+
+ return true;
+ },
+ getTestTimeout()
+ ));
+
+ // Check setting with spaces.
+ mbean.invoke(
+ CommandMBean.INVOKE,
+ new Object[] {propName, ",, ExposeIndexRule , ,\t,
CorrelatedNestedLoopJoinRule "},
+ new String[] {String.class.getName(), String.class.getName()}
+ );
+
+ assertTrue(waitForCondition(
+ () -> {
+ String[] expectedVal = new String[] {"ExposeIndexRule",
"CorrelatedNestedLoopJoinRule"};
+
+ for (Ignite g : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop0 =
((IgniteEx)g).context().distributedConfiguration().property(propName);
+
+ if (F.compareArrays(expectedVal, prop0.get()) != 0)
+ return false;
+ }
+
+ return true;
+ },
+ getTestTimeout()
+ ));
+ }
+
+ /**
+ * Tests the ability to disable planning rules globally.
+ *
+ * @see DistributedCalciteConfiguration#disabledRules()}.
+ */
+ @Test
+ public void testDisableRuleDistributedProperty() throws Exception {
+ sql( "drop table if exists test_tbl", true);
+ sql( "create table test_tbl (c1 int, c2 int, c3 int)", true);
+ sql( "create index idx2 on test_tbl(c2)", true);
+ sql( "create index idx3 on test_tbl(c3)", true);
+
+ assertQuery(client, "select * from test_tbl order by c2")
+ .matches(containsIndexScan("PUBLIC", "TEST_TBL", "IDX2"))
+ .matches(not(containsSubPlan("IgniteSort")))
+ .check();
+ assertQuery(client, "select * from test_tbl order by c3")
+ .matches(containsIndexScan("PUBLIC", "TEST_TBL", "IDX3"))
+ .matches(not(containsSubPlan("IgniteSort")))
+ .check();
+
+ String propName =
DistributedCalciteConfiguration.DISABLED_RULES_PROPERTY_NAME;
+
+ DistributedChangeableProperty<String[]> prop =
client.context().distributedConfiguration().property(propName);
+
+ // Disable index scanning at all.
+ assertTrue(prop.propagate(new
String[]{ExposeIndexRule.INSTANCE.toString()}));
+
+ assertTrue(waitForCondition(
+ () -> {
+ for (Ignite g : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop0 =
((IgniteEx)g).context().distributedConfiguration().property(propName);
+
+ if (F.isEmpty(prop0.get()))
+ return false;
+ }
+
+ return true;
+ },
+ getTestTimeout()
+ ));
+
+ // Check that there are no index scans now.
+ assertQuery(client, "select * from test_tbl order by c2")
+ .matches(not(containsIndexScan("PUBLIC", "TEST_TBL", "IDX2")))
+ .matches(containsSubPlan("IgniteSort"))
+ .check();
+ assertQuery(client, "select * from test_tbl order by c3")
+ .matches(not(containsIndexScan("PUBLIC", "TEST_TBL", "IDX3")))
+ .matches(containsSubPlan("IgniteSort"))
+ .check();
+
+ // Swith on the index scanning back.
+ prop.propagate(new String[0]);
+
+ assertTrue(waitForCondition(
+ () -> {
+ for (Ignite g : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop0 =
((IgniteEx)g).context().distributedConfiguration().property(propName);
+
+ if (!F.isEmpty(prop0.get()))
+ return false;
+ }
+
+ return true;
+ },
+ getTestTimeout()
+ ));
+
+ // Check that the index are available again.
+ assertQuery(client, "select * from test_tbl order by c2")
+ .matches(containsIndexScan("PUBLIC", "TEST_TBL", "IDX2"))
+ .matches(not(containsSubPlan("IgniteSort")))
+ .check();
+ assertQuery(client, "select * from test_tbl order by c3")
+ .matches(containsIndexScan("PUBLIC", "TEST_TBL", "IDX3"))
+ .matches(not(containsSubPlan("IgniteSort")))
+ .check();
+ }
Review Comment:
Can we test global + local (by hints) rules disable?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessorTest.java:
##########
@@ -1167,6 +1172,154 @@ private static List<String>
deriveColumnNamesFromCursor(FieldsQueryCursor cursor
return names;
}
+ /** */
+ @Test
+ public void testDisableRuleDistributedPropertyCommand() throws Exception {
+ String propName =
DistributedCalciteConfiguration.DISABLED_RULES_PROPERTY_NAME;
+
+ for (Ignite ig : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop =
((IgniteEx)ig).context().distributedConfiguration().property(propName);
+
+ assertNotNull(prop);
+ assertNotNull(prop.get());
+ assertTrue(prop.get().length == 0);
+ }
+
+ DynamicMBean mbean = getMxBean(
+ client.context().igniteInstanceName(),
+ "management",
+ Collections.singletonList("Property"),
+ "Set",
+ DynamicMBean.class
+ );
+
+ // Check simple value setting.
+ mbean.invoke(
+ CommandMBean.INVOKE,
+ new Object[] {propName, "ExposeIndexRule"},
+ new String[] {String.class.getName(), String.class.getName()}
+ );
+
+ assertTrue(waitForCondition(
+ () -> {
+ String[] expectedVal = new String[] {"ExposeIndexRule"};
+
+ for (Ignite g : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop0 =
((IgniteEx)g).context().distributedConfiguration().property(propName);
+
+ if (F.compareArrays(expectedVal, prop0.get()) != 0)
+ return false;
+ }
+
+ return true;
+ },
+ getTestTimeout()
+ ));
+
+ // Check setting with spaces.
+ mbean.invoke(
+ CommandMBean.INVOKE,
+ new Object[] {propName, ",, ExposeIndexRule , ,\t,
CorrelatedNestedLoopJoinRule "},
+ new String[] {String.class.getName(), String.class.getName()}
+ );
+
+ assertTrue(waitForCondition(
+ () -> {
+ String[] expectedVal = new String[] {"ExposeIndexRule",
"CorrelatedNestedLoopJoinRule"};
+
+ for (Ignite g : G.allGrids()) {
+ DistributedChangeableProperty<String[]> prop0 =
((IgniteEx)g).context().distributedConfiguration().property(propName);
+
+ if (F.compareArrays(expectedVal, prop0.get()) != 0)
+ return false;
+ }
+
+ return true;
+ },
+ getTestTimeout()
+ ));
+ }
+
+ /**
+ * Tests the ability to disable planning rules globally.
+ *
+ * @see DistributedCalciteConfiguration#disabledRules()}.
+ */
+ @Test
+ public void testDisableRuleDistributedProperty() throws Exception {
+ sql( "drop table if exists test_tbl", true);
+ sql( "create table test_tbl (c1 int, c2 int, c3 int)", true);
+ sql( "create index idx2 on test_tbl(c2)", true);
+ sql( "create index idx3 on test_tbl(c3)", true);
+
+ assertQuery(client, "select * from test_tbl order by c2")
+ .matches(containsIndexScan("PUBLIC", "TEST_TBL", "IDX2"))
+ .matches(not(containsSubPlan("IgniteSort")))
+ .check();
+ assertQuery(client, "select * from test_tbl order by c3")
+ .matches(containsIndexScan("PUBLIC", "TEST_TBL", "IDX3"))
+ .matches(not(containsSubPlan("IgniteSort")))
+ .check();
+
+ String propName =
DistributedCalciteConfiguration.DISABLED_RULES_PROPERTY_NAME;
+
+ DistributedChangeableProperty<String[]> prop =
client.context().distributedConfiguration().property(propName);
+
+ // Disable index scanning at all.
+ assertTrue(prop.propagate(new
String[]{ExposeIndexRule.INSTANCE.toString()}));
Review Comment:
Maybe use mbean instead of propagate if new methods for mbean call will be
added? (up to you)
--
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]
