From bc6ee219520e55bcdb2a36c0499d265d426f298c Mon Sep 17 00:00:00 2001 From: Julian Hyde Date: Fri, 27 Feb 2026 22:36:08 -0800 Subject: [PATCH 1/6] [CALCITE-7424] LintTest should check that files marked with '// lint: sort' are sorted Replace the hard-coded testContributorsFileIsSorted test with a general '// lint: sort' directive that files can embed to declare their own sort requirements. Add Sort and SortConsumer classes to parse and enforce the directive. Annotate .mailmap and contributors.yml with the directive. Co-Authored-By: Claude Sonnet 4.6 --- .mailmap | 3 + .../org/apache/calcite/test/LintTest.java | 294 ++++++++++++++++-- site/_data/contributors.yml | 3 + 3 files changed, 277 insertions(+), 23 deletions(-) diff --git a/.mailmap b/.mailmap index 094cc7b95cac..a0784207f8ad 100644 --- a/.mailmap +++ b/.mailmap @@ -15,6 +15,9 @@ # See the License for the specific language governing permissions and # limitations under the License. # +# The following directive tells LintTest to ensure that this file is sorted: +# // lint: sort where '^[^#]' +# Abhishek Dasgupta Adam Kennedy Alan Jin diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java b/core/src/test/java/org/apache/calcite/test/LintTest.java index 2d6953d13385..98ad582380a6 100644 --- a/core/src/test/java/org/apache/calcite/test/LintTest.java +++ b/core/src/test/java/org/apache/calcite/test/LintTest.java @@ -28,6 +28,7 @@ import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.dataformat.yaml.YAMLMapper; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -63,6 +64,7 @@ import static org.junit.jupiter.api.Assumptions.assumeTrue; import static java.lang.Integer.parseInt; +import static java.util.Objects.requireNonNull; import static java.util.regex.Pattern.compile; /** Various automated checks on the code and git history. */ @@ -193,9 +195,63 @@ && isJava(line.filename()), line -> line.state().ulCount++) .add(line -> line.contains(""), line -> line.state().ulCount--) + + // Apply active sort consumer to each line. + .add(line -> line.state().sortConsumer != null, + line -> requireNonNull(line.state().sortConsumer).accept(line)) + + // Start sorting when a "// lint: sort until ..." directive is found. + .add(line -> line.contains("// lint: sort") + && !line.source().fileOpt() + .filter(f -> f.getName().equals("LintTest.java")).isPresent(), + line -> { + line.state().sortConsumer = null; + boolean continued = line.line().endsWith("\\"); + if (continued) { + line.state().partialSort = ""; + } else { + line.state().partialSort = null; + Sort sort = Sort.parse(line.line()); + if (sort != null) { + line.state().sortConsumer = new SortConsumer(sort); + } + } + }) + + // Continue accumulating a multi-line sort specification. + .add(line -> line.state().partialSort != null, + line -> { + String thisLine = line.line(); + boolean continued = thisLine.endsWith("\\"); + if (continued) { + thisLine = skipLast(thisLine); + } + final String nextLine; + if (requireNonNull(line.state().partialSort).isEmpty()) { + nextLine = thisLine; + } else { + thisLine = thisLine.replaceAll("^ *(// )?", ""); + nextLine = line.state().partialSort + thisLine; + } + if (continued) { + line.state().partialSort = nextLine; + } else { + line.state().partialSort = null; + Sort sort = Sort.parse(nextLine); + if (sort != null) { + line.state().sortConsumer = new SortConsumer(sort); + } + } + }) + .build(); } + /** Strips the last character from a string. */ + private static String skipLast(String s) { + return s.substring(0, s.length() - 1); + } + /** Returns whether we are currently in a region where lint rules should not * be applied. */ private static boolean skipping(Puffin.Line line) { @@ -497,23 +553,6 @@ private static void checkMessage(String subject, String body, } } - /** Ensures that the {@code contributors.yml} file is sorted by name. */ - @Test void testContributorsFileIsSorted() throws IOException { - final ObjectMapper mapper = new YAMLMapper(); - final File contributorsFile = ROOT_PATH.resolve("site/_data/contributors.yml").toFile(); - JavaType listType = - mapper.getTypeFactory() - .constructCollectionType(List.class, Contributor.class); - List contributors = - mapper.readValue(contributorsFile, listType); - Contributor contributor = - firstOutOfOrder(contributors, - Comparator.comparing(c -> c.name, String.CASE_INSENSITIVE_ORDER)); - if (contributor != null) { - fail("contributor '" + contributor.name + "' is out of order"); - } - } - /** Ensures that the {@code .mailmap} file is sorted. */ @Test void testMailmapFile() { final File mailmapFile = ROOT_PATH.resolve(".mailmap").toFile(); @@ -591,6 +630,8 @@ private static class FileState { int javadocEndLine; int blockquoteCount; int ulCount; + @Nullable String partialSort; + @Nullable Consumer> sortConsumer; FileState(GlobalState global) { this.global = global; @@ -605,13 +646,220 @@ public boolean inJavadoc() { } } - /** Contributor element in "contributors.yaml" file. */ - @JsonIgnoreProperties(ignoreUnknown = true) - private static class Contributor { - final String name; + /** Tests the sort specification syntax. */ + @Test void testSort() { + // With "until" and "where": 'case b' arrives after 'case c', 'd'. + checkSortSpec( + "class Test {\n" + + " switch (x) {\n" + + " // lint: sort until '#}' where '##case '\n" + + " case a\n" + + " case c\n" + + " case d\n" + + " case b\n" + + " case e\n" + + " }\n" + + "}\n", + "GuavaCharSource{memory}:7:" + + "Lines must be sorted; ' case b' should be before ' case c'"); + + // Cases after "until" are checked against the same sorted list. + checkSortSpec( + "class Test {\n" + + " switch (x) {\n" + + " // lint: sort until '#}' where '##case '\n" + + " case x\n" + + " case y\n" + + " case z\n" + + " }\n" + + " switch (y) {\n" + + " case a\n" + + " }\n" + + "}\n", + "GuavaCharSource{memory}:9:" + + "Lines must be sorted; ' case a' should be before ' case x'"); + + // Change '#}' to '##}': consumer stops at the same-indent '}', so + // the second switch's cases are not compared. No violations. + checkSortSpec( + "class Test {\n" + + " switch (x) {\n" + + " // lint: sort until '##}' where '##case '\n" + + " case x\n" + + " case y\n" + + " case z\n" + + " }\n" + + " switch (y) {\n" + + " case a\n" + + " }\n" + + "}\n"); + + // Specification has "until", "where" and "erase" clauses. + checkSortSpec( + "class Test {\n" + + " // lint: sort until '#}' where '##A::' erase '^ .*::'\n" + + " A::c\n" + + " A::a\n" + + " A::b\n" + + " }\n" + + "}\n", + "GuavaCharSource{memory}:4:" + + "Lines must be sorted; 'a' should be before 'c'"); + + // Specification spread over multiple lines using '\' continuation. + checkSortSpec( + "class Test {\n" + + " // lint: sort until '#}'\\\n" + + " // where '##A::'\\\n" + + " // erase '^ .*::'\n" + + " A::c\n" + + " A::a\n" + + " A::b\n" + + " }\n" + + "}\n", + "GuavaCharSource{memory}:6:" + + "Lines must be sorted; 'a' should be before 'c'"); + } - @JsonCreator Contributor(@JsonProperty("name") String name) { - this.name = name; + private void checkSortSpec(String code, String... expectedMessages) { + final Puffin.Program program = makeProgram(); + final StringWriter sw = new StringWriter(); + final GlobalState g; + try (PrintWriter pw = new PrintWriter(sw)) { + g = program.execute(Stream.of(Sources.of(code)), pw); + } + assertThat(g.messages.toString(), + is("[" + String.join(", ", expectedMessages) + "]")); + } + + /** Specification for a sort directive, parsed from a comment like + * {@code // lint: sort until 'pattern' where 'filter' erase 'erasePattern'}. + * + *

Supports indentation placeholders: + *

    + *
  • {@code ##} - the directive line's indentation (as a regex anchor) + *
  • {@code #} - one level up (indent minus 2 spaces) + *
+ */ + private static class Sort { + final Pattern until; + final @Nullable Pattern where; + final @Nullable Pattern erase; + + Sort(Pattern until, @Nullable Pattern where, @Nullable Pattern erase) { + this.until = until; + this.where = where; + this.erase = erase; + } + + /** Parses a sort directive from a line like + * {@code // lint: sort until 'X' where 'Y' erase 'Z'}. + * Returns null if parsing fails. */ + static @Nullable Sort parse(String line) { + int sortIndex = line.indexOf("lint: sort"); + if (sortIndex < 0) { + return null; + } + final String spec = + line.substring(sortIndex + "lint: sort".length()).trim(); + int indent = 0; + while (indent < line.length() && line.charAt(indent) == ' ') { + indent++; + } + Pattern until = extractPattern(spec, "until", indent); + if (until == null) { + return null; + } + Pattern where = extractPattern(spec, "where", indent); + Pattern erase = extractPattern(spec, "erase", indent); + return new Sort(until, where, erase); + } + + /** Extracts and compiles the pattern from a clause like + * {@code until 'pattern'}. Returns null if the keyword is absent or + * the pattern is malformed. */ + private static @Nullable Pattern extractPattern( + String spec, String keyword, int indent) { + int keywordIndex = spec.indexOf(keyword); + if (keywordIndex < 0) { + return null; + } + final String rest = + spec.substring(keywordIndex + keyword.length()).trim(); + if (rest.isEmpty() || rest.charAt(0) != '\'') { + return null; + } + int endQuote = rest.indexOf('\'', 1); + if (endQuote < 0) { + return null; + } + String pattern = rest.substring(1, endQuote); + pattern = pattern.replace("##", "^" + Strings.repeat(" ", indent)); + pattern = + pattern.replace("#", "^" + Strings.repeat(" ", Math.max(0, indent - 2))); + try { + return compile(pattern); + } catch (Exception e) { + return null; + } + } + } + + /** Checks that lines in a sorted region are in order. */ + private static class SortConsumer + implements Consumer> { + final Sort sort; + final Comparator comparator = String.CASE_INSENSITIVE_ORDER; + final List lines = new ArrayList<>(); + boolean done = false; + + SortConsumer(Sort sort) { + this.sort = sort; + } + + @Override public void accept(Puffin.Line line) { + if (done) { + return; + } + final String thisLine = line.line(); + + // End of sorted region. + if (sort.until.matcher(thisLine).find()) { + done = true; + line.state().sortConsumer = null; + return; + } + + // If a "where" filter is present, skip non-matching lines. + if (sort.where != null && !sort.where.matcher(thisLine).find()) { + return; + } + + // Apply the "erase" pattern before comparing. + String compareLine = thisLine; + if (sort.erase != null) { + compareLine = sort.erase.matcher(thisLine).replaceAll(""); + } + + addLine(line, compareLine); + } + + private void addLine(Puffin.Line line, + String thisLine) { + if (!lines.isEmpty()) { + final String prevLine = lines.get(lines.size() - 1); + if (comparator.compare(prevLine, thisLine) > 0) { + final String earlierLine = + Util.filter(lines, s -> comparator.compare(s, thisLine) > 0) + .iterator().next(); + line.state().message( + String.format(Locale.ROOT, + "Lines must be sorted; '%s' should be before '%s'", + thisLine, earlierLine), + line); + } + } + lines.add(thisLine); } } } diff --git a/site/_data/contributors.yml b/site/_data/contributors.yml index f8347e026a59..8a075acd9445 100644 --- a/site/_data/contributors.yml +++ b/site/_data/contributors.yml @@ -17,7 +17,10 @@ # Database of contributors to Apache Calcite. # Pages such as developer.md use this data. +# # List must be sorted by first name, last name. +# The following directive tells LintTest to check: +# // lint: sort where ' name:' erase '^.*name:' # - name: Alan Gates emeritus: 2018/05/04 From f1e04828cc02ee3f81d770678dbbce98f52faf03 Mon Sep 17 00:00:00 2001 From: Julian Hyde Date: Fri, 27 Feb 2026 22:54:40 -0800 Subject: [PATCH 2/6] Improve 7424 --- .../java/org/apache/calcite/test/LintTest.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java b/core/src/test/java/org/apache/calcite/test/LintTest.java index 98ad582380a6..c1356e0141c6 100644 --- a/core/src/test/java/org/apache/calcite/test/LintTest.java +++ b/core/src/test/java/org/apache/calcite/test/LintTest.java @@ -742,11 +742,11 @@ private void checkSortSpec(String code, String... expectedMessages) { * */ private static class Sort { - final Pattern until; + final @Nullable Pattern until; final @Nullable Pattern where; final @Nullable Pattern erase; - Sort(Pattern until, @Nullable Pattern where, @Nullable Pattern erase) { + Sort(@Nullable Pattern until, @Nullable Pattern where, @Nullable Pattern erase) { this.until = until; this.where = where; this.erase = erase; @@ -767,9 +767,6 @@ private static class Sort { indent++; } Pattern until = extractPattern(spec, "until", indent); - if (until == null) { - return null; - } Pattern where = extractPattern(spec, "where", indent); Pattern erase = extractPattern(spec, "erase", indent); return new Sort(until, where, erase); @@ -794,9 +791,11 @@ private static class Sort { return null; } String pattern = rest.substring(1, endQuote); - pattern = pattern.replace("##", "^" + Strings.repeat(" ", indent)); - pattern = - pattern.replace("#", "^" + Strings.repeat(" ", Math.max(0, indent - 2))); + if (!pattern.contains("[#")) { + pattern = pattern.replace("##", "^" + Strings.repeat(" ", indent)); + pattern = + pattern.replace("#", "^" + Strings.repeat(" ", Math.max(0, indent - 2))); + } try { return compile(pattern); } catch (Exception e) { @@ -824,7 +823,7 @@ private static class SortConsumer final String thisLine = line.line(); // End of sorted region. - if (sort.until.matcher(thisLine).find()) { + if (sort.until != null && sort.until.matcher(thisLine).find()) { done = true; line.state().sortConsumer = null; return; From e4b9940a92d289186f9e4258c738c3bb4c76a7b9 Mon Sep 17 00:00:00 2001 From: Julian Hyde Date: Fri, 27 Feb 2026 23:23:13 -0800 Subject: [PATCH 3/6] Improve 7424 further --- core/src/test/java/org/apache/calcite/test/LintTest.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java b/core/src/test/java/org/apache/calcite/test/LintTest.java index c1356e0141c6..661aa94f17fa 100644 --- a/core/src/test/java/org/apache/calcite/test/LintTest.java +++ b/core/src/test/java/org/apache/calcite/test/LintTest.java @@ -22,12 +22,6 @@ import org.apache.calcite.util.TestUnsafe; import org.apache.calcite.util.Util; -import com.fasterxml.jackson.annotation.JsonCreator; -import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.databind.JavaType; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.dataformat.yaml.YAMLMapper; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; From 7de7499bc2fd2ed48516c3ce862040c206d22506 Mon Sep 17 00:00:00 2001 From: Julian Hyde Date: Fri, 27 Feb 2026 23:45:24 -0800 Subject: [PATCH 4/6] Improve 7424 further still --- core/src/test/java/org/apache/calcite/test/LintTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java b/core/src/test/java/org/apache/calcite/test/LintTest.java index 661aa94f17fa..0b69f8ee7a4b 100644 --- a/core/src/test/java/org/apache/calcite/test/LintTest.java +++ b/core/src/test/java/org/apache/calcite/test/LintTest.java @@ -67,6 +67,7 @@ class LintTest { * space. */ private static final Pattern CALCITE_PATTERN = compile("^(\\[CALCITE-[0-9]{1,4}][ ]).*"); + private static final Pattern PATTERN = compile("^ *(// )?"); private static final Path ROOT_PATH = Paths.get(System.getProperty("gradle.rootDir")); private static final String TERMINOLOGY_ERROR_MSG = @@ -224,7 +225,7 @@ && isJava(line.filename()), if (requireNonNull(line.state().partialSort).isEmpty()) { nextLine = thisLine; } else { - thisLine = thisLine.replaceAll("^ *(// )?", ""); + thisLine = PATTERN.matcher(thisLine).replaceAll(""); nextLine = line.state().partialSort + thisLine; } if (continued) { @@ -445,7 +446,7 @@ private static final class TermRule { private final Set validTerms; TermRule(String regex, String... validTerms) { - this.termPattern = Pattern.compile(regex, Pattern.CASE_INSENSITIVE); + this.termPattern = compile(regex, Pattern.CASE_INSENSITIVE); this.validTerms = ImmutableSet.copyOf(validTerms); } From 46924eab35f7a8e49610365203668dfe12ae3abe Mon Sep 17 00:00:00 2001 From: Julian Hyde Date: Sat, 28 Feb 2026 17:54:37 -0800 Subject: [PATCH 5/6] Revise A man page-style specification follows. LINT:SORT DIRECTIVE NAME lint:sort - Specification-based sorting directive for maintaining alphabetically ordered code sections SYNOPSIS // lint: sort [until 'END_PATTERN'] [where 'FILTER_PATTERN'] [erase 'ERASE_PATTERN'] DESCRIPTION The lint:sort directive enforces alphabetical ordering of lines within a specified code section. It extracts sort keys from matching lines, optionally filters and transforms them, then validates alphabetical order. PARAMETERS until 'END_PATTERN' Optional. Regular expression marking the end of the sorted section. Supports '#' placeholder (parent indent, -2 spaces) and '##' (current line indent). where 'FILTER_PATTERN' Optional. Regular expression to select lines for sorting. Only matching lines are checked. Supports '#' and '##' placeholders. If you want to match a literal '#' (as in a bash comment), write '[#]'. erase 'ERASE_PATTERN' Optional. Regular expression to remove from lines before comparing. Useful for ignoring type annotations, modifiers, etc. PLACEHOLDERS # Parent indentation level (current indent minus 2 spaces) ## Current line's indentation level MULTI-LINE SPECIFICATIONS Long directives can span multiple lines by ending each line with '\'. The backslash and newline are removed during parsing. Example: // lint: sort until '#}' \ // where '##private static final' \ // erase 'Applicable[0-4]*' EXAMPLES Sort cases of a switch: switch (x) { // lint: sort until '#}' where '##case ' case A: // some code case B: // some code } Sort Maven dependencies: Sort constants, ignoring the type of those constants // lint: sort until '#}' \ // where '##private static final [^ ]+ [^ ]+ =' \ // erase '##private static final [^ ]+ ' VIOLATIONS Violations report: - File path and line number - The out-of-order line - The line it should precede NOTES - Sorting is case-sensitive - Empty lines and comments between sorted items are preserved - The directive itself is not included in the sorted section - Use specific patterns to avoid false matches (e.g., exact indentation instead of \\s* for nested structures) - Multi-line specs help with readability and avoid formatter issues --- core/src/test/java/org/apache/calcite/test/LintTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java b/core/src/test/java/org/apache/calcite/test/LintTest.java index 0b69f8ee7a4b..01f652cb10de 100644 --- a/core/src/test/java/org/apache/calcite/test/LintTest.java +++ b/core/src/test/java/org/apache/calcite/test/LintTest.java @@ -195,7 +195,7 @@ && isJava(line.filename()), .add(line -> line.state().sortConsumer != null, line -> requireNonNull(line.state().sortConsumer).accept(line)) - // Start sorting when a "// lint: sort until ..." directive is found. + // Start sorting when a "// lint: sort ..." directive is found. .add(line -> line.contains("// lint: sort") && !line.source().fileOpt() .filter(f -> f.getName().equals("LintTest.java")).isPresent(), From f6599e2b47fd3ce8896816136eafd62975fc6f39 Mon Sep 17 00:00:00 2001 From: Julian Hyde Date: Sat, 28 Feb 2026 23:22:30 -0800 Subject: [PATCH 6/6] Remove unused test Close apache/calcite#4813 --- .../org/apache/calcite/test/LintTest.java | 50 ------------------- 1 file changed, 50 deletions(-) diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java b/core/src/test/java/org/apache/calcite/test/LintTest.java index 01f652cb10de..ff012bfa16c0 100644 --- a/core/src/test/java/org/apache/calcite/test/LintTest.java +++ b/core/src/test/java/org/apache/calcite/test/LintTest.java @@ -29,13 +29,9 @@ import org.checkerframework.checker.nullness.qual.Nullable; import org.junit.jupiter.api.Test; -import java.io.BufferedReader; import java.io.File; -import java.io.IOException; import java.io.PrintWriter; import java.io.StringWriter; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.ArrayList; import java.util.Comparator; import java.util.HashSet; @@ -54,7 +50,6 @@ import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.startsWith; -import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assumptions.assumeTrue; import static java.lang.Integer.parseInt; @@ -68,7 +63,6 @@ class LintTest { private static final Pattern CALCITE_PATTERN = compile("^(\\[CALCITE-[0-9]{1,4}][ ]).*"); private static final Pattern PATTERN = compile("^ *(// )?"); - private static final Path ROOT_PATH = Paths.get(System.getProperty("gradle.rootDir")); private static final String TERMINOLOGY_ERROR_MSG = "Message contains '%s' word; use one of the following instead: %s"; @@ -548,50 +542,6 @@ private static void checkMessage(String subject, String body, } } - /** Ensures that the {@code .mailmap} file is sorted. */ - @Test void testMailmapFile() { - final File mailmapFile = ROOT_PATH.resolve(".mailmap").toFile(); - final List lines = new ArrayList<>(); - forEachLineIn(mailmapFile, line -> { - if (!line.startsWith("#")) { - lines.add(line); - } - }); - String line = firstOutOfOrder(lines, String.CASE_INSENSITIVE_ORDER); - if (line != null) { - fail("line '" + line + "' is out of order"); - } - } - - /** Performs an action for each line in a file. */ - private static void forEachLineIn(File file, Consumer consumer) { - try (BufferedReader r = Util.reader(file)) { - for (;;) { - String line = r.readLine(); - if (line == null) { - break; - } - consumer.accept(line); - } - } catch (IOException e) { - throw Util.throwAsRuntime(e); - } - } - - /** Returns the first element in a list that is out of order, or null if the - * list is sorted. */ - private static @Nullable E firstOutOfOrder(Iterable elements, - Comparator comparator) { - E previous = null; - for (E e : elements) { - if (previous != null && comparator.compare(previous, e) > 0) { - return e; - } - previous = e; - } - return null; - } - /** Warning that code is not as it should be. */ private static class Message { final Source source;