Enhancement #3028: Add verbosity-level option to SQL EXPLAIN #3485

satishsuryanarayan · 2025-07-15T14:36:42Z

No description provided.

Commit for Issue 3028

satishsuryanarayan · 2025-07-15T14:41:45Z

@alecgrieser @ohadzeliger @normen662 @arnaud-lacurie My first PR - please review at your convenience.

…rd-layer Get latest

…sitFullDescribeStatement

normen662 · 2025-07-23T12:31:50Z

...-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/explain/ExplainLevel.java

 /**
 * Explain level. Adjusts the granularity of explain output.
 */
-public final class ExplainLevel {
+public enum ExplainLevel {


Let's not introduce yet another naming of the same underlying thing. Can't you make the ExplainLevel an enum directly?

normen662 · 2025-07-23T12:32:12Z

fdb-relational-core/src/main/antlr/RelationalLexer.g4

@@ -1346,8 +1348,6 @@ fragment INT_TYPE_MODIFIER:          ('I' | 'i');
 fragment LONG_TYPE_MODIFIER:         ('L' | 'l');
 fragment DECIMAL_TYPE_MODIFIER:      (INT_TYPE_MODIFIER | LONG_TYPE_MODIFIER);

-


please undo

satishsuryanarayan · 2025-07-23T19:24:16Z

fdb-relational-core/src/main/antlr/RelationalLexer.g4

@@ -1352,4 +1354,4 @@ fragment DECIMAL_TYPE_MODIFIER:      (INT_TYPE_MODIFIER | LONG_TYPE_MODIFIER);

 ERROR_RECOGNITION
    : . { this.notifyListeners(new LexerNoViableAltException(this, _input, _tokenStartCharIndex, null)); }
-    ;
+    ;


@normen662 Done!

satishsuryanarayan

@normen662 I undid my changes to ExplainLevel as suggested.

hatyo

Thank you for creating the PR, nice work, I left few comments.

Please align your PR description with the conventions in this repo adding explanation, and referencing the corresponding GitHub issue with a hyperlink, this is important for other downstream GitHub tasks that generate the changelog, you can look here for example:
#3511

hatyo · 2025-08-06T16:54:10Z

...n/java/com/apple/foundationdb/relational/recordlayer/query/MutablePlanGenerationContext.java

@@ -89,6 +88,8 @@ public class MutablePlanGenerationContext implements QueryExecutionContext {

    private boolean forExplain;

+    private int explainLevel;


would be nicer to use an enum instead of int to make the intention clear, you can also use a boolean if choices degenerate to two (verbose / not verbose)

hatyo · 2025-08-06T19:06:49Z

fdb-relational-core/src/main/antlr/RelationalParser.g4

@@ -645,7 +645,8 @@ helpStatement
 // details

 describeObjectClause
-    : (
+    : level=(VERBOSE | MINIMAL)?


I don't think we want MINIMAL verbosity here. Please remove it and leave an optional VERBOSE.

hatyo · 2025-08-06T19:09:35Z

...n/java/com/apple/foundationdb/relational/recordlayer/query/MutablePlanGenerationContext.java

@@ -261,6 +262,7 @@ public MutablePlanGenerationContext(@Nonnull PreparedParams preparedParams,
        constantObjectValues = new LinkedList<>();
        shouldProcessLiteral = true;
        forExplain = false;
+        this.explainLevel = ExplainLevel.SOME_DETAILS;


Suggested change

this.explainLevel = ExplainLevel.SOME_DETAILS;

explainLevel = ExplainLevel.SOME_DETAILS;

hatyo · 2025-08-06T19:09:54Z

...n/java/com/apple/foundationdb/relational/recordlayer/query/MutablePlanGenerationContext.java

@@ -372,6 +378,10 @@ public void setForExplain(boolean forExplain) {
        this.forExplain = forExplain;
    }

+    public void setExplainLevel(final int explainLevel) {


Suggested change

public void setExplainLevel(final int explainLevel) {

public void setExplainLevel(int explainLevel) {

hatyo · 2025-08-06T19:10:43Z

...ava/com/apple/foundationdb/relational/recordlayer/query/NormalizedQueryExecutionContext.java

@@ -146,6 +159,12 @@ public Builder setForExplain(boolean isForExplain) {
            return this;
        }

+        @Nonnull
+        public Builder setExplainLevel(int level) {
+            this.explainLevel = level;


Suggested change

this.explainLevel = level;

explainLevel = level;

hatyo · 2025-08-06T19:11:14Z

...ava/com/apple/foundationdb/relational/recordlayer/query/NormalizedQueryExecutionContext.java

@@ -107,6 +117,8 @@ public static final class Builder {

        private boolean isForExplain;

+        private int explainLevel;


Suggested change

private int explainLevel;

private boolean isVerboseExplain;

hatyo · 2025-08-06T19:12:23Z

...nal-core/src/test/java/com/apple/foundationdb/relational/recordlayer/query/ExplainTests.java

@@ -73,7 +73,7 @@ void explainResultSetMetadataTest() throws Exception {
        final var expectedContTypes = List.of(Types.BINARY, Types.INTEGER, Types.VARCHAR);
        try (var ddl = Ddl.builder().database(URI.create("/TEST/QT")).relationalExtension(relationalExtension).schemaTemplate(schemaTemplate).build()) {
            executeInsert(ddl);
-            try (RelationalPreparedStatement ps = ddl.setSchemaAndGetConnection().prepareStatement("EXPLAIN SELECT * FROM RestaurantComplexRecord")) {
+            try (RelationalPreparedStatement ps = ddl.setSchemaAndGetConnection().prepareStatement("EXPLAIN VERBOSE SELECT * FROM RestaurantComplexRecord")) {


please revert, and add targeted tests for verbose in separate tests

hatyo · 2025-08-06T19:24:39Z

...src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/QueryVisitor.java

+            getDelegate().getPlanGenerationContext().setExplainLevel(ExplainLevel.ALL_DETAILS);
+        } else if (!ctx.describeObjectClause().getTokens(RelationalLexer.MINIMAL).isEmpty()) {
+            getDelegate().getPlanGenerationContext().setExplainLevel(ExplainLevel.STRUCTURE);


I don't think we care about this level of distinction here, only an optional verbose, it is aligned with Postgres.

satishsuryanarayan · 2025-08-07T14:25:35Z

Thank you for creating the PR, nice work, I left few comments.

Please align your PR description with the conventions in this repo adding explanation, and referencing the corresponding GitHub issue with a hyperlink, this is important for other downstream GitHub tasks that generate the changelog, you can look here for example: #3511

@hatyo Thanks for the feedback - I will work on the suggestions.

…rd-layer

satishsuryanarayan · 2025-08-07T15:20:34Z

@hatyo I updated the PR with the suggested changes. Please review at your convenience.

satishsuryanarayan added 3 commits July 15, 2025 01:33

Commit for Issue 3028

6fbe5b8

Merge branch 'main' into ss/3028

d7da0ab

Merge pull request #2 from satishsuryanarayan/ss/3028

ecb37aa

Commit for Issue 3028

satishsuryanarayan added 9 commits July 16, 2025 20:53

Remove unused import

d9eedc9

Merge branch 'FoundationDB:main' into main

36178ea

Add explain level to NormalizedQueryExecutionContext

d02442c

Merge branch 'FoundationDB:main' into main

a9d07da

Revert to version in main

a4a2ace

Merge branch 'main' of https://github.com/satishsuryanarayan/fdb-reco…

e6c84b0

…rd-layer Get latest

Don't overwrite explainLevel

e5624c8

Override visitDescribeStatements instead of adding explainLevel to vi…

96fdbd0

…sitFullDescribeStatement

Added tests for explains

b62e176

normen662 requested changes Jul 23, 2025

View reviewed changes

Changes suggested by Norman

c8b54b2

satishsuryanarayan commented Jul 23, 2025

View reviewed changes

hatyo requested changes Aug 6, 2025

View reviewed changes

satishsuryanarayan added 3 commits August 7, 2025 09:46

Merge branch 'FoundationDB:main' into main

69fd3cc

Explain supports two options - VERBOSE or default

7a3d4cb

Merge branch 'main' of https://github.com/satishsuryanarayan/fdb-reco…

82fdb9e

…rd-layer

		@@ -1346,8 +1348,6 @@ fragment INT_TYPE_MODIFIER: ('I' \| 'i');
		fragment LONG_TYPE_MODIFIER: ('L' \| 'l');
		fragment DECIMAL_TYPE_MODIFIER: (INT_TYPE_MODIFIER \| LONG_TYPE_MODIFIER);

		@@ -89,6 +88,8 @@ public class MutablePlanGenerationContext implements QueryExecutionContext {

		private boolean forExplain;

		private int explainLevel;

	this.explainLevel = ExplainLevel.SOME_DETAILS;
	explainLevel = ExplainLevel.SOME_DETAILS;

	public void setExplainLevel(final int explainLevel) {
	public void setExplainLevel(int explainLevel) {

		@@ -107,6 +117,8 @@ public static final class Builder {

		private boolean isForExplain;

		private int explainLevel;

Enhancement #3028: Add verbosity-level option to SQL EXPLAIN #3485

Are you sure you want to change the base?

Enhancement #3028: Add verbosity-level option to SQL EXPLAIN #3485

Uh oh!

Conversation

satishsuryanarayan commented Jul 15, 2025

Uh oh!

satishsuryanarayan commented Jul 15, 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!

satishsuryanarayan left a comment

Choose a reason for hiding this comment

Uh oh!

hatyo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

satishsuryanarayan commented Aug 7, 2025

Uh oh!

satishsuryanarayan commented Aug 7, 2025

Uh oh!

Uh oh!

hatyo left a comment •

edited

Loading