Skip to content

HIVE-28984: Replace nashorn-core with graalvm which is compatible wit… #5856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HIVE-28984: Replace nashorn-core with graalvm which is compatible wit… #5856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@
<hamcrest.version>1.3</hamcrest.version>
<hbase.version>2.5.6-hadoop3</hbase.version>
<hppc.version>0.7.2</hppc.version>
<nashorn.version>15.4</nashorn.version>
<graalvm.version>23.0.8</graalvm.version>
<!-- required for logging test to avoid including hbase which pulls disruptor transitively -->
<disruptor.version>3.3.7</disruptor.version>
<hikaricp.version>4.0.3</hikaricp.version>
Expand Down Expand Up @@ -420,9 +420,14 @@
<version>${commons-math3.version}</version>
</dependency>
<dependency>
<groupId>org.openjdk.nashorn</groupId>
<artifactId>nashorn-core</artifactId>
<version>${nashorn.version}</version>
<groupId>org.graalvm.js</groupId>
<artifactId>js-scriptengine</artifactId>
<version>${graalvm.version}</version>
</dependency>
<dependency>
<groupId>org.graalvm.js</groupId>
<artifactId>js</artifactId>
<version>${graalvm.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both libraries are licensed as UPL. It's fine as it's classified as Category A.

</dependency>
<dependency>
<groupId>io.jsonwebtoken</groupId>
Expand Down
8 changes: 6 additions & 2 deletions ql/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,12 @@
<!-- intra-project -->
<!-- used for vector code-gen -->
<dependency>
<groupId>org.openjdk.nashorn</groupId>
<artifactId>nashorn-core</artifactId>
<groupId>org.graalvm.js</groupId>
<artifactId>js-scriptengine</artifactId>
</dependency>
<dependency>
<groupId>org.graalvm.js</groupId>
<artifactId>js</artifactId>
</dependency>
<dependency>
<groupId>org.apache.atlas</groupId>
Expand Down
59 changes: 26 additions & 33 deletions ql/src/java/org/apache/hadoop/hive/ql/metadata/PartitionTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/
package org.apache.hadoop.hive.ql.metadata;

import com.oracle.truffle.js.scriptengine.GraalJSScriptEngine;
import org.apache.hadoop.hive.metastore.api.AlreadyExistsException;
import org.apache.hadoop.hive.metastore.api.GetPartitionsFilterSpec;
import org.apache.hadoop.hive.metastore.api.GetPartitionsRequest;
Expand All @@ -28,11 +29,8 @@
import org.apache.hadoop.hive.metastore.api.PartitionFilterMode;
import org.apache.hadoop.hive.metastore.api.PartitionListComposingSpec;
import org.apache.hadoop.hive.metastore.api.PartitionSpec;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.graalvm.polyglot.Context;

import javax.script.ScriptEngine;
import javax.script.ScriptEngineManager;
import javax.script.ScriptException;
import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -41,7 +39,6 @@
import java.util.List;
import java.util.Map;

import static org.apache.hadoop.hive.metastore.Warehouse.LOG;
import static org.apache.hadoop.hive.metastore.Warehouse.makePartName;
import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.makePartNameMatcher;

Expand All @@ -50,7 +47,6 @@
* via references.
*/
final class PartitionTree {
private static final Logger LOG = LoggerFactory.getLogger(PartitionTree.class);
private Map<String, org.apache.hadoop.hive.metastore.api.Partition> parts = new LinkedHashMap<>();
private final org.apache.hadoop.hive.metastore.api.Table tTable;

Expand Down Expand Up @@ -258,21 +254,20 @@ List<Partition> getPartitionsByFilter(final String filter) throws MetaException
return new ArrayList<>(parts.values());
}
List<Partition> result = new ArrayList<>();
ScriptEngine se = new ScriptEngineManager().getEngineByName("JavaScript");
if (se == null) {
LOG.error("JavaScript script engine is not found, therefore partition filtering "
+ "for temporary tables is disabled.");
return result;
}
for (Map.Entry<String, Partition> entry : parts.entrySet()) {
se.put("partitionName", entry.getKey());
se.put("values", entry.getValue().getValues());
try {
if ((Boolean)se.eval(filter)) {
result.add(entry.getValue());
try (GraalJSScriptEngine se = GraalJSScriptEngine.create(null,
Context.newBuilder().allowExperimentalOptions(true)
.option("js.nashorn-compat", "true")
.allowAllAccess(true))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm checking this document.
https://www.graalvm.org/latest/reference-manual/js/NashornMigrationGuide/

I guess allowAllAccess(true) is redundant: https://github.com/oracle/graaljs/blob/release/graal-vm/23.0/graal-js/src/com.oracle.truffle.js.scriptengine/src/com/oracle/truffle/js/scriptengine/GraalJSScriptEngine.java#L346

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@okumin thanks for the review.
i checked and debug the above code. so here we are setting builder.allowAllAccess(true) in the method updateForNashornCompatibilityMode(). But this method call when contextConfig is null. But in our current case we are setting flags related to nashorn compatibility("js.nashorn-compat") in Context config and calling method GraalJSScriptEngine.create(). So we will not hit the null case(see code below) and the method(updateForNashornCompatibilityMode) will not call. So here setting "allowAllAccess(true)" in Context builder is ok in my opinion. Please let me know your opinion on same. thank you !

        if (contextConfig == null) {      --> in our current case this will not be NULL.
            contextConfigToUse = Context.newBuilder(new String[]{"js"}).allowExperimentalOptions(true);
            contextConfigToUse.option("js.syntax-extensions", "true");
            contextConfigToUse.option("js.load", "true");
            contextConfigToUse.option("js.print", "true");
            contextConfigToUse.option("js.global-arguments", "true");
            contextConfigToUse.option("js.charset", "UTF-8");
            if (NASHORN_COMPATIBILITY_MODE) {
                updateForNashornCompatibilityMode(contextConfigToUse);
            } else if (Boolean.getBoolean("graaljs.insecure-scriptengine-access")) {
                updateForScriptEngineAccessibility(contextConfigToUse);
            }
        }

    private static void updateForNashornCompatibilityMode(Context.Builder builder) {
        builder.allowAllAccess(true);
        builder.allowHostAccess(NASHORN_HOST_ACCESS);
        builder.useSystemExit(true);
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll be right. What if .allowAllAccess(true) is removed? The document doesn't mention the option.

try (Context context = Context.newBuilder().allowExperimentalOptions(true).option("js.nashorn-compat", "true").build()) {
      context.eval("js", "print(__LINE__)");
  }

If GraalJS works with Hive without the option, it's better from the point of view of security.
https://www.graalvm.org/sdk/javadoc/org/graalvm/polyglot/Context.Builder.html#allowAllAccess(boolean)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i removed property .allowAllAccess(true) and checked it previously, we are getting error "failed due to: Unknown identifier: get". So in our case we need to pass this field. thanks !

for (Map.Entry<String, Partition> entry : parts.entrySet()) {
se.put("partitionName", entry.getKey());
se.put("values", entry.getValue().getValues());
try {
if ((Boolean) se.eval(filter)) {
result.add(entry.getValue());
}
} catch (ScriptException e) {
throw new MetaException("Incorrect partition filter");
}
} catch (ScriptException e) {
throw new MetaException("Incorrect partition filter");
}
}
return result;
Expand Down Expand Up @@ -311,19 +306,17 @@ GetPartitionsResponse getPartitionsWithSpecs(GetPartitionsRequest getPartitionsR
matches = filterSpec.getFilters().stream().anyMatch(str -> entry.getValue().getValues().contains(str));
break;
case BY_EXPR:
ScriptEngine se = new ScriptEngineManager().getEngineByName("JavaScript");
if (se == null) {
LOG.error("JavaScript script engine is not found, therefore partition filtering "
+ "for temporary tables is disabled.");
break;
}

for (String filter : filterSpec.getFilters()) {
try {
se.put("partition", partition);
matches = (Boolean) se.eval(filter);
} catch (ScriptException e) {
throw new MetaException("Error evaluating filter expression: " + e.getMessage());
try (GraalJSScriptEngine se = GraalJSScriptEngine.create(null,
Context.newBuilder().allowExperimentalOptions(true)
.option("js.nashorn-compat", "true")
.allowAllAccess(true))) {
for (String filter : filterSpec.getFilters()) {
try {
se.put("partition", partition);
matches = (Boolean) se.eval(filter);
} catch (ScriptException e) {
throw new MetaException("Error evaluating filter expression: " + e.getMessage());
}
}
}
break;
Expand Down
Loading