Skip to content
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.

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

SQL: Whitelist SQL utility class for better scripting #30681

Merged
merged 10 commits into from
Jun 13, 2018
3 changes: 2 additions & 1 deletion x-pack/plugin/sql/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ esplugin {
name 'x-pack-sql'
description 'The Elasticsearch plugin that powers SQL for Elasticsearch'
classname 'org.elasticsearch.xpack.sql.plugin.SqlPlugin'
extendedPlugins = ['x-pack-core']
extendedPlugins = ['x-pack-core', 'lang-painless']
}

configurations {
Expand All @@ -20,6 +20,7 @@ integTest.enabled = false

dependencies {
compileOnly "org.elasticsearch.plugin:x-pack-core:${version}"
compileOnly project(':modules:lang-painless')
compile project('sql-proto')
compile "org.elasticsearch.plugin:aggs-matrix-stats-client:${version}"
compile "org.antlr:antlr4-runtime:4.5.3"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.elasticsearch.xpack.sql.tree.NodeInfo;
import org.elasticsearch.xpack.sql.type.DataType;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;

import java.time.Instant;
import java.time.ZoneId;
Expand Down Expand Up @@ -52,8 +51,18 @@ public abstract class DateTimeFunction extends UnaryScalarFunction {
protected final NodeInfo<DateTimeFunction> info() {
return NodeInfo.create(this, ctorForInfo(), field(), timeZone());
}

protected abstract NodeInfo.NodeCtor2<Expression, TimeZone, DateTimeFunction> ctorForInfo();

@Override
protected TypeResolution resolveType() {
if (field().dataType() == DataType.DATE) {
return TypeResolution.TYPE_RESOLVED;
}
return new TypeResolution("Function [" + functionName() + "] cannot be applied on a non-date expression (["
+ Expressions.name(field()) + "] of type [" + field().dataType().esType + "])");
}

public TimeZone timeZone() {
return timeZone;
}
Expand All @@ -70,47 +79,24 @@ public Object fold() {
return null;
}

ZonedDateTime time = ZonedDateTime.ofInstant(
Instant.ofEpochMilli(folded.getMillis()), ZoneId.of(timeZone.getID()));
return time.get(chronoField());
return dateTimeChrono(folded.getMillis(), timeZone.getID(), chronoField().name());
}

@Override
protected TypeResolution resolveType() {
if (field().dataType() == DataType.DATE) {
return TypeResolution.TYPE_RESOLVED;
}
return new TypeResolution("Function [" + functionName() + "] cannot be applied on a non-date expression (["
+ Expressions.name(field()) + "] of type [" + field().dataType().esType + "])");
public static Integer dateTimeChrono(long millis, String tzId, String chronoName) {
ZonedDateTime time = ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), ZoneId.of(tzId));
return Integer.valueOf(time.get(ChronoField.valueOf(chronoName)));
}

@Override
protected ScriptTemplate asScriptFrom(FieldAttribute field) {
ParamsBuilder params = paramsBuilder();

String template = null;
if (TimeZone.getTimeZone("UTC").equals(timeZone)) {
// TODO: it would be nice to be able to externalize the extract function and reuse the script across all extractors
template = formatTemplate("doc[{}].value.get" + extractFunction() + "()");
params.variable(field.name());
} else {
// TODO ewwww
/*
* This uses the Java 8 time API because Painless doesn't whitelist creation of new
* Joda classes.
*
* The actual script is
* ZonedDateTime.ofInstant(Instant.ofEpochMilli(<insert doc field>.value.millis),
* ZoneId.of(<insert user tz>)).get(ChronoField.get(MONTH_OF_YEAR))
*/

template = formatTemplate("ZonedDateTime.ofInstant(Instant.ofEpochMilli(doc[{}].value.millis), "
+ "ZoneId.of({})).get(ChronoField.valueOf({}))");
params.variable(field.name())
.variable(timeZone.getID())
.variable(chronoField().name());
}

template = formatTemplate("{sql}.dateTimeChrono(doc[{}].value.millis, {}, {})");
params.variable(field.name())
.variable(timeZone.getID())
.variable(chronoField().name());

return new ScriptTemplate(template, params.build(), dataType());
}

Expand All @@ -120,10 +106,6 @@ protected ScriptTemplate asScriptFrom(AggregateFunctionAttribute aggregate) {
throw new UnsupportedOperationException();
}

protected String extractFunction() {
return getClass().getSimpleName();
}

/**
* Used for generating the painless script version of this function when the time zone is not UTC
*/
Expand Down Expand Up @@ -165,4 +147,4 @@ public boolean equals(Object obj) {
public int hashCode() {
return Objects.hash(field(), timeZone);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.DataTypes;
import org.elasticsearch.xpack.sql.util.StringUtils;

import java.util.List;
Expand Down Expand Up @@ -93,6 +92,6 @@ public String toString() {
}

public static String formatTemplate(String template) {
return template.replace("{}", "params.%s");
return template.replace("{sql}", "SqlScripts").replace("{}", "params.%s");
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just write SqlScripts rather than {sql}.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
package org.elasticsearch.xpack.sql.expression.function.scalar.whitelist;

import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction;

/**
* Whitelisted class for SQL scripts.
* Acts as a registry of the various static methods used internally by the scalar functions
* (to simplify the whitelist definition).
*/
public final class SqlScripts {

private SqlScripts() {}

public static Integer dateTimeChrono(long millis, String tzId, String chronoName) {
return DateTimeFunction.dateTimeChrono(millis, tzId, chronoName);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
package org.elasticsearch.xpack.sql.plugin;

import org.elasticsearch.painless.spi.PainlessExtension;
import org.elasticsearch.painless.spi.Whitelist;
import org.elasticsearch.painless.spi.WhitelistLoader;
import org.elasticsearch.script.FilterScript;
import org.elasticsearch.script.ScriptContext;
import org.elasticsearch.script.SearchScript;

import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

import static java.util.Collections.singletonList;

public class SqlPainlessExtension implements PainlessExtension {

private static final Whitelist WHITELIST = WhitelistLoader.loadFromResourceFiles(SqlPainlessExtension.class, "sql_whitelist.txt");

@Override
public Map<ScriptContext<?>, List<Whitelist>> getContextWhitelists() {
Map<ScriptContext<?>, List<Whitelist>> whitelist = new LinkedHashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to expose SQL functions to all queries though....

Copy link
Member

Choose a reason for hiding this comment

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

A bunch of us talked about this. While we'd prefer not to expose these all the other options are worse....

Could you switch this to a HashMap? I don't believe the ordering matters here.

whitelist.put(FilterScript.CONTEXT, singletonList(WHITELIST));
whitelist.put(SearchScript.CONTEXT, singletonList(WHITELIST));
whitelist.put(SearchScript.AGGS_CONTEXT, singletonList(WHITELIST));
return whitelist;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.elasticsearch.xpack.sql.plugin.SqlPainlessExtension
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#
# Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
# or more contributor license agreements. Licensed under the Elastic License;
# you may not use this file except in compliance with the Elastic License.
#

# This file contains a whitelist for SQL specific utilities available inside SQL scripting

class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.SqlScripts {

Integer dateTimeChrono(long, String, String)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
"SQL methods available in Painless":
- do:
bulk:
refresh: true
body:
- index:
_index: test
_type: doc
_id: 1
- str: test1
int: 1
- do:
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a fairly internal detail so we shouldn't have an integration test for it. I think the normal SQL integration tests cover this use case plenty so we don't need this anyway.

index: test
search:
body:
query:
match_all: {}
script_fields:
sNum1:
script:
source: "return
SqlScripts.dateTimeChrono(doc['int'].value, 'UTC', 'YEAR')"
lang: painless

- match: { hits.total: 1 }
- match: { hits.hits.0.fields.sNum1.0: 1970 }