Skip to content

[native] Handle NULLIF without the need for specialForm #19791

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
vermapratyush merged 2 commits into from
Jun 29, 2023
Merged

[native] Handle NULLIF without the need for specialForm #19791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e68b49
[refactor] Pass functionAndTypeManager to SqlToRowExpressionTranslator
vermapratyush Jun 26, 2023
565bb22
[native] Handle NULLIF without the need for specialForm
vermapratyush Jun 29, 2023
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
2 changes: 1 addition & 1 deletion presto-druid/src/test/java/com/facebook/presto/druid/TestDruidQueryBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ protected RowExpression toRowExpression(Expression expression, Session session)
expression,
ImmutableMap.of(),
WarningCollector.NOOP);
return SqlToRowExpressionTranslator.translate(expression, expressionTypes, ImmutableMap.of(), functionAndTypeManager.getFunctionAndTypeResolver(), session);
return SqlToRowExpressionTranslator.translate(expression, expressionTypes, ImmutableMap.of(), functionAndTypeManager, session);
}

protected LimitNode limit(PlanBuilder pb, long count, PlanNode source)
Expand Down
2 changes: 1 addition & 1 deletion ...o-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ private RowExpression convertToRowExpression(Expression expression, Scope scope)
coercedMaybe,
coercedExpressionAnalysis.getExpressionTypes(),
ImmutableMap.of(),
metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver(),
metadata.getFunctionAndTypeManager(),
session);
}

Expand Down
8 changes: 2 additions & 6 deletions presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1495,7 +1495,7 @@ private MaterializedViewStatus getMaterializedViewStatus(QualifiedObjectName mat
viewQueryWhereClause,
analysis.getTypes(),
ImmutableMap.of(),
functionAndTypeResolver,
metadata.getFunctionAndTypeManager(),
session);

TupleDomain<String> viewQueryDomain = MaterializedViewUtils.getDomainFromFilter(session, domainTranslator, rowExpression);
Expand Down Expand Up @@ -2099,11 +2099,7 @@ private List<FunctionCall> analyzeWindowFunctions(QuerySpecification node, List<
}

Window window = windowFunction.getWindow().get();
if (window.getOrderBy().filter(
orderBy -> orderBy.getSortItems()
.stream()
.anyMatch(item -> item.getSortKey() instanceof Literal))
.isPresent()) {
if (window.getOrderBy().filter(orderBy -> orderBy.getSortItems().stream().anyMatch(item -> item.getSortKey() instanceof Literal)).isPresent()) {
Comment thread
vermapratyush marked this conversation as resolved.
if (isAllowWindowOrderByLiterals(session)) {
warningCollector.add(
new PrestoWarning(
Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/main/java/com/facebook/presto/sql/gen/RowExpressionCompiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public BytecodeNode visitCall(CallExpression call, Context context)
RowExpression function = getSqlFunctionRowExpression(
functionMetadata,
functionImplementation,
metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver(),
metadata.getFunctionAndTypeManager(),
sqlFunctionProperties,
sessionFunctions,
call.getArguments());
Expand Down
10 changes: 5 additions & 5 deletions presto-main/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 @@ -282,7 +282,7 @@ else if (implementationType.equals(JAVA)) {
RowExpression function = getSqlFunctionRowExpression(
functionMetadata,
functionImplementation,
metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver(),
metadata.getFunctionAndTypeManager(),
session.getSqlFunctionProperties(),
session.getSessionFunctions(),
node.getArguments());
Expand Down Expand Up @@ -582,10 +582,10 @@ else if (!found && result) {

if (hasUnresolvedValue) {
List<RowExpression> simplifiedExpressionValues = Stream.concat(
Stream.concat(
Stream.of(toRowExpression(target, node.getArguments().get(0))),
unresolvedValues.stream().filter(determinismEvaluator::isDeterministic).distinct()),
unresolvedValues.stream().filter((expression -> !determinismEvaluator.isDeterministic(expression))))
Stream.concat(
Stream.of(toRowExpression(target, node.getArguments().get(0))),
unresolvedValues.stream().filter(determinismEvaluator::isDeterministic).distinct()),
unresolvedValues.stream().filter((expression -> !determinismEvaluator.isDeterministic(expression))))
.collect(toImmutableList());
return new SpecialFormExpression(IN, node.getType(), simplifiedExpressionValues);
}
Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/main/java/com/facebook/presto/sql/planner/TranslateExpressionsUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public static RowExpression toRowExpression(Expression expression, Metadata meta

public static RowExpression toRowExpression(Expression expression, Metadata metadata, Session session, Map<NodeRef<Expression>, Type> types, SqlToRowExpressionTranslator.Context context)
{
return SqlToRowExpressionTranslator.translate(expression, types, ImmutableMap.of(), metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver(), session, context);
return SqlToRowExpressionTranslator.translate(expression, types, ImmutableMap.of(), metadata.getFunctionAndTypeManager(), session, context);
}

public static Map<NodeRef<Expression>, Type> analyzeCallExpressionTypes(
Expand Down
2 changes: 1 addition & 1 deletion ...main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineSqlFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public RowExpression rewriteCall(CallExpression expression, Void context, RowExp
return getSqlFunctionRowExpression(
functionMetadata,
(SqlInvokedScalarFunctionImplementation) metadata.getFunctionAndTypeManager().getScalarFunctionImplementation(functionHandle),
metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver(),
metadata.getFunctionAndTypeManager(),
session.getSqlFunctionProperties(),
session.getSessionFunctions(),
rewrittenArguments);
Expand Down
2 changes: 1 addition & 1 deletion ...in/src/main/java/com/facebook/presto/sql/planner/optimizations/ExpressionEquivalence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private RowExpression toRowExpression(Session session, Expression expression, Ma
WarningCollector.NOOP);

// convert to row expression
return translate(expression, expressionTypes, variableInput, metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver(), session);
return translate(expression, expressionTypes, variableInput, metadata.getFunctionAndTypeManager(), session);
}

private static class CanonicalizationVisitor
Expand Down
14 changes: 9 additions & 5 deletions presto-main/src/main/java/com/facebook/presto/sql/relational/SqlFunctionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.facebook.presto.common.type.Type;
import com.facebook.presto.expressions.RowExpressionRewriter;
import com.facebook.presto.expressions.RowExpressionTreeRewriter;
import com.facebook.presto.metadata.FunctionAndTypeManager;
import com.facebook.presto.spi.VariableAllocator;
import com.facebook.presto.spi.function.FunctionMetadata;
import com.facebook.presto.spi.function.SqlFunctionId;
Expand Down Expand Up @@ -82,31 +83,34 @@ public static Expression getSqlFunctionExpression(
public static RowExpression getSqlFunctionRowExpression(
FunctionMetadata functionMetadata,
SqlInvokedScalarFunctionImplementation implementation,
FunctionAndTypeResolver functionAndTypeResolver,
FunctionAndTypeManager functionAndTypeManager,
SqlFunctionProperties sqlFunctionProperties,
Map<SqlFunctionId, SqlInvokedFunction> sessionFunctions,
List<RowExpression> arguments)
{
VariableAllocator variableAllocator = new VariableAllocator();
Map<String, VariableReferenceExpression> argumentVariables = allocateFunctionArgumentVariables(functionMetadata, functionAndTypeResolver, variableAllocator);
Expression expression = getSqlFunctionImplementationExpression(functionMetadata, implementation, functionAndTypeResolver, variableAllocator, sqlFunctionProperties, argumentVariables);
Map<String, VariableReferenceExpression> argumentVariables = allocateFunctionArgumentVariables(functionMetadata, functionAndTypeManager.getFunctionAndTypeResolver(), variableAllocator);
Expression expression = getSqlFunctionImplementationExpression(functionMetadata, implementation, functionAndTypeManager.getFunctionAndTypeResolver(), variableAllocator, sqlFunctionProperties, argumentVariables);

// Translate to row expression
return SqlFunctionArgumentBinder.bindFunctionArguments(
SqlToRowExpressionTranslator.translate(
expression,
analyzeSqlFunctionExpression(
functionAndTypeResolver,
functionAndTypeManager.getFunctionAndTypeResolver(),
sqlFunctionProperties,
expression,
argumentVariables.values().stream()
.collect(toImmutableMap(VariableReferenceExpression::getName, VariableReferenceExpression::getType))).getExpressionTypes(),
ImmutableMap.of(),
functionAndTypeResolver,
functionAndTypeManager,
Optional.empty(),
Optional.empty(),
sqlFunctionProperties,
sessionFunctions,
// TODO: use session to determine if this is a native query
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Jun 29, 2023

Choose a reason for hiding this comment

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

Why can't we address this TODO now?

Copy link
Copy Markdown
Member Author

@vermapratyush vermapratyush Jun 29, 2023

Choose a reason for hiding this comment

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

Detailed conversation in this thread: https://github.com/prestodb/presto/pull/19791/files/189afe0a571d7ee128f242fd803a6098d967861b#r1243767843

In short,

The SqlFunctionUtils, which are used in the sql function code path does not currently properly support native, since there is no access to the session there, only connector session.

Copy link
Copy Markdown
Contributor

@rschlussel rschlussel Jun 29, 2023

Choose a reason for hiding this comment

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

the solution would be to add the isNative flag to the connector session and pass it through when we create the connector session. I think that's a very reasonable to do, and there may be other places that would want to handle things differently depending on the native flag too, so could be generally useful.

// https://github.com/prestodb/presto/issues/20008
false,
Copy link
Copy Markdown
Contributor

@rschlussel rschlussel Jun 27, 2023

Choose a reason for hiding this comment

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

is this safe to assume it's not native?

Copy link
Copy Markdown
Member Author

@vermapratyush vermapratyush Jun 27, 2023

Choose a reason for hiding this comment

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

I don't see this code path being triggered in native path.
It is used in

Copy link
Copy Markdown
Contributor

@rschlussel rschlussel Jun 27, 2023

Choose a reason for hiding this comment

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

I think these can be invoked from native flow if someone uses a sql function that uses NULLIF in its implementation. And seems like an easy place for bugs to lurk.

Copy link
Copy Markdown
Contributor

@aditi-pandit aditi-pandit Jun 28, 2023

Choose a reason for hiding this comment

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

Could this be because all the native tests set configuration "inline-sql-functions" to false ? https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/NativeQueryRunnerUtils.java#L32

Ideally, we should try to avoid such properties/configurations.

But is it possible to use that property instead in this rule ?

Copy link
Copy Markdown
Member Author

@vermapratyush vermapratyush Jun 29, 2023

Choose a reason for hiding this comment

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

@aditi-pandit Can you please elaborate what do you mean by can we use that property instead of this rule?

Copy link
Copy Markdown
Contributor

@aditi-pandit aditi-pandit Jun 29, 2023

Choose a reason for hiding this comment

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

@vermapratyush : I'm not super familiar with optimizer code, but it is possible that instead of hardcoding to false, we get the value for inline-sql-functions property at a higher level (or here itself if there is access to it) and use that instead in this code ?

Copy link
Copy Markdown
Member Author

@vermapratyush vermapratyush Jun 29, 2023

Choose a reason for hiding this comment

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

The issue with inline-sql-functions is the same as that of using Session to determine if the query is for native, certain code-flow does not have session object.
I also verified with the velox team, and it would not make sense to provide support for SQL function for native queries within the java stack. It is better to support sql function by implementing it natively in velox itself. So I think hard-coding is ok here.

new SqlToRowExpressionTranslator.Context()),
functionMetadata.getArgumentNames().get(),
arguments,
Expand Down
78 changes: 63 additions & 15 deletions ...o-main/src/main/java/com/facebook/presto/sql/relational/SqlToRowExpressionTranslator.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.relational;

import com.facebook.presto.Session;
import com.facebook.presto.SystemSessionProperties;
import com.facebook.presto.common.function.OperatorType;
import com.facebook.presto.common.function.SqlFunctionProperties;
import com.facebook.presto.common.transaction.TransactionId;
Expand All @@ -27,6 +28,8 @@
import com.facebook.presto.common.type.TypeWithName;
import com.facebook.presto.common.type.UnknownType;
import com.facebook.presto.common.type.VarcharType;
import com.facebook.presto.metadata.FunctionAndTypeManager;
import com.facebook.presto.spi.function.FunctionHandle;
import com.facebook.presto.spi.function.SqlFunctionId;
import com.facebook.presto.spi.function.SqlInvokedFunction;
import com.facebook.presto.spi.relation.ConstantExpression;
Expand Down Expand Up @@ -139,6 +142,7 @@
import static com.facebook.presto.spi.relation.SpecialFormExpression.Form.WHEN;
import static com.facebook.presto.sql.analyzer.ExpressionTreeUtils.getSourceLocation;
import static com.facebook.presto.sql.analyzer.ExpressionTreeUtils.resolveEnumLiteral;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.TYPE_MISMATCH;
import static com.facebook.presto.sql.analyzer.TypeSignatureProvider.fromTypes;
import static com.facebook.presto.sql.relational.Expressions.call;
import static com.facebook.presto.sql.relational.Expressions.constant;
Expand Down Expand Up @@ -171,14 +175,14 @@ public static RowExpression translate(
Expression expression,
Map<NodeRef<Expression>, Type> types,
Map<VariableReferenceExpression, Integer> layout,
FunctionAndTypeResolver functionAndTypeResolver,
FunctionAndTypeManager functionAndTypeManager,
Session session)
{
return translate(
expression,
types,
layout,
functionAndTypeResolver,
functionAndTypeManager,
session,
new Context());
}
Expand All @@ -187,41 +191,44 @@ public static RowExpression translate(
Expression expression,
Map<NodeRef<Expression>, Type> types,
Map<VariableReferenceExpression, Integer> layout,
FunctionAndTypeResolver functionAndTypeResolver,
FunctionAndTypeManager functionAndTypeManager,
Session session,
Context context)
{
return translate(
expression,
types,
layout,
functionAndTypeResolver,
functionAndTypeManager,
Optional.of(session.getUser()),
session.getTransactionId(),
session.getSqlFunctionProperties(),
session.getSessionFunctions(),
SystemSessionProperties.isNativeExecutionEnabled(session),
context);
}

public static RowExpression translate(
Expression expression,
Map<NodeRef<Expression>, Type> types,
Map<VariableReferenceExpression, Integer> layout,
FunctionAndTypeResolver functionAndTypeResolver,
FunctionAndTypeManager functionAndTypeManager,
Optional<String> user,
Optional<TransactionId> transactionId,
SqlFunctionProperties sqlFunctionProperties,
Map<SqlFunctionId, SqlInvokedFunction> sessionFunctions,
boolean isNative,
Context context)
{
Visitor visitor = new Visitor(
types,
layout,
functionAndTypeResolver,
functionAndTypeManager,
user,
transactionId,
sqlFunctionProperties,
sessionFunctions);
sessionFunctions,
isNative);
RowExpression result = visitor.process(expression, context);
requireNonNull(result, "translated expression is null");
return result;
Expand Down Expand Up @@ -256,30 +263,35 @@ private static class Visitor
{
private final Map<NodeRef<Expression>, Type> types;
private final Map<VariableReferenceExpression, Integer> layout;
private final FunctionAndTypeManager functionAndTypeManager;
private final FunctionAndTypeResolver functionAndTypeResolver;
private final Optional<String> user;
private final Optional<TransactionId> transactionId;
private final SqlFunctionProperties sqlFunctionProperties;
private final Map<SqlFunctionId, SqlInvokedFunction> sessionFunctions;
private final FunctionResolution functionResolution;
private final boolean isNative;

private Visitor(
Map<NodeRef<Expression>, Type> types,
Map<VariableReferenceExpression, Integer> layout,
FunctionAndTypeResolver functionAndTypeResolver,
FunctionAndTypeManager functionAndTypeManager,
Optional<String> user,
Optional<TransactionId> transactionId,
SqlFunctionProperties sqlFunctionProperties,
Map<SqlFunctionId, SqlInvokedFunction> sessionFunctions)
Map<SqlFunctionId, SqlInvokedFunction> sessionFunctions,
boolean isNative)
{
this.types = requireNonNull(types, "types is null");
this.layout = layout;
this.functionAndTypeResolver = functionAndTypeResolver;
this.user = user;
this.transactionId = transactionId;
this.sqlFunctionProperties = sqlFunctionProperties;
this.layout = requireNonNull(layout);
this.functionAndTypeManager = requireNonNull(functionAndTypeManager);
this.functionAndTypeResolver = functionAndTypeManager.getFunctionAndTypeResolver();
this.user = requireNonNull(user);
this.transactionId = requireNonNull(transactionId);
this.sqlFunctionProperties = requireNonNull(sqlFunctionProperties);
this.functionResolution = new FunctionResolution(functionAndTypeResolver);
this.sessionFunctions = sessionFunctions;
this.sessionFunctions = requireNonNull(sessionFunctions);
this.isNative = isNative;
}

private Type getType(Expression node)
Expand Down Expand Up @@ -830,6 +842,42 @@ protected RowExpression visitNullIfExpression(NullIfExpression node, Context con
RowExpression first = process(node.getFirst(), context);
RowExpression second = process(node.getSecond(), context);

if (isNative && !second.getType().equals(first.getType())) {
Optional<Type> commonType = functionAndTypeResolver.getCommonSuperType(first.getType(), second.getType());
if (!commonType.isPresent()) {
throw new SemanticException(TYPE_MISMATCH, node, "Types are not comparable with NULLIF: %s vs %s", first.getType(), second.getType());
}

Type returnType = getType(node);
// If the first type is unknown, as per presto's NULL_IF semantics we should not infer the type using second argument.
// Always return a null with unknown type.
if (first.getType().equals(UnknownType.UNKNOWN)) {
return constantNull(UnknownType.UNKNOWN);
}
RowExpression originalFirst = first;
// cast(first as <common type>)
if (!first.getType().equals(commonType.get())) {
first = call(
getSourceLocation(node),
CAST.name(),
functionAndTypeResolver.lookupCast(CAST.name(), first.getType(), commonType.get()),
commonType.get(), first);
}
// cast(second as <common type>)
if (!second.getType().equals(commonType.get())) {
second = call(
getSourceLocation(node),
CAST.name(),
functionAndTypeResolver.lookupCast(CAST.name(), second.getType(), commonType.get()),
commonType.get(), second);
}
FunctionHandle equalsFunctionHandle = functionAndTypeResolver.resolveOperator(EQUAL, fromTypes(first.getType(), second.getType()));
// equal(cast(first as <common type>), cast(second as <common type>))
RowExpression equal = call(EQUAL.name(), equalsFunctionHandle, BOOLEAN, first, second);

// if (equal(cast(first as <common type>), cast(second as <common type>)), cast(null as firstType), first)
return specialForm(IF, returnType, equal, constantNull(originalFirst.getType()), originalFirst);
}
return specialForm(getSourceLocation(node), NULL_IF, getType(node), first, second);
}

Expand Down
13 changes: 13 additions & 0 deletions presto-main/src/test/java/com/facebook/presto/operator/scalar/CustomFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,13 @@
package com.facebook.presto.operator.scalar;

import com.facebook.presto.common.type.StandardTypes;
import com.facebook.presto.spi.function.Description;
import com.facebook.presto.spi.function.LiteralParameters;
import com.facebook.presto.spi.function.ScalarFunction;
import com.facebook.presto.spi.function.SqlInvokedScalarFunction;
import com.facebook.presto.spi.function.SqlNullable;
import com.facebook.presto.spi.function.SqlParameter;
import com.facebook.presto.spi.function.SqlParameters;
import com.facebook.presto.spi.function.SqlType;
import io.airlift.slice.Slice;

Expand Down Expand Up @@ -45,4 +49,13 @@ public static boolean customIsNullBigint(@SqlNullable @SqlType(StandardTypes.BIG
{
return value == null;
}

@SqlInvokedScalarFunction(value = "custom_square", deterministic = true, calledOnNullInput = false)
@Description("Custom SQL to test NULLIF in Functions")
@SqlParameters({@SqlParameter(name = "x", type = "integer"), @SqlParameter(name = "y", type = "integer")})
@SqlType("integer")
public static String customSquare()
{
return "RETURN IF(NULLIF(x, y) IS NOT NULL, x * x, y * y)";
}
}
Loading