You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/06/30 20:56:49 UTC
[GitHub] [calcite] birschick-bq opened a new pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.4) to provide class loader.
birschick-bq opened a new pull request #2449:
URL: https://github.com/apache/calcite/pull/2449
## Description
This change addresses the issue of the Janino library not being able to load the correct class loader.
- Passes the class loader of the calling class.
- Updates the Janino version to 3.1.4
## Notes
- There is a [know issue](https://github.com/janino-compiler/janino/pull/150) in the Janino library where the class loader is not being properly passed to underlying methods. A [fix ](https://github.com/janino-compiler/janino/pull/150) has been applied but not release yet.
- As of version 3.1.3, a number of Calcite test fail with `org.codehaus.commons.compiler.InternalCompilerException`. This pull request is here to help highlight the issue and work with Janino team to resolve that issue.
- So this pull request will need to be updated with a newer version of Janino where both issue are resolved.
## Related Issue
- https://issues.apache.org/jira/browse/CALCITE-3745
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-939881009
@birschick-bq when the method argument does not have an explicit annotation then CheckerFramework uses a default based on some [rules||https://checkerframework.org/manual/#climb-to-top] and in the case of `getDefaultCompilerFactory` it is `@NonNull`. Moverover looking into the source code of `getDefaultCompilerFactory` it seems that if `null` is given as an input it will fail with NPE.
Regarding the `setParentClassLoader` method the `@Nullable` annotations used by Janino are not the same used with those used by the CheckerFramework. To align them we make use of `.astub` files.
I resolved the problems in commit https://github.com/apache/calcite/pull/2449/commits/a0ef603257c274d445a0c1b117db8b1eb595ca5a. Please have a look and let me know what you think. If all is good I will rebase and commit afterwards.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#discussion_r679014233
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java
##########
@@ -135,7 +135,8 @@ static Bindable getBindable(ClassDeclaration expr, String s, int fieldCount)
throws CompileException, IOException, ExecutionException {
ICompilerFactory compilerFactory;
try {
- compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory();
+ compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory(
Review comment:
Why do we need to pass explicitly the class loader of the class? Is this change necessary due to the Janino update or we are trying to tackle another problem at the same time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] birschick-bq commented on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
birschick-bq commented on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-908642395
> @birschick-bq
>
> > Hmm. Build errors raised by CheckerFramework seem to be a know bug [typetools/checker-framework#979](https://github.com/typetools/checker-framework/issues/979). May need to suppress this false-positive error.
>
> The errors seem to be related to the nullability of `Class#getClassLoader()` method. According to the javadoc of the respective method it may return `null`.
>
> > Some implementations may use null to represent the bootstrap class loader
>
> Is it legal to pass `null` in the `getDefaultCompilerFactory`?
@zabetak
It is "legal" to pass `null` to getDefaultCompilerFactory - as the parameter is not marked with any explicit attribute. The other methods (i.e., `setParentClassLoader`) have an "explicit" attribute for `@Nullable`.
This is an issue of the **CheckerFramework** - not the source code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] birschick-bq commented on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
birschick-bq commented on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-889360412
Hmm. Build errors raised by CheckerFramework seem to be a know bug https://github.com/typetools/checker-framework/issues/979. May need to suppress this false-positive error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] birschick-bq commented on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
birschick-bq commented on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-908642395
> @birschick-bq
>
> > Hmm. Build errors raised by CheckerFramework seem to be a know bug [typetools/checker-framework#979](https://github.com/typetools/checker-framework/issues/979). May need to suppress this false-positive error.
>
> The errors seem to be related to the nullability of `Class#getClassLoader()` method. According to the javadoc of the respective method it may return `null`.
>
> > Some implementations may use null to represent the bootstrap class loader
>
> Is it legal to pass `null` in the `getDefaultCompilerFactory`?
@zabetak
It is "legal" to pass `null` to getDefaultCompilerFactory - as the parameter is not marked with any explicit attribute. The other methods (i.e., `setParentClassLoader`) have an "explicit" attribute for `@Nullable`.
This is an issue of the **CheckerFramework** - not the source code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-891005637
@birschick-bq
> Hmm. Build errors raised by CheckerFramework seem to be a know bug [typetools/checker-framework#979](https://github.com/typetools/checker-framework/issues/979). May need to suppress this false-positive error.
The errors seem to be related to the nullability of `Class#getClassLoader()` method. According to the javadoc of the respective method it may return `null`.
> Some implementations may use null to represent the bootstrap class loader
Is it legal to pass `null` in the `getDefaultCompilerFactory`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] birschick-bq commented on a change in pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
birschick-bq commented on a change in pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#discussion_r679357499
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java
##########
@@ -135,7 +135,8 @@ static Bindable getBindable(ClassDeclaration expr, String s, int fieldCount)
throws CompileException, IOException, ExecutionException {
ICompilerFactory compilerFactory;
try {
- compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory();
+ compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory(
Review comment:
@zabetak Good question.
The root issue is that using Calcite in an "uber" JAR fails. When Calcite tries to load the Janino compiler using the `CompilerFactoryFactory`, it fails in the previous version of Janino commons library. This is because It was using the `Thread.currentThread().getContextClassLoader()` which was `null` at runtime.
I worked with the team at Janino to find a solution - where Calcite can pass its class loader to the Janino factory class so that it has the working class's class loader.
So we needed the latest version of Janino to get a fix to the problem of using Calcite in an "uber" JAR.
Please see the origin [JIRA issue](https://issues.apache.org/jira/browse/CALCITE-3745).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] birschick-bq commented on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
birschick-bq commented on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-940348964
> @birschick-bq when the method argument does not have an explicit annotation then CheckerFramework uses a default based on some [rules](https://checkerframework.org/manual/#climb-to-top) and in the case of `getDefaultCompilerFactory` it is `@NonNull`. Moverover looking into the source code of `getDefaultCompilerFactory` it seems that if `null` is given as an input it will fail with NPE.
>
> Regarding the `setParentClassLoader` method the `@Nullable` annotations used by Janino are not the same used with those used by the CheckerFramework. To align them we make use of `.astub` files.
>
> I resolved the problems in commit [a0ef603](https://github.com/apache/calcite/commit/a0ef603257c274d445a0c1b117db8b1eb595ca5a). Please have a look and let me know what you think. If all is good I will rebase and commit afterwards.
@zabetak Looks good to me. Thanks for resolving this issue. I was not familiar with how CheckerFramework works, in this situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak edited a comment on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
zabetak edited a comment on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-939881009
@birschick-bq when the method argument does not have an explicit annotation then CheckerFramework uses a default based on some [rules|https://checkerframework.org/manual/#climb-to-top] and in the case of `getDefaultCompilerFactory` it is `@NonNull`. Moverover looking into the source code of `getDefaultCompilerFactory` it seems that if `null` is given as an input it will fail with NPE.
Regarding the `setParentClassLoader` method the `@Nullable` annotations used by Janino are not the same used with those used by the CheckerFramework. To align them we make use of `.astub` files.
I resolved the problems in commit https://github.com/apache/calcite/pull/2449/commits/a0ef603257c274d445a0c1b117db8b1eb595ca5a. Please have a look and let me know what you think. If all is good I will rebase and commit afterwards.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#discussion_r680954001
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java
##########
@@ -135,7 +135,8 @@ static Bindable getBindable(ClassDeclaration expr, String s, int fieldCount)
throws CompileException, IOException, ExecutionException {
ICompilerFactory compilerFactory;
try {
- compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory();
+ compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory(
Review comment:
Got it, thanks for the explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak edited a comment on pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
zabetak edited a comment on pull request #2449:
URL: https://github.com/apache/calcite/pull/2449#issuecomment-939881009
@birschick-bq when the method argument does not have an explicit annotation then CheckerFramework uses a default based on some [rules](https://checkerframework.org/manual/#climb-to-top) and in the case of `getDefaultCompilerFactory` it is `@NonNull`. Moverover looking into the source code of `getDefaultCompilerFactory` it seems that if `null` is given as an input it will fail with NPE.
Regarding the `setParentClassLoader` method the `@Nullable` annotations used by Janino are not the same used with those used by the CheckerFramework. To align them we make use of `.astub` files.
I resolved the problems in commit https://github.com/apache/calcite/pull/2449/commits/a0ef603257c274d445a0c1b117db8b1eb595ca5a. Please have a look and let me know what you think. If all is good I will rebase and commit afterwards.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak closed pull request #2449: [CALCITE-3745] Integrate to new version of Janino (3.1.6) to provide class loader.
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2449:
URL: https://github.com/apache/calcite/pull/2449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org