Skip to content

Fix constant folding for SpecialFormExpression and LambdaDefinitionExpression in sidecar enabled clusters #26125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pdabre12 merged 1 commit into from
Sep 26, 2025
Merged

Fix constant folding for SpecialFormExpression and LambdaDefinitionExpression in sidecar enabled clusters #26125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions presto-main-base/src/main/java/com/facebook/presto/sql/planner/QueryPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.facebook.presto.sql.planner;

import com.facebook.presto.Session;
import com.facebook.presto.common.CatalogSchemaName;
import com.facebook.presto.common.QualifiedObjectName;
import com.facebook.presto.common.block.SortOrder;
import com.facebook.presto.common.predicate.TupleDomain;
Expand Down Expand Up @@ -114,7 +115,6 @@
import static com.facebook.presto.sql.NodeUtils.getSortItemsFromOrderBy;
import static com.facebook.presto.sql.analyzer.ExpressionAnalyzer.isNumericType;
import static com.facebook.presto.sql.analyzer.ExpressionTreeUtils.getSourceLocation;
import static com.facebook.presto.sql.analyzer.TypeSignatureProvider.fromTypes;
import static com.facebook.presto.sql.planner.PlannerUtils.newVariable;
import static com.facebook.presto.sql.planner.PlannerUtils.toOrderingScheme;
import static com.facebook.presto.sql.planner.PlannerUtils.toSortOrder;
Expand Down Expand Up @@ -1112,7 +1112,7 @@ private FrameBoundPlanAndSymbols planFrameBound(PlanBuilder subPlan, PlanAndMapp
// First, append filter to validate offset values. They mustn't be negative or null.
VariableReferenceExpression offsetSymbol = coercions.get(frameOffset.get());
Expression zeroOffset = zeroOfType(TypeProvider.viewOf(variableAllocator.getVariables()).get(offsetSymbol));
FunctionHandle fail = metadata.getFunctionAndTypeManager().resolveFunction(Optional.empty(), Optional.empty(), QualifiedObjectName.valueOf("presto.default.fail"), fromTypes(VARCHAR));
CatalogSchemaName defaultNamespace = metadata.getFunctionAndTypeManager().getDefaultNamespace();
Expression predicate = new IfExpression(
new ComparisonExpression(
GREATER_THAN_OR_EQUAL,
Expand All @@ -1121,7 +1121,7 @@ private FrameBoundPlanAndSymbols planFrameBound(PlanBuilder subPlan, PlanAndMapp
TRUE_LITERAL,
new Cast(
new FunctionCall(
QualifiedName.of("presto", "default", "fail"),
QualifiedName.of(defaultNamespace.getCatalogName(), defaultNamespace.getSchemaName(), "fail"),
ImmutableList.of(new Cast(new StringLiteral("Window frame offset value must not be negative or null"), VARCHAR.getTypeSignature().toString()))),
BOOLEAN.getTypeSignature().toString()));
subPlan = subPlan.withNewRoot(new FilterNode(
Expand Down
2 changes: 1 addition & 1 deletion presto-main-base/src/main/java/com/facebook/presto/sql/planner/RowExpressionInterpreter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ public Object visitCall(CallExpression node, Object context)
(!functionMetadata.isDeterministic() ||
hasUnresolvedValue(argumentValues) ||
isDynamicFilter(node) ||
resolution.isFailFunction(functionHandle))) {
resolution.isJavaBuiltInFailFunction(functionHandle))) {
return call(node.getDisplayName(), functionHandle, node.getType(), toRowExpressions(argumentValues, node.getArguments()));
}

Expand Down
7 changes: 5 additions & 2 deletions presto-main-base/src/main/java/com/facebook/presto/sql/relational/FunctionResolution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,12 @@ public boolean isTryFunction(FunctionHandle functionHandle)
return functionAndTypeResolver.getFunctionMetadata(functionHandle).getName().getObjectName().equals("$internal$try");
}

public boolean isFailFunction(FunctionHandle functionHandle)
public boolean isJavaBuiltInFailFunction(FunctionHandle functionHandle)
{
return functionAndTypeResolver.getFunctionMetadata(functionHandle).getName().equals(functionAndTypeResolver.qualifyObjectName(QualifiedName.of("fail")));
// todo: Revert this hack once constant folding support lands in C++.
Copy link
Copy Markdown
Contributor

@rschlussel rschlussel Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems a bit sketchy -- why only the fail function and not the other functions in this class? Seems like it's that you happen to know this function is only used in the rowExpressionInterpreter.

I would rather add a new method to this class isJavaBuiltinFailFunction or something so that the assumption that something uses the java built in namespace is isolated to the RowExpressionInterpreter.

Copy link
Copy Markdown
Contributor

@aditi-pandit aditi-pandit Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschlussel : That's fair. Changed the function name to isJavaBuiltinFailFunction for more clarity. PTAL.

// For now, we always use the presto.default.fail function even when the default namespace is switched.
// This is done for consistency since the BuiltInNamespaceRewriter rewrites the functionHandles to presto.default functionHandles.
return functionAndTypeResolver.getFunctionMetadata(functionHandle).getName().equals(QualifiedObjectName.valueOf(JAVA_BUILTIN_NAMESPACE, "fail"));
Comment thread
aditi-pandit marked this conversation as resolved.
}

@Override
Expand Down
68 changes: 20 additions & 48 deletions ...to-main-base/src/main/java/com/facebook/presto/sql/relational/RowExpressionOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

import com.facebook.presto.common.CatalogSchemaName;
import com.facebook.presto.common.QualifiedObjectName;
import com.facebook.presto.expressions.RowExpressionRewriter;
import com.facebook.presto.expressions.RowExpressionTreeRewriter;
import com.facebook.presto.metadata.FunctionAndTypeManager;
import com.facebook.presto.metadata.Metadata;
import com.facebook.presto.spi.ConnectorSession;
Expand All @@ -24,7 +26,6 @@
import com.facebook.presto.spi.relation.CallExpression;
import com.facebook.presto.spi.relation.ExpressionOptimizer;
import com.facebook.presto.spi.relation.RowExpression;
import com.facebook.presto.spi.relation.RowExpressionVisitor;
import com.facebook.presto.spi.relation.VariableReferenceExpression;
import com.facebook.presto.sql.analyzer.TypeSignatureProvider;
import com.facebook.presto.sql.planner.RowExpressionInterpreter;
Expand Down Expand Up @@ -106,33 +107,23 @@ public RowExpression convertToInterpreterNamespace(RowExpression expression)
// No need to replace built-in namespaces if the default namespace is already the Java built-in namespace
return expression;
}
return expression.accept(new ReplaceBuiltInNamespaces(), null);
return RowExpressionTreeRewriter.rewriteWith(new ReplaceBuiltInNamespaces(), expression, null);
}

public RowExpression restoreOriginalNamespaces(RowExpression expression)
{
if (defaultToOriginalFunctionHandles.isEmpty()) {
return expression;
}
return expression.accept(new ReplaceOriginalNamespaces(), null);
return RowExpressionTreeRewriter.rewriteWith(new ReplaceOriginalNamespaces(), expression, null);
}

private class ReplaceBuiltInNamespaces
implements RowExpressionVisitor<RowExpression, Void>
extends RowExpressionRewriter<Void>
{
@Override
public RowExpression visitExpression(RowExpression expression, Void context)
public RowExpression rewriteCall(CallExpression call, Void context, RowExpressionTreeRewriter<Void> treeRewriter)
{
return expression;
}

@Override
public RowExpression visitCall(CallExpression call, Void context)
{
ImmutableList<RowExpression> rewrittenArgs = call.getArguments().stream()
.map(arg -> arg.accept(this, context))
.collect(toImmutableList());

FunctionHandle functionHandle = call.getFunctionHandle();
FunctionMetadata functionMetadata = functionAndTypeManager.getFunctionMetadata(functionHandle);
if (!functionMetadata.getImplementationType().canBeEvaluatedInCoordinator()) {
Expand All @@ -148,13 +139,8 @@ public RowExpression visitCall(CallExpression call, Void context)
}
catch (PrestoException e) {
if (e.getErrorCode().equals(FUNCTION_NOT_FOUND.toErrorCode())) {
// If the function is not found in the Java built-in namespace, return with rewritten children.
return new CallExpression(
call.getSourceLocation(),
call.getDisplayName(),
call.getFunctionHandle(),
call.getType(),
rewrittenArgs);
// If the function is not found in the Java built-in namespace, let default rewriter handle it
return null;
}
throw e; // Rethrow other exceptions
}
Expand All @@ -163,6 +149,9 @@ public RowExpression visitCall(CallExpression call, Void context)
format("FunctionHandle %s in the Java built-in namespace (%s) is not eligible to be evaluated in the coordinator", javaNamespaceFunctionHandle, JAVA_BUILTIN_NAMESPACE));

defaultToOriginalFunctionHandles.put(javaNamespaceFunctionHandle, functionHandle);
ImmutableList<RowExpression> rewrittenArgs = call.getArguments().stream()
.map(arg -> treeRewriter.rewrite(arg, context))
.collect(toImmutableList());
return new CallExpression(
call.getSourceLocation(),
call.getDisplayName(),
Expand All @@ -171,48 +160,31 @@ public RowExpression visitCall(CallExpression call, Void context)
rewrittenArgs);
}

// If no rewrite needed, still return with rewritten children
return new CallExpression(
call.getSourceLocation(),
call.getDisplayName(),
functionHandle,
call.getType(),
rewrittenArgs);
// Return null to let the default rewriter handle it (which will rewrite children automatically)
return null;
}
}

private class ReplaceOriginalNamespaces
implements RowExpressionVisitor<RowExpression, Void>
extends RowExpressionRewriter<Void>
{
@Override
public RowExpression visitExpression(RowExpression expression, Void context)
public RowExpression rewriteCall(CallExpression call, Void context, RowExpressionTreeRewriter<Void> treeRewriter)
{
return expression;
}

@Override
public RowExpression visitCall(CallExpression call, Void context)
{
ImmutableList<RowExpression> rewrittenArgs = call.getArguments().stream()
.map(arg -> arg.accept(this, context))
.collect(toImmutableList());

if (defaultToOriginalFunctionHandles.containsKey(call.getFunctionHandle())) {
FunctionHandle originalFunctionHandle = defaultToOriginalFunctionHandles.get(call.getFunctionHandle());
ImmutableList<RowExpression> rewrittenArgs = call.getArguments().stream()
.map(arg -> treeRewriter.rewrite(arg, context))
.collect(toImmutableList());
return new CallExpression(
call.getSourceLocation(),
call.getDisplayName(),
originalFunctionHandle,
call.getType(),
rewrittenArgs);
}
// Nothing to restore, just rebuild with rewritten children
return new CallExpression(
call.getSourceLocation(),
call.getDisplayName(),
call.getFunctionHandle(),
call.getType(),
rewrittenArgs);
// Return null to let the default rewriter handle it (which will rewrite children automatically)
return null;
}
}
}
Expand Down
Loading
Loading