chore: avoid Java version mismatch #433
Conversation
bestbeforetoday
force-pushed
the
java-version
branch
from
64d9cbf to
4fc2133
Compare
There was a problem hiding this comment.
Took a quick skim, and I'm broadly on-board with these changes. If you can fix the conflicts introduced by #427, I can prioritize this PR to avoid future conflicts.
Do we still need to keep the various Jabel plugins around after this?
| (select i_item_sk from item where item.i_item_sk = store_sales.ss_item_sk)"""; | ||
| "SELECT ss_sold_date_sk, ss_item_sk, ss_customer_sk\n" | ||
| + "FROM store_sales CROSS JOIN LATERAL\n" | ||
| + " (select i_item_sk from item where item.i_item_sk = store_sales.ss_item_sk)"; |
There was a problem hiding this comment.
The multi-line strings are the one thing I'm going to really miss, and they're really only used in the tests. Is it possible to use a different version of Java in just the tests?
There was a problem hiding this comment.
It might be possible to configure toolchains for specific tasks so that the tests are compiled and run with a different JVM to the one used for the main code compilation. I haven't tried this personally.
Alternatively, it might be possible to use a later JVM / toolchain but specify different release options for the main and test code compiles. Again, I haven't tried this.
There isn't much test code that uses formatted multi-line strings. Maybe we can live with it for those cases. Or the expected strings could be pulled out to files and loaded from disk by the test.
bestbeforetoday
force-pushed
the
java-version
branch
2 times, most recently
from
b3e444e to
209f902
Compare
|
I have rebased on the new commit and resolved the merge conflicts. I have removed the Jabel usage too — I hadn't noticed that before! The failing build checks look to be caused by vulnerabilities in the Calcite's dependency tree. I'd prefer not mix any changes to address that into this PR. |
I created #442 for the vulnerability |
bestbeforetoday
force-pushed
the
java-version
branch
from
209f902 to
56be40d
Compare
|
Rebased and resolved merge conflicts again. |
|
@vbarua I have added a second commit that configures a Java 17 toolchain for only the Isthmus tests, and reverting (almost) all of the changes to the Isthmus unit test code to make it compliant with Java 11. This allows the use of text blocks in the unit tests while maintaining strict Java 11 compliance for the main code. Just be warned that — for me at least — this seems to make VS Code very unhappy since it still expects the test code to be Java 11. Intellij seems to deal with it OK. It is likely to cause problems for some users though. Let me know if you want me to remove this change, or to make similar changes to any of the other test components. |
Let's undo that commit. I can live without multi-line strings, and I'd rather focus on making sure the development experience is good, which is the whole point of this change in a lot of ways. |
bestbeforetoday
force-pushed
the
java-version
branch
from
a67157a to
56be40d
Compare
|
@vbarua I have reverted to last commit so this change is back at the state where a consistent toolchain (matching the target release bytecode version) is used for each sub-project. |
bestbeforetoday
force-pushed
the
java-version
branch
from
56be40d to
39ea6bb
Compare
There was a problem hiding this comment.
I am anyway a fan of using explicit types instead of the var keyword so most of the changes to core I prefer.
Losing multi-line strings and the new instanceof syntax is unfortunate.
I'm wondering whether the release level for the spark module should also be Java 8 ort 11 instead of 17 given that apparently a significant number of Spark users are on old Java versions.
I'm also wondering after looking into what jabel is supposed to do whether there is a better approach than rewriting so much code.
| case SELECTION: | ||
| return from(expr.getSelection()); | ||
| case SCALAR_FUNCTION: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces? or did we only need those due to the previous -> syntax?
There was a problem hiding this comment.
The braces scope the local variables to only that switch case; otherwise they are visible to following switch cases.
| case WINDOW_FUNCTION: | ||
| return fromWindowFunction(expr.getWindowFunction()); | ||
| case IF_THEN: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| return ExpressionCreator.ifThenStatement(from(ifThen.getElse()), clauses); | ||
| } | ||
| case SWITCH_EXPRESSION: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| from(switchExpr.getMatch()), from(switchExpr.getElse()), clauses); | ||
| } | ||
| case SINGULAR_OR_LIST: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| .build(); | ||
| } | ||
| case MULTI_OR_LIST: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| case INTERVAL_MINUTE: | ||
| case INTERVAL_MINUTE_SECOND: | ||
| case INTERVAL_SECOND: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| } | ||
|
|
||
| case ROW: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| } | ||
|
|
||
| case ARRAY: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| } else { | ||
| return FieldReference.newStructReference(fieldAccess.getField().getIndex(), expression); | ||
| case CORREL_VARIABLE: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
| case ITEM: | ||
| case INPUT_REF: | ||
| case FIELD_ACCESS: | ||
| { |
There was a problem hiding this comment.
do we still need those curly braces?
I played a bit with it and came up with this alternative: #443 |
bestbeforetoday
force-pushed
the
java-version
branch
from
d7009e9 to
650f8b8
Compare
When developing, the build (both on the command-line and in the IDE) frequently fails due to usage of unsupported Java language features for the target Java runtime. This is due to the the Java source version being set to a newer version than the target Java version. While this sometimes works, this combination is explicitly disallowed by the Java compiler. Source version must be equal to or less than the target version. This change uses the correct Java toolchain version for each sub-project. The exception being the core sub-project, since it targets Java 8 whereas ANTLR requires Java 11+ to run. Instead, the Java compiler release option is set to Java 8, ensuring that Java 8 bytecode is produced and that no APIs not present in Java 8 are used. While numerous code changes are required to adhere to the target Java language version, there are no functional changes. Signed-off-by: Mark S. Lewis <[email protected]>
Signed-off-by: Mark S. Lewis <[email protected]>
bestbeforetoday
force-pushed
the
java-version
branch
from
650f8b8 to
a435b66
Compare
|
Rebased on latest main branch content. |
When developing, the build (both on the command-line and in the IDE) frequently fails due to usage of unsupported Java language features for the target Java runtime. This is due to the the Java source version being set to a newer version than the target Java version. While this sometimes works, this combination is explicitly disallowed by the Java compiler. Source version must be equal to or less than the target version.
This change uses the correct Java toolchain version for each sub-project. The exception being the core sub-project, since it targets Java 8 whereas ANTLR requires Java 11+ to run. Instead, the Java compiler release option is set to Java 8, ensuring that Java 8 bytecode is produced and that no APIs not present in Java 8 are used.
While numerous code changes are required to adhere to the target Java language version, there are no functional changes.