-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Changes from 8 commits
92609bf
b63a3e2
d6b17e9
1cc2e5d
c60e901
5b2a86b
d32b40b
5f4e6bb
83d1bbc
a06cb53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
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 <b>internally</b> by the scalar functions | ||
* (to simplify the whitelist definition). | ||
*/ | ||
public final class InternalSqlScriptUtils { | ||
|
||
private InternalSqlScriptUtils() {} | ||
|
||
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,35 @@ | ||
/* | ||
* 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<>(); | ||
List<Whitelist> list = singletonList(WHITELIST); | ||
whitelist.put(FilterScript.CONTEXT, list); | ||
whitelist.put(SearchScript.AGGS_CONTEXT, list); | ||
whitelist.put(SearchScript.CONTEXT, list); | ||
whitelist.put(SearchScript.SCRIPT_SORT_CONTEXT, list); | ||
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.InternalSqlScriptUtils { | ||
|
||
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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
InternalSqlScriptUtils.dateTimeChrono(doc['int'].value, 'UTC', 'YEAR')" | ||
lang: painless | ||
|
||
- match: { hits.total: 1 } | ||
- match: { hits.hits.0.fields.sNum1.0: 1970 } |
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.