SQL: Added support for string manipulating functions with more than one parameter #32356

astefan · 2018-07-25T09:40:42Z

This PR is the second part fix for #31604. It covers all the functions with more than one parameter: CONCAT, LEFT, RIGHT, REPEAT, POSITION, LOCATE, REPLACE, SUBSTRING, INSERT.

…rameter: CONCAT, LEFT, RIGHT, REPEAT, POSITION, LOCATE, REPLACE, SUBSTRING, INSERT

elasticmachine · 2018-07-25T09:40:43Z

Pinging @elastic/es-search-aggs

nik9000 · 2018-07-26T17:36:42Z

...java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java

+import static org.elasticsearch.xpack.sql.expression.function.scalar.script.ScriptTemplate.formatTemplate;
+
+/**
+ * A binary string function that abstracts away the second parameter of a binary string function and its result.


I don't think "abstracts away" is quite right here.

nik9000 · 2018-07-26T17:37:41Z

...java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java

+/**
+ * A binary string function that abstracts away the second parameter of a binary string function and its result.
+ * This class is the base for binary functions that have the first parameter a string, the second parameter a number
+ * or a string and the result can be a string or a number.


The javadoc says the result can be a string or a number but the dataType function below is always a KEYWORD which makes me think it is always a string.

Good catch. dataType should be implemented in the concrete classes where the specific type should be specified.

nik9000 · 2018-07-26T17:39:14Z

...java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java

+
+    @Override
+    public int hashCode() {
+        return Objects.hash(left(), right(), operation());


I don't think operation belongs in equals or hashCode.

nik9000 · 2018-07-26T17:40:25Z

...java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java

+        super(location, left, right);
+    }
+
+    protected abstract BiFunction<String, T, R> operation();


Should this be protected abstract R operation(String, R)?

Ah, I see why this is a function ref - so that the toString generates the right method to invoke. That feels a little brittle but I understand what is up.

Maybe add some javadoc here explaining this.

nik9000 · 2018-07-26T17:47:15Z

...g/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringStringProcessor.java

+    }
+
+    public enum BinaryStringStringOperation implements BiFunction<String, String, Number> {
+        //not implemented yet. Relies on soundex similarity for computation


Maybe we should throw an exception here rather than return 0? Do we really plan on implementing this? I don't think we should drag the phonetic analysis plugin as a depndency of SQL. I worry that implementing these as scripts is fairly likely to send the wrong message about how to do fancy searching with Elasticsearch-SQL. Namely, you should use Elasticsearch queries for fancy string searches rather than SQL things.

You're right @nik9000 . We discussed about this before and the fancy searches should be done with Elasticsearch directly. I'll remove the function from there.

nik9000 · 2018-07-26T17:52:16Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java

+
+        int startInt = ((Number) start).intValue() - 1;
+        int realStart = startInt < 0 ? 0 : startInt;
+        StringBuilder sb = new StringBuilder(source instanceof String ? (String) source : ((Character) source).toString());


Same deal as above with the instanceof

nik9000 · 2018-07-26T17:53:24Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java

+        StringBuilder sb = new StringBuilder(source instanceof String ? (String) source : ((Character) source).toString());
+        String replString = (replacement instanceof String ? (String) replacement : ((Character) replacement).toString());
+
+        if (startInt > sb.length()) {


I think it is worth double checking the bounds here because you already subtract one from it. It is sneaky enough that we need to have a tests case for it.

Also maybe check this before building sb and stuff.

@nik9000 good point about building sb AFTER the possible return.
But I'm failing to see the issue with startInts comparison with the length. Indeed, startInt can be negative, but when it happens realStart (the one used with StringBuilder) will be 0 and it shouldn't be an issue I think...

nik9000 · 2018-07-26T17:54:34Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java

+            return source;
+        }
+
+        return sb.replace(realStart, 


Do you need sb at all, actually? I think String.replace works fine here.

@nik9000 not sure String.replace will work here. Strings method is about replacing a certain sequence with another one in the given string. And doing this for each matching substring. The SQL insert function is about replacing whatever characters (not having a certain template to search for) are found starting at a certain position within the target string, for length characters and put back in the replacement string.

nik9000 · 2018-07-26T18:01:03Z

x-pack/qa/sql/src/main/resources/functions.csv-spec

@@ -28,3 +28,366 @@ len:i          |  first_name:s
 72             |Prasadram
 88             |Sreekrishna
 ;
+
+selectConcatWithOrderBy
+SELECT first_name f, last_name l, CONCAT(first_name,last_name) cct FROM test_emp ORDER BY CONCAT(first_name,last_name) LIMIT 10;


CONCAT(first_name, ' ', last_name)`? Feels a little more like what folks will do.

nik9000 · 2018-07-26T18:01:33Z

x-pack/qa/sql/src/main/resources/functions.csv-spec

+selectAsciiOfConcatWithGroupByOrderByCount
+SELECT ASCII(CONCAT("first_name","last_name")) ascii, COUNT(*) count FROM "test_emp" GROUP BY ASCII(CONCAT("first_name","last_name")) ORDER BY ASCII(CONCAT("first_name","last_name")) DESC LIMIT 10;
+
+ascii: i | count: l  


Do we usually have the space between the : and the type?

…earch for concat function, small refactoring in the binary string functions.

astefan · 2018-07-28T01:27:40Z

@nik9000 thank you for the valuable feedback. I've made another commit with some changes.

imotov

I think you need to merge in master to get the BWC tests to pass. I left a few nitpicking comments, but otherwise LGTM.

imotov · 2018-07-30T15:13:22Z

...g/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringStringProcessor.java

+
+import java.io.IOException;
+import java.util.function.BiFunction;
+


Javadocs would be really appreciated here.

imotov · 2018-07-30T15:14:06Z

.../elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringNumericProcessor.java

+
+import java.io.IOException;
+import java.util.function.BiFunction;
+


Javadocs would be helpful here.

imotov · 2018-07-30T15:24:36Z

x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt

+  String substring(String, int, int)
+  String replace(String, String, String)
+  Integer locate(String,String)
+  Integer locate(String,String,Integer)


Nitpick: could you add spaces after commas.

astefan · 2018-07-31T12:51:21Z

@nik9000 @imotov thank you for your reviews. I've incorporated most of your comments in the follow up commits.

nik9000

LGTM

nik9000 · 2018-07-31T14:01:37Z

...java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java

+        super(location, left, right);
+    }
+
+    protected abstract BiFunction<String, T, R> operation();


Maybe add some javadoc here explaining this.

…class

…ne parameter (#32356) Added support for string manipulating functions with more than one parameter: CONCAT, LEFT, RIGHT, REPEAT, POSITION, LOCATE, REPLACE, SUBSTRING, INSERT

@throws

* 6.x: Fix scriptdocvalues tests with dates Correct minor typo in explain.asciidoc for HLRC Fix painless whitelist and warnings from backporting #31441 Build: Add elastic maven to repos used by BuildPlugin (#32549) Scripting: Conditionally use java time api in scripting (#31441) [ML] Improve error when no available field exists for rule scope (#32550) [ML] Improve error for functions with limited rule condition support (#32548) [ML] Remove multiple_bucket_spans [ML] Fix thread leak when waiting for job flush (#32196) (#32541) Painless: Clean Up PainlessField (#32525) Add @AwaitsFix for #32554 Remove broken @link in Javadoc Add AwaitsFix to failing test - see #32546 SQL: Added support for string manipulating functions with more than one parameter (#32356) [DOCS] Reloadable Secure Settings (#31713) Fix compilation error introduced by #32339 [Rollup] Remove builders from TermsGroupConfig (#32507) Use hostname instead of IP with SPNEGO test (#32514) Switch x-pack rolling restart to new style Requests (#32339) [DOCS] Small fixes in rule configuration page (#32516) Painless: Clean up PainlessMethod (#32476) SQL: Add test for handling of partial results (#32474) Docs: Add missing migration doc for logging change Build: Remove shadowing from benchmarks (#32475) Docs: Add all JDKs to CONTRIBUTING.md Logging: Make node name consistent in logger (#31588) High-level client: fix clusterAlias parsing in SearchHit (#32465) REST high-level client: parse back _ignored meta field (#32362) backport fix of reduceRandom fix (#32508) Add licensing enforcement for FIPS mode (#32437) INGEST: Clean up Java8 Stream Usage (#32059) (#32485) Improve the error message when an index is incompatible with field aliases. (#32482) Mute testFilterCacheStats Scripting: Fix painless compiler loader to know about context classes (#32385) [ML][DOCS] Fix typo applied_to => applies_to Mute SSLTrustRestrictionsTests on JDK 11 Changed ReindexRequest to use Writeable.Reader (#32401) Increase max chunk size to 256Mb for repo-azure (#32101) Mute KerberosAuthenticationIT fix no=>not typo (#32463) HLRC: Add delete watch action (#32337) Fix calculation of orientation of polygons (#27967) [Kerberos] Add missing javadocs (#32469) Fix missing JavaDoc for @throws in several places in KerberosTicketValidator. Make get all app privs requires "*" permission (#32460) Ensure KeyStoreWrapper decryption exceptions are handled (#32472) update rollover to leverage write-alias semantics (#32216) [Kerberos] Remove Kerberos bootstrap checks (#32451) Switch security to new style Requests (#32290) Switch security spi example to new style Requests (#32341) Painless: Add PainlessConstructor (#32447) Update Fuzzy Query docs to clarify default behavior re max_expansions (#30819) Remove > from Javadoc (fatal with Java 11) Tests: Fix convert error tests to use fixed value (#32415) IndicesClusterStateService should replace an init. replica with an init. primary with the same aId (#32374) auto-interval date histogram - 6.x backport (#32107) [CI] Mute DocumentSubsetReaderTests testSearch [TEST] Mute failing InternalEngineTests#testSeqNoAndCheckpoints TEST: testDocStats should always use forceMerge (#32450) TEST: Avoid deletion in FlushIT AwaitsFix IndexShardTests#testDocStats Painless: Add method type to method. (#32441) Remove reference to non-existent store type (#32418) [TEST] Mute failing FlushIT test Fix ordering of bootstrap checks in docs (#32417) Wrong discovery.type for azure in breaking changes (#32432) Mute ConvertProcessorTests failing tests TESTS: Move netty leak detection to paranoid level (#32354) (#32425) Upgrade to Lucene-7.5.0-snapshot-608f0277b0 (#32390) [Kerberos] Avoid vagrant update on precommit (#32416) TEST: Avoid triggering merges in FlushIT [DOCS] Fixes formatting of scope object in job resource Switch x-pack/plugin to new style Requests (#32327) Release requests in cors handle (#32410) Remove BouncyCastle dependency from runtime (#32402) Copy missing segment attributes in getSegmentInfo (#32396) Rest HL client: Add put license action (#32214) Docs: Correcting a typo in tophits (#32359) Build: Stop double generating buildSrc pom (#32408) Switch x-pack full restart to new style Requests (#32294) Painless: Clean Up PainlessClass Variables (#32380) [ML] Consistent pattern for strict/lenient parser names (#32399) Add Restore Snapshot High Level REST API Update update-settings.asciidoc (#31378) Introduce index store plugins (#32375) Rank-Eval: Reduce scope of an unchecked supression Make sure _forcemerge respects `max_num_segments`. (#32291)

* master: HLRC: Move commercial clients from XPackClient (#32596) Add cluster UUID to Cluster Stats API response (#32206) Security: move User to protocol project (#32367) [TEST] Test for shard failures, add debug to testProfileMatchesRegular Minor fix for javadoc (applicable for java 11). (#32573) Painless: Move Some Lookup Logic to PainlessLookup (#32565) TEST: Avoid merges in testSeqNoAndCheckpoints [Rollup] Remove builders from HistoGroupConfig (#32533) Mutes failing SQL string function tests due to #32589 fixed elements in array of produced terms (#32519) INGEST: Enable default pipelines (#32286) Remove cluster state initial customs (#32501) Mutes LicensingDocumentationIT due to #32580 [ML] Remove multiple_bucket_spans (#32496) [ML] Rename JobProvider to JobResultsProvider (#32551) Correct minor typo in explain.asciidoc for HLRC Build: Add elastic maven to repos used by BuildPlugin (#32549) Clarify the error message when a pipeline agg is used in the 'order' parameter. (#32522) Revert "[test] turn on host io cache for opensuse (#32053)" Enable packaging tests on suse boxes [ML] Improve error when no available field exists for rule scope (#32550) [ML] Improve error for functions with limited rule condition support (#32548) Painless: Clean Up PainlessField (#32525) Add @AwaitsFix for #32554 Remove broken @link in Javadoc Scripting: Conditionally use java time api in scripting (#31441) [ML] Fix thread leak when waiting for job flush (#32196) (#32541) Add AwaitsFix to failing test - see #32546 Core: Minor size reduction for AbstractComponent (#32509) SQL: Added support for string manipulating functions with more than one parameter (#32356) [DOCS] Reloadable Secure Settings (#31713) Watcher: Reenable HttpSecretsIntegrationTests#testWebhookAction test (#32456) [Rollup] Remove builders from TermsGroupConfig (#32507) Use hostname instead of IP with SPNEGO test (#32514) Switch x-pack rolling restart to new style Requests (#32339) NETWORKING: Fix Netty Leaks by upgrading to 4.1.28 (#32511) [DOCS] Small fixes in rule configuration page (#32516) Painless: Clean up PainlessMethod (#32476) Build: Remove shadowing from benchmarks (#32475) Docs: Add all JDKs to CONTRIBUTING.md Add licensing enforcement for FIPS mode (#32437) SQL: Add test for handling of partial results (#32474) Mute testFilterCacheStats [ML][DOCS] Fix typo applied_to => applies_to Scripting: Fix painless compiler loader to know about context classes (#32385)

Added support for string manipulating functions with more than one pa…

3a7e81b

…rameter: CONCAT, LEFT, RIGHT, REPEAT, POSITION, LOCATE, REPLACE, SUBSTRING, INSERT

astefan added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.4.0 labels Jul 25, 2018

astefan requested review from costin, pcsanwald, nik9000 and imotov July 25, 2018 09:40

nik9000 reviewed Jul 26, 2018

View reviewed changes

PR review changes: another test for the insert function, additional s…

d23edee

…earch for concat function, small refactoring in the binary string functions.

imotov reviewed Jul 30, 2018

View reviewed changes

astefan added 2 commits July 31, 2018 07:22

Incorporated PR review comments.

d212a0c

Merge remote-tracking branch 'remotes/origin/master' into 31604_2nd_part

7b6e8c5

nik9000 approved these changes Jul 31, 2018

View reviewed changes

Clarified the role of the BiFunction inside the BinaryStringFunction …

8b3bca0

…class

astefan merged commit 4c38853 into elastic:master Aug 1, 2018

astefan added v6.5.0 and removed v6.4.0 labels Aug 1, 2018

astefan mentioned this pull request Aug 1, 2018

SQL: Implement missing text manipulating scalar functions #31604

Closed

19 tasks

This was referenced Aug 1, 2018

Broken @link in Javadocs #32553

Closed

[CI] LocateFunctionProcessorDefinitionTests.testTransform fails reproducibly #32554

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Added support for string manipulating functions with more than one parameter #32356

SQL: Added support for string manipulating functions with more than one parameter #32356

astefan commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

nik9000 Jul 26, 2018

nik9000 Jul 26, 2018

astefan Jul 28, 2018

nik9000 Jul 26, 2018

nik9000 Jul 26, 2018

nik9000 Jul 26, 2018

nik9000 Jul 31, 2018

nik9000 Jul 26, 2018

astefan Jul 28, 2018

nik9000 Jul 26, 2018

nik9000 Jul 26, 2018

nik9000 Jul 26, 2018

astefan Jul 27, 2018

nik9000 Jul 26, 2018

astefan Jul 27, 2018

nik9000 Jul 26, 2018

nik9000 Jul 26, 2018

astefan commented Jul 28, 2018

imotov left a comment

imotov Jul 30, 2018

imotov Jul 30, 2018

imotov Jul 30, 2018

astefan commented Jul 31, 2018

nik9000 left a comment

nik9000 Jul 31, 2018


		import java.io.IOException;
		import java.util.function.BiFunction;

SQL: Added support for string manipulating functions with more than one parameter #32356

SQL: Added support for string manipulating functions with more than one parameter #32356

Conversation

astefan commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Jul 28, 2018

imotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Jul 31, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment