OAK-10527 Improve readability of the explain query output #1187

thomasmueller · 2023-11-03T08:40:03Z

I had a to change a lot of tests, because they depended (too much) on implementation details.

Even thought this would be a good idea, I did not switch to Json output so far, because:

some upstream UI tools depend on the query plans to have a certain form, which would break,
it would make the patch even larger,
the Lucene query string might contain double quotes, which would then be escaped, and so the output hard to read

So basically, first the upstream UI tools would

The Elasticsearch query is Json, which wouldn't need to be escaped if we add it as a child object.

nfsantos

I did not look at the tests, just have a minor comment.

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndex.java

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

oak-core/src/main/java/org/apache/jackrabbit/oak/query/ast/SelectorImpl.java

oak-core/src/test/java/org/apache/jackrabbit/oak/query/QueryFormatterTest.java

…/nodetype/NodeTypeIndex.java Co-authored-by: Fabrizio Fortino <fabrizio.fortino@gmail.com>

…rmatter.java Co-authored-by: Fabrizio Fortino <fabrizio.fortino@gmail.com>

…rmatterTest.java Co-authored-by: Fabrizio Fortino <fabrizio.fortino@gmail.com>

nfsantos

Just a few more comments.

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

nfsantos · 2023-11-06T17:05:39Z

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

+        if (language != null) {
+            return "xpath".equals(language);
+        }
+        query = query.trim().toLowerCase(Locale.ENGLISH);


If we do not want any locale related interpretation, it's better to use Locale.ROOT.

Locale.ENGLISH is safe according to https://stackoverflow.com/questions/10336730/which-locale-should-i-specify-when-i-call-stringtolowercase . We use that in other parts too, so I would keep this.

nfsantos · 2023-11-06T17:09:06Z

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

+        }
+        // union queries
+        while (query.startsWith("(")) {
+            query = query.substring("(".length()).trim();


Maybe replace with substring(1) since we are just removing the first character? I understand the reason for this convention when it is a word, prevents mistakes, but when it's only a single character, maybe it is simpler to use 1.

I used this for consistency, and to prevent errors in maintenance.

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

nfsantos · 2023-11-06T17:15:23Z

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

+        if (xpath) {
+            return formatXPath(query);
+        } else {
+            return formatSQL(query);


We are not sure here if the language is sql. If language is specified, add a check that it is either sql or xpath. If we autodetect, then can we check if it is sql by looking at if it starts with select? That way, we could have a nice error message saying that we do not recognize the language instead of failing with a syntax error or returning a string with newlines added in random places.

we could have a nice error message saying that we do not recognize the language

I don't see a need for this currently. The current use case is debug log, where we actually always know which language it is.

I do want to re-use this in the near future for http://oakutils.appspot.com/generate/index where the language detection is (unfortunately) incorrect.

nfsantos · 2023-11-06T17:16:09Z

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java

+        StringBuilder buff = new StringBuilder(query);
+        for (int i = 0; i < buff.length(); i++) {
+            char c = buff.charAt(i);
+            if (c == '\'' || c == '"') {


This could be transformed into a switch.

Yes. But that wouldn't necessarily be better. I prefer the "if else" in this case.

nfsantos · 2023-11-06T17:19:51Z

...core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/MultiPropertyOrTest.java

@@ -121,22 +121,17 @@ public void unionSortResultCount() throws Exception {
        root.commit();

        List<Integer> nodes = Lists.newArrayList();
-        Random r = new Random();
-        int seed = -2;
+        Random r = new Random(1);


Why? You want the test to be deterministic?

sonarcloud · 2023-11-07T09:05:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
18 Code Smells

87.3% Coverage
0.0% Duplication

The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

amit-jain

LGTM. Haven't looked at the tests carefully though.

OAK-10527 Improve readability of the explain query output

8551bae

thomasmueller requested review from fabriziofortino, amit-jain, nit0906 and tihom88 November 3, 2023 08:40

thomasmueller added 2 commits November 3, 2023 11:07

OAK-10527 Improve readability of the explain query output

445ec09

OAK-10527 Improve readability of the explain query output

833b7bc

nfsantos reviewed Nov 3, 2023

View reviewed changes

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java Outdated Show resolved Hide resolved

thomasmueller added 2 commits November 3, 2023 12:11

OAK-10527 Improve readability of the explain query output

ae90b67

OAK-10527 Improve readability of the explain query output

711c32c

nfsantos reviewed Nov 3, 2023

View reviewed changes

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFormatter.java Show resolved Hide resolved

OAK-10527 Improve readability of the explain query output

c706175

fabriziofortino requested changes Nov 3, 2023

View reviewed changes

thomasmueller and others added 5 commits November 3, 2023 18:16

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

b1139ad

…/nodetype/NodeTypeIndex.java Co-authored-by: Fabrizio Fortino <fabrizio.fortino@gmail.com>

Update oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFo…

8f0bf4d

…rmatter.java Co-authored-by: Fabrizio Fortino <fabrizio.fortino@gmail.com>

Update oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryFo…

d83f0e1

…rmatter.java Co-authored-by: Fabrizio Fortino <fabrizio.fortino@gmail.com>

Update oak-core/src/test/java/org/apache/jackrabbit/oak/query/QueryFo…

612debf

…rmatterTest.java Co-authored-by: Fabrizio Fortino <fabrizio.fortino@gmail.com>

OAK-10527 Improve readability of the explain query output

04e2f8e

thomasmueller requested review from fabriziofortino and nfsantos November 4, 2023 08:14

fabriziofortino approved these changes Nov 4, 2023

View reviewed changes

OAK-10527 Improve readability of the explain query output

1c3f7ce

nfsantos reviewed Nov 6, 2023

View reviewed changes

thomasmueller added 3 commits November 7, 2023 07:50

OAK-10527 Improve readability of the explain query output

3d7aad0

OAK-10527 Improve readability of the explain query output

fb2a1f1

OAK-10527 Improve readability of the explain query output

f77fd7e

thomasmueller requested a review from nfsantos November 7, 2023 09:02

nfsantos approved these changes Nov 7, 2023

View reviewed changes

amit-jain approved these changes Nov 7, 2023

View reviewed changes

thomasmueller merged commit 04f3c97 into trunk Nov 7, 2023
0 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAK-10527 Improve readability of the explain query output #1187

OAK-10527 Improve readability of the explain query output #1187

thomasmueller commented Nov 3, 2023

nfsantos left a comment

nfsantos left a comment

nfsantos Nov 6, 2023

thomasmueller Nov 7, 2023

nfsantos Nov 6, 2023

thomasmueller Nov 7, 2023 •

edited

nfsantos Nov 6, 2023

thomasmueller Nov 7, 2023

nfsantos Nov 6, 2023

thomasmueller Nov 7, 2023

nfsantos Nov 6, 2023

thomasmueller Nov 7, 2023

sonarcloud bot commented Nov 7, 2023

amit-jain left a comment

OAK-10527 Improve readability of the explain query output #1187

OAK-10527 Improve readability of the explain query output #1187

Conversation

thomasmueller commented Nov 3, 2023

nfsantos left a comment

Choose a reason for hiding this comment

nfsantos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasmueller Nov 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 7, 2023

amit-jain left a comment

Choose a reason for hiding this comment

thomasmueller Nov 7, 2023 •

edited