Don't create batching callback in SkyQueryEnvironment.getTargetsMatchingPattern

For SkyQueryEnvironment, the labelFilter parameter was a predicate
that always returned true, so the value of strictScope (read only
when the predicate returned false) didn't matter. In addition to
batching, the responsibility of the callback constructed in
getTargetsMatchingPattern was to apply the filter. Because the filter
always returned true, and because there's already a batching callback
constructed in evaluateQuery, the getTargetsMatchingPattern callback
wasn't doing any useful work.

--
MOS_MIGRATED_REVID=125502498

2 files changed, 34 insertions, 61 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java b/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java
index 7ecdd5a0ee..c2f8f80592 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.query2;
 import com.google.common.base.Predicate;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.PackageProvider;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
@@ -43,10 +44,17 @@ public class QueryEnvironmentFactory {
       EventHandler eventHandler, Set<Setting> settings, Iterable<QueryFunction> functions,
       @Nullable PathPackageLocator packagePath) {
     Preconditions.checkNotNull(universeScope);
-    if (canUseSkyQuery(orderedResults, universeScope, packagePath)) {
-      return new SkyQueryEnvironment(keepGoing, strictScope, loadingPhaseThreads, labelFilter,
-          eventHandler, settings, functions, targetPatternEvaluator.getOffset(), graphFactory,
-          universeScope, packagePath);
+    if (canUseSkyQuery(orderedResults, universeScope, packagePath, strictScope, labelFilter)) {
+      return new SkyQueryEnvironment(
+          keepGoing,
+          loadingPhaseThreads,
+          eventHandler,
+          settings,
+          functions,
+          targetPatternEvaluator.getOffset(),
+          graphFactory,
+          universeScope,
+          packagePath);
     } else {
       return new BlazeQueryEnvironment(transitivePackageLoader, packageProvider,
           targetPatternEvaluator, keepGoing, strictScope, loadingPhaseThreads, labelFilter,
@@ -54,9 +62,17 @@ public class QueryEnvironmentFactory {
     }
   }
 
-  protected static boolean canUseSkyQuery(boolean orderedResults, List<String> universeScope,
-      @Nullable PathPackageLocator packagePath) {
-    return !orderedResults && !universeScope.isEmpty() && packagePath != null;
+  protected static boolean canUseSkyQuery(
+      boolean orderedResults,
+      List<String> universeScope,
+      @Nullable PathPackageLocator packagePath,
+      boolean strictScope,
+      Predicate<Label> labelFilter) {
+    return !orderedResults
+        && !universeScope.isEmpty()
+        && packagePath != null
+        && strictScope
+        && labelFilter == Rule.ALL_LABELS;
   }
 }
 
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 525bcb9e1e..2accdcea63 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -72,7 +72,6 @@ import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.skyframe.TransitiveTraversalValue;
-import com.google.devtools.build.lib.util.BatchCallback;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -112,7 +111,6 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
   private static final int BATCH_CALLBACK_SIZE = 10000;
 
   protected WalkableGraph graph;
-
   private Supplier<ImmutableSet<PathFragment>> blacklistPatternsSupplier;
 
   private final BlazeTargetAccessor accessor = new BlazeTargetAccessor(this);
@@ -132,6 +130,7 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
       return target.getLabel();
     }
   };
+
   private final ListeningExecutorService threadPool =
       MoreExecutors.listeningDecorator(
           Executors.newFixedThreadPool(
@@ -154,14 +153,18 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
     }
   }
 
-  public SkyQueryEnvironment(boolean keepGoing, boolean strictScope, int loadingPhaseThreads,
-      Predicate<Label> labelFilter,
+  public SkyQueryEnvironment(
+      boolean keepGoing,
+      int loadingPhaseThreads,
       EventHandler eventHandler,
       Set<Setting> settings,
       Iterable<QueryFunction> extraFunctions, String parserPrefix,
       WalkableGraphFactory graphFactory,
       List<String> universeScope, PathPackageLocator pkgPath) {
-    super(keepGoing, strictScope, labelFilter,
+    super(
+        keepGoing,
+        /*strictScope=*/ true,
+        /*labelFilter=*/ Rule.ALL_LABELS,
         eventHandler,
         settings,
         extraFunctions);
@@ -458,39 +461,6 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
     return uniquifier();
   }
 
-  /**
-   * Wraps a {@link Callback<Target>} with three additional filtering mechanisms. First, it
-   * validates the scope of the targets it's given before it passes them to the delegate Callback.
-   * Second, it removes {@link Target}s not in the graph (outside the universe scope). Third, it
-   * wraps the Callback in a {@link BatchStreamedCallback}, which aggregates results into batches of
-   * {@link #BATCH_CALLBACK_SIZE} and also deduplicates elements.
-   */
-  private class FilteringBatchingUniquifyingCallback
-      implements BatchCallback<Target, QueryException> {
-    private final BatchStreamedCallback batchStreamedCallback;
-
-    private FilteringBatchingUniquifyingCallback(Callback<Target> callback) {
-      this.batchStreamedCallback =
-          new BatchStreamedCallback(callback, BATCH_CALLBACK_SIZE, createUniquifier());
-    }
-
-    @Override
-    public void process(Iterable<Target> partialResult)
-        throws QueryException, InterruptedException {
-      Set<Target> targets = CompactHashSet.create();
-      for (Target target : partialResult) {
-        if (validateScope(target.getLabel(), strictScope)) {
-          targets.add(target);
-        }
-      }
-      batchStreamedCallback.process(targets);
-    }
-
-    private void processLastPending() throws QueryException, InterruptedException {
-      batchStreamedCallback.processLastPending();
-    }
-  }
-
   @Override
   public void getTargetsMatchingPattern(
       QueryExpression owner, String pattern, Callback<Target> callback) throws QueryException {
@@ -504,10 +474,7 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
       TargetPattern parsedPattern = targetPatternKey.getParsedPattern();
       ImmutableSet<PathFragment> subdirectoriesToExclude =
           targetPatternKey.getAllSubdirectoriesToExclude(blacklistPatternsSupplier);
-      FilteringBatchingUniquifyingCallback wrapper =
-          new FilteringBatchingUniquifyingCallback(callback);
-      parsedPattern.eval(resolver, subdirectoriesToExclude, wrapper, QueryException.class);
-      wrapper.processLastPending();
+      parsedPattern.eval(resolver, subdirectoriesToExclude, callback, QueryException.class);
     } catch (TargetParsingException e) {
       reportBuildFileError(owner, e.getMessage());
     } catch (InterruptedException e) {
@@ -582,15 +549,9 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
     return accessor;
   }
 
-  private SkyKey getPackageKeyAndValidateLabel(Label label) throws QueryException {
-    // Can't use strictScope here because we are expecting a target back.
-    validateScope(label, true);
-    return PackageValue.key(label.getPackageIdentifier());
-  }
-
   @Override
   public Target getTarget(Label label) throws TargetNotFoundException, QueryException {
-    SkyKey packageKey = getPackageKeyAndValidateLabel(label);
+    SkyKey packageKey = PackageValue.key(label.getPackageIdentifier());
     if (!graph.exists(packageKey)) {
       throw new QueryException(packageKey + " does not exist in graph");
     }
@@ -683,11 +644,7 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
       if (label == null) {
         continue;
       }
-      try {
-        packageKeyToTargetKeyMap.put(getPackageKeyAndValidateLabel(label), key);
-      } catch (QueryException e) {
-        // Skip disallowed labels.
-      }
+      packageKeyToTargetKeyMap.put(PackageValue.key(label.getPackageIdentifier()), key);
     }
     ImmutableMap.Builder<SkyKey, Target> result = ImmutableMap.builder();
     Map<SkyKey, SkyValue> packageMap = graph.getSuccessfulValues(packageKeyToTargetKeyMap.keySet());