Fix and rename --incompatible_lexicographical_output to only affect -…

…-order_output=auto.

Previous implementation of --incompatible_use_lexicographical_unordered_output wrongly affects both --order_output=auto and --order_output=no. Renamed the flag for it to make more sense. Github issue for tracking this flag: bazelbuild#12757.

Additionally, using somepath as a top level function (e.g. query "somepath(a, b)") will not be affected by --incompatible_lexicographical_output and will continue to output in dependency order.

PiperOrigin-RevId: 353845699

src/main/java/com/google/devtools/build/lib/query2/engine/QueryExpression.java

@@ -106,4 +106,10 @@ public final String toTrunctatedString() {
   public static String truncate(String expr) {
     return Ascii.truncate(expr, MAX_QUERY_EXPRESSION_LOG_CHARS, "[truncated]");
   }
+
+  /** Checks if this QueryExpression has a SomePathFunction at its top level. */
+  public boolean isTopLevelSomePathFunction() {
+    return this instanceof FunctionExpression
+        && "somepath".equals(((FunctionExpression) this).getFunction().getName());
+  }
 }

src/main/java/com/google/devtools/build/lib/query2/query/output/QueryOptions.java

@@ -116,7 +116,7 @@ public enum OrderOutput {
   public OrderOutput orderOutput;
 
   @Option(
-      name = "incompatible_use_lexicographical_unordered_output",
+      name = "incompatible_lexicographical_output",
       defaultValue = "false",
       documentationCategory = OptionDocumentationCategory.QUERY,
       effectTags = {OptionEffectTag.TERMINAL_OUTPUT},
@@ -125,7 +125,7 @@ public enum OrderOutput {
         OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
       },
       help = "If this option is set, sorts --order_output=auto output in lexicographical order.")
-  public boolean useLexicographicalUnorderedOutput;
+  public boolean lexicographicalOutput;
 
   @Option(
       name = "graph:conditional_edges_limit",

src/main/java/com/google/devtools/build/lib/query2/query/output/QueryOutputUtils.java

@@ -34,13 +34,23 @@ public class QueryOutputUtils {
   // Utility class cannot be instantiated.
   private QueryOutputUtils() {}
 
-  public static boolean shouldStreamResults(QueryOptions queryOptions, OutputFormatter formatter) {
-    return (queryOptions.orderOutput == OrderOutput.NO
-            || (queryOptions.orderOutput == OrderOutput.AUTO
-                && queryOptions.useLexicographicalUnorderedOutput))
+  public static boolean lexicographicallySortOutput(
+      QueryOptions queryOptions, OutputFormatter formatter) {
+    return queryOptions.orderOutput == OrderOutput.AUTO
+        && queryOptions.lexicographicalOutput
         && formatter instanceof StreamedFormatter;
   }
 
+  public static boolean shouldStreamUnorderedOutput(
+      QueryOptions queryOptions, OutputFormatter formatter) {
+    return queryOptions.orderOutput == OrderOutput.NO && formatter instanceof StreamedFormatter;
+  }
+
+  public static boolean shouldStreamResults(QueryOptions queryOptions, OutputFormatter formatter) {
+    return shouldStreamUnorderedOutput(queryOptions, formatter)
+        || lexicographicallySortOutput(queryOptions, formatter);
+  }
+
   public static void output(
       QueryOptions queryOptions,
       QueryEvalResult result,

src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java

@@ -66,7 +66,6 @@
 import com.google.devtools.build.lib.query2.QueryEnvironmentFactory;
 import com.google.devtools.build.lib.query2.common.AbstractBlazeQueryEnvironment;
 import com.google.devtools.build.lib.query2.common.UniverseScope;
-import com.google.devtools.build.lib.query2.engine.FunctionExpression;
 import com.google.devtools.build.lib.query2.engine.QueryEnvironment.Setting;
 import com.google.devtools.build.lib.query2.engine.QueryEvalResult;
 import com.google.devtools.build.lib.query2.engine.QueryException;
@@ -384,7 +383,7 @@ private GenQueryResult doQuery(
       QueryExpression expr = QueryExpression.parse(query, queryEnvironment);
       formatter.verifyCompatible(queryEnvironment, expr);
       targets =
-          graphlessQuery && !hasTopLevelSomePathFunction(expr)
+          graphlessQuery && !expr.isTopLevelSomePathFunction()
               ? QueryUtil.newLexicographicallySortedTargetAggregator()
               : QueryUtil.newOrderedAggregateAllOutputFormatterCallback(queryEnvironment);
       queryResult = queryEnvironment.evaluateQuery(expr, targets);
@@ -427,12 +426,6 @@ private GenQueryResult doQuery(
     return outputStream.getResult();
   }
 
-  /** Checks if the {@code expr} has a SomePathFunction at its top level */
-  private static boolean hasTopLevelSomePathFunction(QueryExpression expr) {
-    return expr instanceof FunctionExpression
-        && "somepath".equals(((FunctionExpression) expr).getFunction().getName());
-  }
-
   @Immutable // assuming no other reference to result
   private static final class QueryResultAction extends AbstractFileWriteAction {
     private final GenQueryResult result;

src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java

@@ -106,6 +106,14 @@ protected Either<BlazeCommandResult, QueryEvalResult> doQuery(
 
     expr = queryEnv.transformParsedQuery(expr);
 
+    // This only applies to --order_output=auto. Instead of being written directly to the stream
+    // by the callback, this option aggregates the results in the lexicographically sorted
+    // aggregator first before using the StreamedFormatter to write it to stream later.
+    // An exception to this is when somepath is used at the top level of the query expression.
+    boolean lexicographicallySortOutput =
+        QueryOutputUtils.lexicographicallySortOutput(queryOptions, formatter)
+            && !expr.isTopLevelSomePathFunction();
+
     OutputStream out;
     if (formatter.canBeBuffered()) {
       // There is no particular reason for the 16384 constant here, except its a multiple of the
@@ -127,7 +135,7 @@ protected Either<BlazeCommandResult, QueryEvalResult> doQuery(
           queryOptions.aspectDeps.createResolver(env.getPackageManager(), env.getReporter()),
           hashFunction);
       streamedFormatter.setEventHandler(env.getReporter());
-      if (queryOptions.useLexicographicalUnorderedOutput) {
+      if (lexicographicallySortOutput) {
         callback = QueryUtil.newLexicographicallySortedTargetAggregator();
       } else {
         callback = streamedFormatter.createStreamCallback(out, queryOptions, queryEnv);
@@ -170,7 +178,7 @@ protected Either<BlazeCommandResult, QueryEvalResult> doQuery(
           out.flush();
         }
       }
-      if (!streamResults || queryOptions.useLexicographicalUnorderedOutput) {
+      if (!streamResults || lexicographicallySortOutput) {
         disableAnsiCharactersFiltering(env);
         try (SilentCloseable closeable = Profiler.instance().profile("QueryOutputUtils.output")) {
           Set<Target> targets =

src/main/java/com/google/devtools/build/lib/runtime/commands/QueryEnvironmentBasedCommand.java

@@ -153,8 +153,8 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
       return reportAndCreateFailureResult(
           env,
           String.format(
-              "--experimental_graphless_query requires --order_output=no and an --output"
-                  + " option that supports streaming; valid values are: %s",
+              "--experimental_graphless_query requires --order_output=no or --order_output=auto and"
+                  + " an --output option that supports streaming; valid values are: %s",
               OutputFormatters.streamingFormatterNames(formatters)),
           Query.Code.GRAPHLESS_PREREQ_UNMET);
     }

src/test/shell/integration/bazel_query_test.sh

@@ -712,6 +712,7 @@ EOF
 }
 
 function test_graphless_query_matches_graphless_genquery_output() {
+  rm -rf foo
   mkdir -p foo
   cat > foo/BUILD <<EOF
 sh_library(name = "b", deps = [":c"])
@@ -734,21 +735,71 @@ EOF
   bazel build --experimental_genquery_use_graphless_query \
       //foo:q || fail "Expected success"
 
-  # The --incompatible_use_lexicographical_unordered_output flag is used to
+  # The --incompatible_lexicographical_output flag is used to
   # switch order_output=auto to use graphless query and output in
   # lexicographical order.
-  bazel query --incompatible_use_lexicographical_unordered_output \
+  bazel query --incompatible_lexicographical_output \
       "deps(//foo:b)" | grep foo >& foo/query_output || fail "Expected success"
 
-  # The outputs of graphless query and graphless genquery should be the same.
-  assert_equals "$(cat bazel-bin/foo/q)" "$(cat foo/query_output)"
-
-  # The outputs of both graphless query and graphless genquery should be in
-  # lexicographical order (comparing one should be sufficient).
+  # The outputs of graphless query and graphless genquery should be the same and
+  # should both be in lexicographical order.
+  assert_equals \
+      "$(cat foo/expected_lexicographical_result)" "$(cat foo/query_output)"
   assert_equals \
       "$(cat foo/expected_lexicographical_result)" "$(cat bazel-bin/foo/q)"
 }
 
+function test_lexicographical_output_does_not_affect_order_output_no() {
+  rm -rf foo
+  mkdir -p foo
+  cat > foo/BUILD <<EOF
+sh_library(name = "b", deps = [":c"])
+sh_library(name = "c", deps = [":a"])
+sh_library(name = "a")
+genquery(
+    name = "q",
+    expression = "deps(//foo:b)",
+    scope = ["//foo:b"],
+)
+EOF
+
+  bazel query --order_output=no \
+      "deps(//foo:b)" | grep foo >& foo/query_output \
+      || fail "Expected success"
+  bazel query --order_output=no \
+      --incompatible_lexicographical_output \
+      "deps(//foo:b)" | grep foo >& foo/lex_query_output \
+      || fail "Expected success"
+
+  # The --incompatible_lexicographical_output flag should not affect query
+  # order_output=no. Note that there is a chance it may output in
+  # lexicographical order since it is unordered.
+  assert_equals \
+      "$(cat foo/query_output)" "$(cat foo/lex_query_output)"
+}
+
+function test_lexicographical_output_does_not_affect_somepath() {
+  rm -rf foo
+  mkdir -p foo
+  cat > foo/BUILD <<EOF
+sh_library(name = "b", deps = [":c"])
+sh_library(name = "c", deps = [":a"])
+sh_library(name = "a")
+EOF
+
+  cat > foo/expected_deps_output <<EOF
+//foo:b
+//foo:c
+//foo:a
+EOF
+
+  bazel query --incompatible_lexicographical_output \
+      "somepath(//foo:b, //foo:a)" | grep foo >& foo/query_output
+
+  assert_equals \
+      "$(cat foo/expected_deps_output)" "$(cat foo/query_output)"
+}
+
 # Regression test for https://github.com/bazelbuild/bazel/issues/8582.
 function test_rbuildfiles_can_handle_non_loading_phase_edges() {
   mkdir -p foo