tada / pljava

PL/Java is a free add-on module that brings Java™ Stored Procedures, Triggers, Functions, Aggregates, Operators, Types, etc., to the PostgreSQL™ backend.
http://tada.github.io/pljava/
Other
238 stars 77 forks source link

Allow functions from an interface #426

Closed Zastai closed 1 year ago

Zastai commented 1 year ago

It is not an uncommon practice to use an interface as a container for public static methods. However, adding @Function to a public static method in an interface results in a compilation failure:

[ERROR] Somehow this method got enclosed by something other than a class

This is a rather unhelpful message to being with: Which method are you talking about? Who is telling me this?

But as far as I can tell, PL/Java has no need to instantiate such an outer class at all, so there seems to be no real reason why it can't be an interface; that should not affect the ability to invoke a contained static method.

jcflack commented 1 year ago

Allowing a static function on an interface seems reasonable on its face. The SQL/JRT standard does literally say

If R is an external Java routine, then the <Java method name> is the name of one or
more Java methods in the class specified by <Java class name> in the JAR specified by <jar name>. The
combination of <Java class name> and <Java method name> represent a fully qualified Java class name
and method name. The method name can reference a method of the class, or a method of a superclass of
the class.

and does not mention interfaces. Likewise, the PL/Java documentation has historically said "A Java function is declared with the name of a class and a static method on that class".

It could be fair to wonder whether the SQL committee really wrote class intending to exclude interfaces, or class in a more inclusive sense.

You might try having a static function on an interface, without the @Function annotation, and declaring it by issuing a suitable SQL CREATE FUNCTION directly. If that works, and there are no runtime issues, then it would seem the PL/Java runtime can already support the method being on an interface, and only the compile-time check in DDRProcessor would need to be relaxed to allow for an interface as well as a class.

The unhelpfulness of the error message is a bit disappointing. It is reported using the overload of Messager.printMessage that takes an Element argument, and is described in the Java API docs as producing a message "at the location of the element" in the source.

It might be worth checking what message is produced running javac standalone. I seem to remember noticing that Maven must be running the compiler programmatically and hooking the diagnostic messages, which it sometimes displays in less helpful ways than javac itself would.

Zastai commented 1 year ago

Used mvn compile -X to get the command line it uses, and ran javac directly:

$ javac -d /xxx/target/classes -classpath ... -sourcepath /xxx/src/main/java: -s /xxx/target/generated-sources/annotations -g -nowarn -target 11 -source 11 -encoding UTF-8 /xxx/src/main/java/.../Hello.java
error: Somehow this method got enclosed by something other than a class
error: Somehow this method got enclosed by something other than a class
2 errors
$ javac -version
javac 11.0.18

Note also that that Hello source only has

public interface Hello {

    @Function(onNullInput = Function.OnNullInput.RETURNS_NULL)
    public static String hello(String toWhom) {
        return "Hello, " + toWhom + "!";
    }

}

but results in 2 separate diagnostics.

jcflack commented 1 year ago

I am further disappointed then that even the standalone compiler is not displaying any of the information from the Element parameter passed to printMessage.

The duplication of the message probably just reflects the passes of operation within DDRProcessor. (It is possible the absence of Element information does also; I seem to remember learning that javac throws out its collected source locations at some stage of processing, and then printMessage cannot show that information if called later than that, and the API docs don't mention this.) Both issues might, therefore, be improved by reorganizing DDRProcessor to make this check earlier (I haven't looked).

But improving the message handling seems like a secondary priority. I am especially interested in whether you can compile Hello (without the @Function) and successfully use it in Postgres with a CREATE FUNCTION that you issue yourself.

If that works, then all that is needed is to relax this compile-time check to allow an interface also. If it does not work, then attention would also be needed to the runtime code.

Zastai commented 1 year ago

I'll try to remember to try that tomorrow. I would expect it to work though (there is no difference in bytecode for such a call, it's always an invokestatic).

jcflack commented 1 year ago

I'm thinking more about possible subtleties in the PL/Java code that looks it up and constructs the method handle for it. Not that I necessarily expect it won't work, but it's never been advertised or tested and there is room for complications.

Zastai commented 1 year ago

Looks like it works, at least for the simple hello(toWhom) example case.

jcflack commented 1 year ago

Good to know.

jcflack commented 1 year ago

Would you have time to try a build from the feature/REL1_6_STABLE/issue426 branch?

The Appveyor tests appear failed, but I think that's just because Appveyor fell over.

Zastai commented 1 year ago

Will do, although I currently don't know when that would be.

jcflack commented 1 year ago

Believed resolved in 1.6.5.