Skip to content

Commit

Permalink
Fix some naming related to AggregatePullUpLookupRule. (#15677)
Browse files Browse the repository at this point in the history
It was called "split" rather than "pull up" in some places. This patch
standardizes on "pull up".
  • Loading branch information
gianm committed Jan 12, 2024
1 parent 0457c71 commit 866fe1c
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,14 @@ public DruidExpression toDruidExpression(
false,
replaceMissingValueWith,
null,
// If SplitLookupRule is disabled, then enable optimization at the extractionFn level, since a
// If LOOKUP pull-up is disabled, then enable optimization at the extractionFn level, since a
// similar optimization may be done by the native query toolchests. We'd like to ensure that if
// people upgrade to a version where this rule was added, and then disable the rule due to some
// problem with it, they still get any optimization that the native layer was able to do.
//
// Note that we don't check plannerContext.isReverseLookup(), because the native layer doesn't
// optimize filters on RegisteredLookupExtractionFn anyway.
!plannerContext.isSplitLookup()
!plannerContext.isPullUpLookup()
)
);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ private Program buildReductionProgram(final PlannerContext plannerContext, final
builder.addRuleInstance(new FilterDecomposeConcatRule());

// Include rule to split injective LOOKUP across a GROUP BY.
if (plannerContext.isSplitLookup()) {
if (plannerContext.isPullUpLookup()) {
builder.addRuleInstance(new AggregatePullUpLookupRule(plannerContext));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public class PlannerContext
public static final boolean DEFAULT_SQL_USE_BOUNDS_AND_SELECTORS = NullHandling.replaceWithDefault();

/**
* Context key for {@link PlannerContext#isSplitLookup()}.
* Context key for {@link PlannerContext#isPullUpLookup()}.
*/
public static final String CTX_SQL_PULL_UP_LOOKUP = "sqlPullUpLookup";
public static final boolean DEFAULT_SQL_PULL_UP_LOOKUP = true;
Expand All @@ -120,7 +120,7 @@ public class PlannerContext
private final String sqlQueryId;
private final boolean stringifyArrays;
private final boolean useBoundsAndSelectors;
private final boolean splitLookup;
private final boolean pullUpLookup;
private final boolean reverseLookup;
private final CopyOnWriteArrayList<String> nativeQueryIds = new CopyOnWriteArrayList<>();
private final PlannerHook hook;
Expand Down Expand Up @@ -148,7 +148,7 @@ private PlannerContext(
final DateTime localNow,
final boolean stringifyArrays,
final boolean useBoundsAndSelectors,
final boolean splitLookup,
final boolean pullUpLookup,
final boolean reverseLookup,
final SqlEngine engine,
final Map<String, Object> queryContext,
Expand All @@ -164,7 +164,7 @@ private PlannerContext(
this.localNow = Preconditions.checkNotNull(localNow, "localNow");
this.stringifyArrays = stringifyArrays;
this.useBoundsAndSelectors = useBoundsAndSelectors;
this.splitLookup = splitLookup;
this.pullUpLookup = pullUpLookup;
this.reverseLookup = reverseLookup;
this.hook = hook == null ? NoOpPlannerHook.INSTANCE : hook;

Expand All @@ -188,14 +188,14 @@ public static PlannerContext create(
final DateTimeZone timeZone;
final boolean stringifyArrays;
final boolean useBoundsAndSelectors;
final boolean splitLookup;
final boolean pullUpLookup;
final boolean reverseLookup;

final Object stringifyParam = queryContext.get(QueryContexts.CTX_SQL_STRINGIFY_ARRAYS);
final Object tsParam = queryContext.get(CTX_SQL_CURRENT_TIMESTAMP);
final Object tzParam = queryContext.get(CTX_SQL_TIME_ZONE);
final Object useBoundsAndSelectorsParam = queryContext.get(CTX_SQL_USE_BOUNDS_AND_SELECTORS);
final Object splitLookupParam = queryContext.get(CTX_SQL_PULL_UP_LOOKUP);
final Object pullUpLookupParam = queryContext.get(CTX_SQL_PULL_UP_LOOKUP);
final Object reverseLookupParam = queryContext.get(CTX_SQL_REVERSE_LOOKUP);

if (tsParam != null) {
Expand All @@ -222,10 +222,10 @@ public static PlannerContext create(
useBoundsAndSelectors = DEFAULT_SQL_USE_BOUNDS_AND_SELECTORS;
}

if (splitLookupParam != null) {
splitLookup = Numbers.parseBoolean(splitLookupParam);
if (pullUpLookupParam != null) {
pullUpLookup = Numbers.parseBoolean(pullUpLookupParam);
} else {
splitLookup = DEFAULT_SQL_PULL_UP_LOOKUP;
pullUpLookup = DEFAULT_SQL_PULL_UP_LOOKUP;
}

if (reverseLookupParam != null) {
Expand All @@ -241,7 +241,7 @@ public static PlannerContext create(
utcNow.withZone(timeZone),
stringifyArrays,
useBoundsAndSelectors,
splitLookup,
pullUpLookup,
reverseLookup,
engine,
queryContext,
Expand Down Expand Up @@ -378,13 +378,12 @@ public boolean isUseBoundsAndSelectors()
}

/**
* Whether we should use {@link AggregatePullUpLookupRule} to split LOOKUP functions on injective lookups when they
* are dimensions in aggregations, and whether we should set the "optimize" flag on
* {@link RegisteredLookupExtractionFn}.
* Whether we should use {@link AggregatePullUpLookupRule} to pull LOOKUP functions on injective lookups up above
* a GROUP BY.
*/
public boolean isSplitLookup()
public boolean isPullUpLookup()
{
return splitLookup;
return pullUpLookup;
}

/**
Expand Down

0 comments on commit 866fe1c

Please sign in to comment.