[CALCITE-7427] Add connection config to enable RuleMatchVisualizer #4602

tjbanghart · 2025-10-25T20:42:04Z

CALCITE-7247

sonarqubecloud · 2025-10-27T18:21:46Z

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
77.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mihaibudiu · 2025-10-31T17:42:57Z

core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java

+  TOPDOWN_OPT("topDownOpt", Type.BOOLEAN, CalciteSystemProperty.TOPDOWN_OPT.value(), false),
+
+  /** Directory path for RuleMatchVisualizer output.
+   * If set, enables visualization of the rule matching process during query optimization.


does this enable visualization or just dumping the data to be visualized in a specific directory?

mihaibudiu · 2025-10-31T17:45:11Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+ * Utility class to enable RuleMatchVisualizer for Calcite connections.
+ *
+ * <p>This class provides hooks to automatically attach a RuleMatchVisualizer
+ * to planners when a connection specifies the ruleVisualizerDir property.


You use plural "planners". If there are multiple planners how are the visualizations for each of them distinguished? The output file name seems to be hardwired.

mihaibudiu · 2025-10-31T17:45:57Z

core/src/test/java/org/apache/calcite/util/RuleMatchVisualizerHookTest.java

+    // Enable visualizer
+    RuleMatchVisualizerHook.INSTANCE.enable(vizDir);
+
+    // Directory should be created


The hook should create the directory?

mihaibudiu · 2025-10-31T17:46:33Z

core/src/test/java/org/apache/calcite/util/RuleMatchVisualizerHookTest.java

+    assertThat(dataFiles, hasSize(1));
+    assertThat(htmlFiles, hasSize(1));
+    assertThat(dataFiles.get(0).getName(), containsString(DATA_FILE_PREFIX));
+    assertThat(htmlFiles.get(0).getName(), containsString(HTML_FILE_PREFIX));


should these files be deleted?

JUnit takes care of this, see TempDir.

mihaibudiu · 2025-10-31T17:49:21Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+   * Attaches a visualizer to the given planner.
+   */
+  private void attachVisualizer(RelOptPlanner planner, String outputDir) {
+


a few too many spaces around here

mihaibudiu · 2025-10-31T17:49:52Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+   */
+  private void attachVisualizer(RelOptPlanner planner, String outputDir) {
+
+      // Check if we've already attached a visualizer to this planner


"we" is probably not appropriate.

mihaibudiu · 2025-10-31T17:50:28Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+
+    int queryNum = queryCounter.incrementAndGet();
+    int queryStart = (int) System.currentTimeMillis() / 1000;
+    String suffix = String.format(Locale.ROOT, "query_%d_%d", queryNum, queryStart);


I see, the names are generated; this probably should be documented

I would suggest using plan_%d_%d, as a query might be compiled in several stages and have multiple plans. As far as I know, Calcite does not provide a query id. Some projects that use Calcite may do, though, and I wonder whether there's an easy way to include this in the file name. The query id could be passed as a setter/getter on the RelOptPlanner.

mihaibudiu · 2025-10-31T17:51:07Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+    visualizer.attachTo(planner);
+    visualizerMap.put(planner, visualizer);
+
+    // For HepPlanner, we need to manually write the output


I am not sure what is "manual" here.

A VolcanoPlanner will automatically write the output when its done with the planning. However, the writeToFile() needs to be called explicitly for a HepPlanner. So maybe explicitly would be a better word.

mihaibudiu · 2025-10-31T17:52:33Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+    hookCloseable.close();
+
+      // Write any pending visualizations
+    for (RuleMatchVisualizer viz : visualizerMap.values()) {


I find it strange that "disable" causes writing.
Maybe the method should be called "close", or "flush", or the write calls should be moved somewhere else.

thomasrebele · 2025-11-03T15:39:47Z

@tjbanghart, thank you for the PR! It makes it easier to use the RuleMatchVisualizer.
When I use the visualizer, I always have to add the same code: an AtomicInteger to distinguish the queries and some code to define the output. I would appreciate to get it into Calcite.

thomasrebele · 2025-11-03T15:44:51Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+
+    int queryNum = queryCounter.incrementAndGet();
+    int queryStart = (int) System.currentTimeMillis() / 1000;
+    String suffix = String.format(Locale.ROOT, "query_%d_%d", queryNum, queryStart);


I would suggest using plan_%d_%d, as a query might be compiled in several stages and have multiple plans. As far as I know, Calcite does not provide a query id. Some projects that use Calcite may do, though, and I wonder whether there's an easy way to include this in the file name. The query id could be passed as a setter/getter on the RelOptPlanner.

thomasrebele · 2025-11-03T16:09:08Z

core/src/main/java/org/apache/calcite/jdbc/CalciteConnectionImpl.java

+        RuleMatchVisualizerHook.INSTANCE.enableFromConnection(this);
+      } catch (Exception e) {
+        // Log but don't fail connection if visualizer setup fails
+        System.err.println("Warning: Failed to enable RuleMatchVisualizer: " + e.getMessage());


I would prefer using a LOGGER. Not sure whether a new method in org.apache.calcite.util.trace.CalciteTrace is needed, or whether a LOGGER can be defined as in org.apache.calcite.runtime.ResultSetEnumerable#LOGGER.

thomasrebele · 2025-11-03T16:20:05Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+    visualizer.attachTo(planner);
+    visualizerMap.put(planner, visualizer);
+
+    // For HepPlanner, we need to manually write the output


A VolcanoPlanner will automatically write the output when its done with the planning. However, the writeToFile() needs to be called explicitly for a HepPlanner. So maybe explicitly would be a better word.

thomasrebele · 2025-11-03T16:21:03Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+    // VolcanoPlanner automatically calls writeToFile() when done
+
+    System.out.println("RuleMatchVisualizer enabled: Output will be written to "
+        + outputDir + File.separator + suffix + "*");


Please use a LOGGER here as well.

thomasrebele · 2025-11-03T16:36:22Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+  /**
+   * Disables the visualizer.
+   */
+  public synchronized void disable() {


The disable() should be called somewhere as well. The Handler defined in org.apache.calcite.jdbc.Driver#createHandler could override onConnectionClose to call it. I would strongly recommend to make the calls to "enable" and "disable" in the same class. So either moving the init code to Driver#createHandler as well, or creating another method in CalciteConnectionImpl to complement the init() method.

thomasrebele · 2025-11-03T16:38:53Z

core/src/main/java/org/apache/calcite/util/RuleMatchVisualizerHook.java

+   *
+   * @param connection The Calcite connection
+   */
+  public synchronized void enableFromConnection(CalciteConnection connection) {


The code in CalciteConnectionImpl already checks for an existing dir, so it could just call the enable method directly.

thomasrebele · 2025-11-03T16:47:02Z

core/src/test/java/org/apache/calcite/util/RuleMatchVisualizerHookTest.java

+    assertThat(dataFiles, hasSize(1));
+    assertThat(htmlFiles, hasSize(1));
+    assertThat(dataFiles.get(0).getName(), containsString(DATA_FILE_PREFIX));
+    assertThat(htmlFiles.get(0).getName(), containsString(HTML_FILE_PREFIX));


JUnit takes care of this, see TempDir.

tjbanghart · 2025-11-04T16:52:51Z

Thank you both for the reviews! I'll be able to revisit shortly.

@thomasrebele since you're here, is there a way to highlight the cheapest path in the visualizer? If not is there an existing issue to add this functionality?

thomasrebele · 2025-11-05T09:19:40Z

@tjbanghart, could you explain what you mean by the cheapest path?

I've got a draft to improve the visualizer. You can have a look at #4506. If you have ideas for improvement, feel free to add a comment to https://issues.apache.org/jira/browse/CALCITE-7133 or create a new ticket and tag me in a comment.

The improvement has a feature to show a sub-graph connecting some specified nodes (and hiding all other nodes). Maybe that's close enough to your use case?

github-actions · 2025-12-06T03:45:41Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

tjbanghart force-pushed the calcite-7247 branch 5 times, most recently from 03007c4 to 008d47e Compare October 25, 2025 23:12

tjbanghart changed the title ~~[CALCITE-7427] Connection config for enabling RuleMatchVisualize~~ [CALCITE-7427] Add connection config to enable RuleMatchVisualizer Oct 25, 2025

tjbanghart force-pushed the calcite-7247 branch from 008d47e to 06288d4 Compare October 27, 2025 17:26

[CALCITE-7427] Connection config for enabling RuleMatchVisualize

7cc21d1

tjbanghart force-pushed the calcite-7247 branch from 06288d4 to 7cc21d1 Compare October 27, 2025 17:57

mihaibudiu reviewed Oct 31, 2025

View reviewed changes

thomasrebele reviewed Nov 3, 2025

View reviewed changes

github-actions bot added the stale label Dec 6, 2025

[CALCITE-7427] Add connection config to enable RuleMatchVisualizer #4602

Are you sure you want to change the base?

[CALCITE-7427] Add connection config to enable RuleMatchVisualizer #4602

Uh oh!

Conversation

tjbanghart commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Oct 27, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasrebele commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjbanghart commented Nov 4, 2025

Uh oh!

thomasrebele commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tjbanghart commented Oct 25, 2025 •

edited

Loading

thomasrebele commented Nov 5, 2025 •

edited

Loading