formatting explain with stable aliases #3015

normen662 · 2025-01-06T20:30:01Z

Reimplementation of stringification of plans:

differentiation of use cases: for debugging versus for external explain
external explains use self contained symbols; for debugging uses plan aliases
iterated on ExplainPlanVisitor to create a token list in a first step then is then rendered in a second step
all TreeLikes get explain methods that are called from toString()
two renderers:
a. renderer for external explains and logging(supports maximum length as well as different explain levels: STRUCTURE, SOME_DETAILS, ALL_DETAILS
b. renderer for pretty explain

foundationdb-ci · 2025-01-06T21:17:33Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 2854ca4
Duration 0:47:19
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-07T10:52:12Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 0bb6fde
Duration 0:34:25
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-07T11:23:03Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 45974e7
Duration 0:35:49
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-07T21:25:25Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 5f1e42f
Duration 0:47:24
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-08T08:44:27Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: e49c3ef
Duration 0:47:01
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

alecgrieser

I've only looked at the new formatter framework classes so far, but I figured you'd want feedback on that first, so I'm submitting what I have now

...ayer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ExplainTokens.java

alecgrieser · 2025-01-08T11:46:14Z

...er-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

-                logger.debug(KeyValueLogMessage.of("explain of plan",
+                logger.debug(KeyValueLogMessage.of("GML explain of plan",
                        "explain", PlannerGraphProperty.explain(singleRoot)));
+                logger.debug(KeyValueLogMessage.of("string explain of plan",
+                        "explain", ExplainPlanVisitor.toStringForDebugging((RecordQueryPlan)singleRoot)));


Should this be consolidated into a single log message with both the GML and the string explain? Arguably, this should just have the string explain, as I think that will be more useful for humans reading through the logs, if we even want to rely on this log line at all.

There is not enough testing for GML that's why it's there in the first place. Maybe I should open a ticket for that. For now I think this is safer and will not ever be triggered other than in local debugging sessions.

Okay, got it. But would it make sense to collapse both calls to a single log message, like:

logger.debug(KeyValueLogMessage.of("explain of plan", "explain_gml", PlannerGraphProperty.explain(singleRoot), "explain_string", ExplainPlanVisitor.toStringForDebugging((RecordQueryPlan)singleRoot)));

I think you get the same amount of coverage, but only one log message. Which is something we sort of care about, even for debug logs

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

...in/java/com/apple/foundationdb/record/query/plan/cascades/ExplainSelfContainedSymbolMap.java

.../src/main/java/com/apple/foundationdb/record/query/plan/cascades/PrettyExplainFormatter.java

foundationdb-ci · 2025-01-08T16:38:19Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 01f1689
Duration 0:47:03
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-08T17:06:54Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: a677078
Duration 0:47:09
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

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

...er-core/src/main/java/com/apple/foundationdb/record/query/plan/explain/ExplainSymbolMap.java

...ain/java/com/apple/foundationdb/record/query/plan/explain/ExplainSelfContainedSymbolMap.java

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

hatyo · 2025-01-08T19:52:24Z

...c/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/QueryPredicate.java

+    }
+
+    @Nonnull
+    ExplainTokensWithPrecedence explain(@Nonnull Iterable<Supplier<ExplainTokensWithPrecedence>> explainSuppliers);


Suggested change

ExplainTokensWithPrecedence explain(@Nonnull Iterable<Supplier<ExplainTokensWithPrecedence>> explainSuppliers);

ExplainTokensWithPrecedence explain(@Nonnull Iterable<Supplier<ExplainTokensWithPrecedence>> childrenExplainSuppliers);

...-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/Type.java

...ore/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/CollateValue.java

...layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/Value.java

foundationdb-ci · 2025-01-09T12:12:23Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 0917e96
Duration 0:47:11
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

hatyo

LGTM, nice work!

* numerous refactorings from describe and explain * rewriting the explain visitor * able to render plan explains * explain levels * unit tests work * touch ups * added documentation * addressing Alec's comments * more renamings * addressing yhatem's comments

normen662 added 5 commits January 6, 2025 21:28

numerous refactorings from describe and explain

85c54ad

rewriting the explain visitor

8959844

able to render plan explains

4d855cb

explain levels

5e7f8fc

unit tests work

2854ca4

normen662 force-pushed the formatting-explain branch from 0bb6fde to 45974e7 Compare January 7, 2025 10:47

touch ups

5f1e42f

normen662 force-pushed the formatting-explain branch from 45974e7 to 5f1e42f Compare January 7, 2025 20:37

normen662 requested a review from hatyo January 7, 2025 20:38

added documentation

e49c3ef

alecgrieser requested changes Jan 8, 2025

View reviewed changes

normen662 added 2 commits January 8, 2025 16:50

addressing Alec's comments

01f1689

more renamings

a677078

alecgrieser previously approved these changes Jan 8, 2025

View reviewed changes

hatyo requested changes Jan 8, 2025

View reviewed changes

addressing yhatem's comments

0917e96

normen662 dismissed alecgrieser’s stale review via 0917e96 January 9, 2025 11:25

hatyo approved these changes Jan 9, 2025

View reviewed changes

normen662 merged commit 8386a32 into FoundationDB:main Jan 9, 2025
1 check passed

arnaud-lacurie mentioned this pull request Jan 10, 2025

Fix post merge issue #3025

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

formatting explain with stable aliases #3015

formatting explain with stable aliases #3015

normen662 commented Jan 6, 2025 •

edited

Loading

foundationdb-ci commented Jan 6, 2025

foundationdb-ci commented Jan 7, 2025

foundationdb-ci commented Jan 7, 2025

foundationdb-ci commented Jan 7, 2025

foundationdb-ci commented Jan 8, 2025

alecgrieser left a comment

alecgrieser Jan 8, 2025

normen662 Jan 8, 2025

normen662 Jan 8, 2025

alecgrieser Jan 8, 2025

foundationdb-ci commented Jan 8, 2025

foundationdb-ci commented Jan 8, 2025

hatyo Jan 8, 2025

foundationdb-ci commented Jan 9, 2025

hatyo left a comment

	ExplainTokensWithPrecedence explain(@Nonnull Iterable<Supplier<ExplainTokensWithPrecedence>> explainSuppliers);
	ExplainTokensWithPrecedence explain(@Nonnull Iterable<Supplier<ExplainTokensWithPrecedence>> childrenExplainSuppliers);

formatting explain with stable aliases #3015

formatting explain with stable aliases #3015

Conversation

normen662 commented Jan 6, 2025 • edited Loading

foundationdb-ci commented Jan 6, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 7, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 7, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 7, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 8, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

alecgrieser left a comment

Choose a reason for hiding this comment

alecgrieser Jan 8, 2025

Choose a reason for hiding this comment

normen662 Jan 8, 2025

Choose a reason for hiding this comment

normen662 Jan 8, 2025

Choose a reason for hiding this comment

alecgrieser Jan 8, 2025

Choose a reason for hiding this comment

foundationdb-ci commented Jan 8, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 8, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

hatyo Jan 8, 2025

Choose a reason for hiding this comment

foundationdb-ci commented Jan 9, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

hatyo left a comment

Choose a reason for hiding this comment

normen662 commented Jan 6, 2025 •

edited

Loading