Function resolution #12743

rongrong · 2019-04-30T05:19:06Z

No description provided.

caithagoras · 2019-08-22T21:45:18Z

I'll take a look today/tomorrow.

highker

Everything up to "Move FunctionNamespaceManager to SPI" LGTM

highker · 2019-08-21T06:57:36Z

presto-main/src/main/java/com/facebook/presto/metadata/FunctionNamespaceManager.java

+    /**
+     * Ideally function namespaces should support transactions like connectors do, and getCandidates should be transaction-aware.
+     * queryId serves as a transaction ID before proper support for transaction is introduced.
+     * TODO Support transaction in function namespaces


Might need to add @Experimental for this interface

highker · 2019-08-21T06:57:51Z

presto-main/src/main/java/com/facebook/presto/metadata/FunctionNamespaceManager.java

+     * for a given queryId, getFunctionHandle with the same queryId should return a valid FunctionHandle, even if the function
+     * is deleted. Multiple calls of this function with the same parameters should return the same FunctionHandle.
+     * queryId serves as a transaction ID before proper support for transaction is introduced.
+     * TODO Support transaction in function namespaces


Might need to add @Experimental for this interface

highker · 2019-08-22T05:56:45Z

presto-main/src/main/java/com/facebook/presto/metadata/FunctionManager.java

+                                .map(TypeSignatureProvider::getTypeSignature)
+                                .collect(toImmutableList()));
+            }
+            else {


else is redundant

caithagoras · 2019-08-23T21:06:23Z

presto-spi/src/main/java/com/facebook/presto/spi/relation/FullyQualifiedName.java

+    private final List<String> originalParts;
+
+    @JsonCreator
+    public static FullyQualifiedName of(String dottedName)


This method has different assumption for the parameter from other overloaded of method. Should we give it a different name to avoid confusion?

It is also less readable this way since both this method and the next method have the same name while both can take one String parameter as their inputs.

caithagoras · 2019-08-23T21:14:18Z

presto-spi/src/main/java/com/facebook/presto/spi/relation/FullyQualifiedName.java

+        return of(asList(parts));
+    }
+
+    public static FullyQualifiedName of(String... parts)


Supplying 2 parts always throw. Instead we can do

public static FullyQualifiedName of(String first, String second, String third, String... rest)

Similar to https://github.com/prestodb/presto/blob/master/presto-parser/src/main/java/com/facebook/presto/sql/tree/QualifiedName.java#L35

caithagoras · 2019-08-23T21:34:54Z

presto-spi/src/main/java/com/facebook/presto/spi/relation/FullyQualifiedName.java

+        return new FullyQualifiedName(originalParts, parts);
+    }
+
+    public static FullyQualifiedName of(FullyQualifiedName.Prefix prefix, String name)


This seems to be only use case of Prefix. Should we add a similar constraints to Prefix to only allow 2 or more parts?

I think I added that in later commits. But here since the constructor is private I don't see the need.

caithagoras · 2019-08-28T05:59:13Z

presto-main/src/main/java/com/facebook/presto/metadata/StaticFunctionNamespaceStore.java

+    public StaticFunctionNamespaceStore(FunctionManager functionManager, StaticFunctionNamespaceStoreConfig config)
+    {
+        this.functionManager = functionManager;
+        this.configDir = config.getFunctionNamespaceConfigurationDir();


is this allowed to be null? If so, use Optional instead.

caithagoras · 2019-08-28T05:59:57Z

presto-main/src/main/java/com/facebook/presto/metadata/StaticFunctionNamespaceStore.java

+
+    private static List<File> listFiles(File dir)
+    {
+        if (dir != null && dir.isDirectory()) {


nit: return empty list early:

if (...) { return ImmutableList.of(); } ...

caithagoras · 2019-08-28T06:22:25Z

presto-main/src/main/java/com/facebook/presto/metadata/StaticFunctionNamespaceStore.java

+import static com.facebook.presto.util.PropertiesUtil.loadProperties;
+import static com.google.common.base.Preconditions.checkState;
+
+public class StaticFunctionNamespaceStore


This class is called StaticFunctionNamespaceStore so I expect it to be relevant only to the built-in functions and StiatcFunctionNamespaceManager, but instead, this class serves as an entry point to load all the function namespace managers?

The implementation of the StaticFunctionNamespaceStore is more or less a copy of StaticCatalogStore. Here "static" is more referring to the available function namespaces and how they are served are configured statically. I'm open to name suggestions. Maybe we can change StaticFunctionNamespace to BuiltinFunctionNamespace.

Sounds good to me. I think that would avoid confusion from overloading the meaning of static

caithagoras · 2019-08-28T06:26:33Z

lgtm overall, but since I'm not yet familiar with the code, I'll let @highker approve.

highker

One high-level comment regarding config

highker · 2019-08-30T05:56:30Z

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

                createTestingViewCodec(),
                blockEncodingSerde,
                sessionPropertyManager,
                schemaPropertyManager,
                tablePropertyManager,
                columnPropertyManager,
                analyzePropertyManager,
-                transactionManager);
+                transactionManager,
+                new FunctionManager(typeManager, blockEncodingSerde, featuresConfig));


FunctionManager can be injected right?

This constructor is used for testing purposes.

Could you annotate the constructor of MetadataManager with for test only as well?

This is called in LocalQueryRunner which is used for testing but is not in the /test directory. Also it's not about visibility, so not sure whether @VisibleForTesting is appropriate. Is there other annotations for testing?

That should be fine. XXXQueryRunner is for testing only (as a convention) anyway....

highker · 2019-08-30T06:06:12Z

presto-main/src/main/java/com/facebook/presto/metadata/StaticFunctionNamespaceStore.java

+    private static final Splitter SPLITTER = Splitter.on(',').trimResults().omitEmptyStrings();
+
+    private final FunctionManager functionManager;
+    private final File configDir;


Do we really need a directory? We have been gradually ditching file-based configs. The only place I remember we still use is the catalog directory that is used for installing connector plugins and the other file-based configs are legacy logic. If the config is just some attributes, shall we use the normal configuration than a file?

I'm basically copying the configuration of catalogs here because the concepts kinda overlap so I think it's nice to keep them the same way. Happy to take other alternatives if they makes more sense. What do you mean by "normal configuration"?

Maybe I should understand more about how function namespaces will be configured. Will it be something similar to connectors, where each connector has its own config? Or it will be something simple like log.properties below. The former may need a file-based config but the latter can be just put into a new class like LogConfig.java.

The way I see this, each config would be configuring a different function namespace manager, which can manage 1 or many function namespaces. Some function namespaces are just logical separations but physically could be managed the same way. For example, you might want to have separate function namespaces for different teams, but they are all SQL functions and can be managed using the same manager with the same configuration. On the other hand, other function namespaces can be physically different. For example, you may want to support MySQL functions as a different function namespace, so people can directly use mysql.function.foo in their queries. This kind of function namespaces would have a 1:1 mapping to the connector they serve.

Let me know whether you think this is reasonable. We can discuss this further.

highker · 2019-08-30T06:25:02Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

@@ -400,7 +400,7 @@ protected void setup(Binder binder)
        // connector plan optimizer manager
        binder.bind(ConnectorPlanOptimizerManager.class).in(Scopes.SINGLETON);

-        // index manager
+        // index managerTestVariableExtractor


Looks like an accidental change

highker

LGTM

rongrong · 2019-09-03T19:16:11Z

@highker I added an additional commit to rename StaticFunctionNamespaceManager to BuiltInFunctionNamespaceManager to avoid confusion between function namespace manager and function namespace config. Do you want to take another look?

StaticFunctionNamespace will be handled separately. It won't need to be dynamically registered nor will it be used to support dynamic namespaces so the factory is not necessary.

Break the resolveFunction API into getCandidates and getFunctionHandle so the function resolution logic can be managed consistently within the query engine.

We enforce explicit naming for dynamic function namespaces. All unqualified function names will only be resolved against the built-in static function namespace. While it is possible to define an ordering (through SQL path or other means) and convention (best match / first match), in reality when complicated namespaces are involved such implicit resolution might hide errors and cause confusion.

facebook-github-bot added the CLA Signed label Apr 30, 2019

rongrong force-pushed the function-resolution branch 11 times, most recently from e0854ad to b455821 Compare May 2, 2019 23:45

highker mentioned this pull request Jun 7, 2019

[test] Symbol partition by #12815

Closed

rongrong force-pushed the function-resolution branch 2 times, most recently from 7f96068 to 2d8b8a9 Compare July 23, 2019 17:50

rongrong force-pushed the function-resolution branch from 2d8b8a9 to 13b5fd8 Compare August 7, 2019 00:21

rongrong force-pushed the function-resolution branch 12 times, most recently from 44f40b4 to 9e9667a Compare August 19, 2019 22:24

rongrong requested review from highker and arhimondr August 19, 2019 22:48

rongrong requested a review from hellium01 August 23, 2019 02:05

highker reviewed Aug 23, 2019

View reviewed changes

caithagoras reviewed Aug 26, 2019

View reviewed changes

rongrong force-pushed the function-resolution branch 2 times, most recently from 352533c to d8b28d0 Compare August 27, 2019 02:25

caithagoras reviewed Aug 28, 2019

View reviewed changes

highker self-requested a review August 28, 2019 06:37

highker reviewed Aug 30, 2019

View reviewed changes

rongrong force-pushed the function-resolution branch from d8b28d0 to ced4deb Compare August 30, 2019 19:21

highker approved these changes Aug 30, 2019

View reviewed changes

rongrong force-pushed the function-resolution branch from ced4deb to 7792381 Compare September 3, 2019 19:15

rongrong force-pushed the function-resolution branch from 7792381 to df42c75 Compare September 4, 2019 00:08

rongrong added 11 commits September 4, 2019 11:33

Introduce FullyQualifiedName in SPI

d8f34b0

Change Signature to use FullyQualifiedName

9622d26

Remove StaticFunctionNamespaceFactory

fa36f4d

StaticFunctionNamespace will be handled separately. It won't need to be dynamically registered nor will it be used to support dynamic namespaces so the factory is not necessary.

Rename FunctionNamespace to FunctionNamespaceManager

e4daef3

Move SqlFunction to spi

272cd95

Break FunctionNamespaceManager.resolveFunction

e23005f

Break the resolveFunction API into getCandidates and getFunctionHandle so the function resolution logic can be managed consistently within the query engine.

Move function resolution logic to FunctionManager

5149d92

Move FunctionNamespaceManager to SPI

85d30a6

Rename StaticFunctionNamespace to BuiltInFunctionNamespace

3052863

Allow FunctionNamespace plugin

c359c21

rongrong force-pushed the function-resolution branch from df42c75 to 3379b4f Compare September 4, 2019 18:34

rongrong merged commit 3379b4f into prestodb:master Sep 4, 2019

rongrong deleted the function-resolution branch September 4, 2019 18:34

neeradsomanchi mentioned this pull request Sep 9, 2019

Release notes for 0.226 #13340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function resolution #12743

Function resolution #12743

rongrong commented Apr 30, 2019

caithagoras commented Aug 22, 2019

highker left a comment

highker Aug 21, 2019

highker Aug 21, 2019

highker Aug 22, 2019

caithagoras Aug 23, 2019

caithagoras Aug 23, 2019

caithagoras Aug 23, 2019

rongrong Aug 27, 2019

caithagoras Aug 28, 2019

caithagoras Aug 28, 2019 •

edited

Loading

caithagoras Aug 28, 2019 •

edited

Loading

rongrong Aug 29, 2019

caithagoras Sep 3, 2019

caithagoras commented Aug 28, 2019

highker left a comment

highker Aug 30, 2019

rongrong Aug 30, 2019

highker Aug 30, 2019

rongrong Aug 30, 2019

highker Aug 30, 2019

highker Aug 30, 2019

rongrong Aug 30, 2019

highker Aug 30, 2019

rongrong Aug 30, 2019

highker Aug 30, 2019

highker left a comment

rongrong commented Sep 3, 2019

Function resolution #12743

Function resolution #12743

Conversation

rongrong commented Apr 30, 2019

caithagoras commented Aug 22, 2019

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caithagoras Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

caithagoras Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caithagoras commented Aug 28, 2019

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

highker left a comment

Choose a reason for hiding this comment

rongrong commented Sep 3, 2019

caithagoras Aug 28, 2019 •

edited

Loading

caithagoras Aug 28, 2019 •

edited

Loading