Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@pdabre12
pdabre12 authored and Pratik Joseph Dabre committed Dec 17, 2024
1 parent 0932870 commit 0986e15
Show file tree
Hide file tree
Showing 23 changed files with 100 additions and 144 deletions.
3 changes: 2 additions & 1 deletion ...on-namespace/src/test/java/com/facebook/presto/hive/functions/HiveFunctionsTestUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ public static TestingPrestoServer createTestingPrestoServer()
"hive-functions",
"hive",
getNamespaceManagerCreationProperties(),
server.getPluginNodeManager());
server.getPluginNodeManager(),
false);
server.refreshNodes();
return server;
}
Expand Down
3 changes: 2 additions & 1 deletion ...espace/src/test/java/com/facebook/presto/hive/functions/TestHiveAggregationFunctions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ private static TestingPrestoServer createServer()
"hive-functions",
"hive",
Collections.emptyMap(),
server.getPluginNodeManager());
server.getPluginNodeManager(),
false);
server.refreshNodes();
return server;
}
Expand Down
55 changes: 10 additions & 45 deletions presto-main/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import com.facebook.presto.common.type.TypeWithName;
import com.facebook.presto.common.type.UserDefinedType;
import com.facebook.presto.operator.window.WindowFunctionSupplier;
import com.facebook.presto.server.ServerConfig;
import com.facebook.presto.spi.NodeManager;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.function.AggregationFunctionImplementation;
Expand Down Expand Up @@ -66,7 +65,6 @@
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.UncheckedExecutionException;
import org.weakref.jmx.Managed;
Expand Down Expand Up @@ -106,7 +104,6 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.lang.String.format;
import static java.util.Collections.emptyList;
Expand All @@ -122,7 +119,6 @@
public class FunctionAndTypeManager
implements FunctionMetadataManager, TypeManager
{
private static final String IS_DEFAULT_NAMESPACE = "default.namespace";
private final TransactionManager transactionManager;
private final BlockEncodingSerde blockEncodingSerde;
private final BuiltInTypeAndFunctionNamespaceManager builtInTypeAndFunctionNamespaceManager;
Expand All @@ -135,7 +131,6 @@ public class FunctionAndTypeManager
private final LoadingCache<FunctionResolutionCacheKey, FunctionHandle> functionCache;
private final CacheStatsMBean cacheStatsMBean;
private final boolean nativeExecution;
private final boolean coordinatorSidecarEnabled;
private CatalogSchemaName currentDefaultNamespace = DEFAULT_NAMESPACE;

@Inject
Expand All @@ -144,7 +139,6 @@ public FunctionAndTypeManager(
BlockEncodingSerde blockEncodingSerde,
FeaturesConfig featuresConfig,
FunctionsConfig functionsConfig,
ServerConfig serverConfig,
HandleResolver handleResolver,
Set<Type> types)
{
Expand All @@ -165,7 +159,6 @@ public FunctionAndTypeManager(
this.functionSignatureMatcher = new FunctionSignatureMatcher(this);
this.typeCoercer = new TypeCoercer(functionsConfig, this);
this.nativeExecution = featuresConfig.isNativeExecutionEnabled();
this.coordinatorSidecarEnabled = serverConfig.isCoordinatorSidecarEnabled();
}

public static FunctionAndTypeManager createTestFunctionAndTypeManager()
Expand All @@ -175,7 +168,6 @@ public static FunctionAndTypeManager createTestFunctionAndTypeManager()
new BlockEncodingManager(),
new FeaturesConfig(),
new FunctionsConfig(),
new ServerConfig(),
new HandleResolver(),
ImmutableSet.of());
}
Expand Down Expand Up @@ -288,14 +280,16 @@ public void loadFunctionNamespaceManager(
String functionNamespaceManagerName,
String catalogName,
Map<String, String> properties,
NodeManager nodeManager)
NodeManager nodeManager,
boolean isDefaultNamespace)
{
requireNonNull(functionNamespaceManagerName, "functionNamespaceManagerName is null");
FunctionNamespaceManagerFactory factory = functionNamespaceManagerFactories.get(functionNamespaceManagerName);
checkState(factory != null, "No factory for function namespace manager %s", functionNamespaceManagerName);
ImmutableMap<String, String> newProperties =
ImmutableMap.copyOf(constructNewPropertyMapIfDefaultNamespaceConfigured(properties, catalogName));
FunctionNamespaceManager<?> functionNamespaceManager = factory.create(catalogName, newProperties, new FunctionNamespaceManagerContext(this, nodeManager, this));
if (isDefaultNamespace) {
this.currentDefaultNamespace = new CatalogSchemaName(catalogName, "default");
}
FunctionNamespaceManager<?> functionNamespaceManager = factory.create(catalogName, properties, new FunctionNamespaceManagerContext(this, nodeManager, this));
functionNamespaceManager.setBlockEncodingSerde(blockEncodingSerde);

transactionManager.registerFunctionNamespaceManager(catalogName, functionNamespaceManager);
Expand Down Expand Up @@ -386,19 +380,10 @@ public List<SqlFunction> listFunctions(Session session, Optional<String> likePat
ImmutableList.Builder<SqlFunction> functions = new ImmutableList.Builder<>();
if (!isListBuiltInFunctionsOnly(session)) {
functions.addAll(SessionFunctionUtils.listFunctions(session.getSessionFunctions()));

// If coordinator sidecar is enabled, filter on the current default namespace
if (coordinatorSidecarEnabled) {
functions.addAll(functionNamespaceManagers.entrySet().stream()
.flatMap(manager -> manager.getValue().listFunctions(likePattern, escape).stream()
.filter((functionName) -> functionName.getSignature().getName().getCatalogSchemaName().equals(currentDefaultNamespace)))
.collect(toImmutableList()));
}
else {
functions.addAll(functionNamespaceManagers.values().stream()
.flatMap(manager -> manager.listFunctions(likePattern, escape).stream())
.collect(toImmutableList()));
}
functions.addAll(functionNamespaceManagers.entrySet().stream()
.flatMap(manager -> manager.getValue().listFunctions(likePattern, escape).stream()
.filter((functionName) -> functionName.getSignature().getName().getCatalogSchemaName().equals(currentDefaultNamespace)))
.collect(toImmutableList()));
}
else {
functions.addAll(listBuiltInFunctions());
Expand Down Expand Up @@ -671,11 +656,6 @@ public FunctionHandle lookupCast(CastType castType, Type fromType, Type toType)
return builtInTypeAndFunctionNamespaceManager.getFunctionHandle(Optional.empty(), signature);
}

public boolean isCoordinatorSidecarEnabled()
{
return coordinatorSidecarEnabled;
}

public CatalogSchemaName getCurrentDefaultNamespace()
{
return currentDefaultNamespace;
Expand Down Expand Up @@ -777,21 +757,6 @@ private Optional<FunctionNamespaceManager<? extends SqlFunction>> getServingFunc
return Optional.ofNullable(functionNamespaceManagers.get(typeSignatureBase.getTypeName().getCatalogName()));
}

private Map<String, String> constructNewPropertyMapIfDefaultNamespaceConfigured(Map<String, String> properties, String catalogName)
{
if (!properties.containsKey(IS_DEFAULT_NAMESPACE)) {
return properties;
}
// If default namespace is configured, update the default namespace
String isDefaultNamespace = properties.get(IS_DEFAULT_NAMESPACE);
if (Boolean.parseBoolean(isDefaultNamespace)) {
this.currentDefaultNamespace = new CatalogSchemaName(catalogName, "default");
}
return properties.entrySet().stream()
.filter(entry -> !entry.getKey().equals(IS_DEFAULT_NAMESPACE))
.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
}

@Override
@SuppressWarnings("unchecked")
public SpecializedFunctionKey getSpecializedFunctionKey(Signature signature)
Expand Down
22 changes: 8 additions & 14 deletions presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import com.facebook.presto.common.type.Type;
import com.facebook.presto.common.type.TypeSignature;
import com.facebook.presto.execution.QueryManager;
import com.facebook.presto.server.ServerConfig;
import com.facebook.presto.spi.ColumnHandle;
import com.facebook.presto.spi.ColumnMetadata;
import com.facebook.presto.spi.ConnectorId;
Expand Down Expand Up @@ -214,39 +213,34 @@ public static MetadataManager createTestMetadataManager()

public static MetadataManager createTestMetadataManager(FeaturesConfig featuresConfig)
{
return createTestMetadataManager(new CatalogManager(), featuresConfig, new FunctionsConfig(), new ServerConfig());
return createTestMetadataManager(new CatalogManager(), featuresConfig, new FunctionsConfig());
}

public static MetadataManager createTestMetadataManager(FunctionsConfig functionsConfig)
{
return createTestMetadataManager(new CatalogManager(), new FeaturesConfig(), functionsConfig, new ServerConfig());
}

public static MetadataManager createTestMetadataManager(ServerConfig serverConfig)
{
return createTestMetadataManager(new CatalogManager(), new FeaturesConfig(), new FunctionsConfig(), serverConfig);
return createTestMetadataManager(new CatalogManager(), new FeaturesConfig(), functionsConfig);
}

public static MetadataManager createTestMetadataManager(CatalogManager catalogManager)
{
return createTestMetadataManager(catalogManager, new FeaturesConfig(), new FunctionsConfig(), new ServerConfig());
return createTestMetadataManager(catalogManager, new FeaturesConfig(), new FunctionsConfig());
}

public static MetadataManager createTestMetadataManager(CatalogManager catalogManager, FeaturesConfig featuresConfig, FunctionsConfig functionsConfig, ServerConfig serverConfig)
public static MetadataManager createTestMetadataManager(CatalogManager catalogManager, FeaturesConfig featuresConfig, FunctionsConfig functionsConfig)
{
return createTestMetadataManager(createTestTransactionManager(catalogManager), featuresConfig, functionsConfig, serverConfig);
return createTestMetadataManager(createTestTransactionManager(catalogManager), featuresConfig, functionsConfig);
}

public static MetadataManager createTestMetadataManager(TransactionManager transactionManager)
{
return createTestMetadataManager(transactionManager, new FeaturesConfig(), new FunctionsConfig(), new ServerConfig());
return createTestMetadataManager(transactionManager, new FeaturesConfig(), new FunctionsConfig());
}

public static MetadataManager createTestMetadataManager(TransactionManager transactionManager, FeaturesConfig featuresConfig, FunctionsConfig functionsConfig, ServerConfig serverConfig)
public static MetadataManager createTestMetadataManager(TransactionManager transactionManager, FeaturesConfig featuresConfig, FunctionsConfig functionsConfig)
{
BlockEncodingManager blockEncodingManager = new BlockEncodingManager();
return new MetadataManager(
new FunctionAndTypeManager(transactionManager, blockEncodingManager, featuresConfig, functionsConfig, serverConfig, new HandleResolver(), ImmutableSet.of()),
new FunctionAndTypeManager(transactionManager, blockEncodingManager, featuresConfig, functionsConfig, new HandleResolver(), ImmutableSet.of()),
blockEncodingManager,
createTestingSessionPropertyManager(),
new SchemaPropertyManager(),
Expand Down
7 changes: 4 additions & 3 deletions presto-main/src/main/java/com/facebook/presto/metadata/StaticFunctionNamespaceStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.io.Files.getNameWithoutExtension;
import static java.lang.Boolean.parseBoolean;
import static java.lang.String.format;

public class StaticFunctionNamespaceStore
Expand Down Expand Up @@ -83,15 +84,15 @@ private void loadFunctionNamespaceManager(String catalogName, Map<String, String
properties = new HashMap<>(properties);
String functionNamespaceManagerName = properties.remove(FUNCTION_NAMESPACE_MANAGER_NAME);
checkState(!isNullOrEmpty(functionNamespaceManagerName), "%s property must be present", FUNCTION_NAMESPACE_MANAGER_NAME);

if (Boolean.parseBoolean(properties.get(DEFAULT_NAMESPACE))) {
boolean defaultNamespace = parseBoolean(properties.remove(DEFAULT_NAMESPACE));
if (defaultNamespace) {
if (isDefaultNamespace) {
throw new IllegalStateException(
format("Only one function namespace manager can have the %s property set.", DEFAULT_NAMESPACE));
}
isDefaultNamespace = true;
}
functionAndTypeManager.loadFunctionNamespaceManager(functionNamespaceManagerName, catalogName, properties, nodeManager);
functionAndTypeManager.loadFunctionNamespaceManager(functionNamespaceManagerName, catalogName, properties, nodeManager, defaultNamespace);
log.info("-- Added function namespace manager [%s] --", catalogName);
}

Expand Down
14 changes: 7 additions & 7 deletions presto-main/src/main/java/com/facebook/presto/sql/planner/PlanOptimizers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -893,13 +893,13 @@ public PlanOptimizers(
// Optimizers above this do not need to care about aggregations with the type other than SINGLE
// This optimizer must be run after all exchange-related optimizers
builder.add(new IterativeOptimizer(
metadata,
ruleStats,
statsCalculator,
costCalculator,
ImmutableSet.of(
new PushPartialAggregationThroughJoin(),
new PushPartialAggregationThroughExchange(metadata.getFunctionAndTypeManager(), featuresConfig.isNativeExecutionEnabled()))),
metadata,
ruleStats,
statsCalculator,
costCalculator,
ImmutableSet.of(
new PushPartialAggregationThroughJoin(),
new PushPartialAggregationThroughExchange(metadata.getFunctionAndTypeManager(), featuresConfig.isNativeExecutionEnabled()))),
// MergePartialAggregationsWithFilter should immediately follow PushPartialAggregationThroughExchange
new MergePartialAggregationsWithFilter(metadata.getFunctionAndTypeManager()),
new IterativeOptimizer(
Expand Down
1 change: 0 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/AddExchanges.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,6 @@ public PlanWithProperties visitTableFinish(TableFinishNode node, PreferredProper
PlanNode child = planChild(node, PreferredProperties.any()).getNode();

ExchangeNode gather;

// in case the input is a union (see PushTableWriteThroughUnion), don't add another exchange
if (child instanceof ExchangeNode) {
ExchangeNode exchangeNode = (ExchangeNode) child;
Expand Down
16 changes: 16 additions & 0 deletions presto-main/src/main/java/com/facebook/presto/sql/relational/FunctionResolution.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import com.facebook.presto.common.function.OperatorType;
import com.facebook.presto.common.type.CharType;
import com.facebook.presto.common.type.Type;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.StandardErrorCode;
import com.facebook.presto.spi.function.FunctionHandle;
import com.facebook.presto.spi.function.StandardFunctionResolution;
import com.facebook.presto.sql.analyzer.FunctionAndTypeResolver;
Expand Down Expand Up @@ -85,6 +87,20 @@ public FunctionHandle likeVarcharFunction()
return functionAndTypeResolver.lookupFunction("LIKE", fromTypes(VARCHAR, LIKE_PATTERN));
}

public boolean supportsLikePatternFunction()
{
try {
functionAndTypeResolver.lookupFunction("LIKE_PATTERN", fromTypes(VARCHAR, VARCHAR));
return true;
}
catch (PrestoException e) {
if (e.getErrorCode() == StandardErrorCode.FUNCTION_NOT_FOUND.toErrorCode()) {
return false;
}
throw e;
}
}

public FunctionHandle likeVarcharVarcharFunction()
{
return functionAndTypeResolver.lookupFunction("LIKE", fromTypes(VARCHAR, VARCHAR));
Expand Down
8 changes: 3 additions & 5 deletions ...o-main/src/main/java/com/facebook/presto/sql/relational/SqlToRowExpressionTranslator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,6 @@ private static class Visitor
private final SqlFunctionProperties sqlFunctionProperties;
private final Map<SqlFunctionId, SqlInvokedFunction> sessionFunctions;
private final FunctionResolution functionResolution;
private final boolean coordinatorSidecarEnabled;

private Visitor(
Map<NodeRef<Expression>, Type> types,
Expand All @@ -292,7 +291,6 @@ private Visitor(
this.sqlFunctionProperties = requireNonNull(sqlFunctionProperties);
this.functionResolution = new FunctionResolution(functionAndTypeResolver);
this.sessionFunctions = requireNonNull(sessionFunctions);
this.coordinatorSidecarEnabled = functionAndTypeManager.isCoordinatorSidecarEnabled();
}

private Type getType(Expression node)
Expand Down Expand Up @@ -910,7 +908,7 @@ protected RowExpression visitLikePredicate(LikePredicate node, Context context)

if (node.getEscape().isPresent()) {
RowExpression escape = process(node.getEscape().get(), context);
if (coordinatorSidecarEnabled) {
if (!functionResolution.supportsLikePatternFunction()) {
return call(value.getSourceLocation(), "LIKE", functionResolution.likeVarcharVarcharVarcharFunction(), BOOLEAN, value, pattern, escape);
}
return likeFunctionCall(value, call(getSourceLocation(node), "LIKE_PATTERN", functionResolution.likePatternFunction(), LIKE_PATTERN, pattern, escape));
Expand All @@ -921,7 +919,7 @@ protected RowExpression visitLikePredicate(LikePredicate node, Context context)
return prefixOrSuffixMatch;
}

if (coordinatorSidecarEnabled) {
if (!functionResolution.supportsLikePatternFunction()) {
return likeFunctionCall(value, pattern);
}

Expand Down Expand Up @@ -970,7 +968,7 @@ else if (LIKE_SIMPLE_EXISTS_PATTERN.matcher(patternString).matches()) {
private RowExpression likeFunctionCall(RowExpression value, RowExpression pattern)
{
if (value.getType() instanceof VarcharType) {
if (coordinatorSidecarEnabled) {
if (!functionResolution.supportsLikePatternFunction()) {
return call(value.getSourceLocation(), "LIKE", functionResolution.likeVarcharVarcharFunction(), BOOLEAN, value, pattern);
}
return call(value.getSourceLocation(), "LIKE", functionResolution.likeVarcharFunction(), BOOLEAN, value, pattern);
Expand Down
Loading

0 comments on commit 0986e15

Please sign in to comment.