SQL: Add text formatting support for multivalue #68606

bpintea · 2021-02-05T19:16:16Z

This adds the text formatting for multivalue fields (available through the ARRAY function),
that is:

CSV and TSV exports;
TXT formatting.

This adds the text formatting for multivalue doc fields, that is: - CSV and TSV exports - TXT formatting

matriv

Looks good, left 2 comments, a minor one and a question regarding the data type.

matriv · 2021-02-08T08:25:00Z

.../plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

@@ -1055,6 +1055,16 @@ private void executeQueryWithNextPage(String format, String expectedHeader, Stri
        assertEquals(0, getNumberOfSearchContexts(client(), "test"));
    }

+    public void testMultiValueQueryText() throws IOException {
+        index("{\"text\":[" + toJson("one") + "," + toJson("two, three") + "," + toJson("\"four\"") + "], \"number\" : [1, [2, 3], 4] }");


Why do you use toJson only for the values but not also for the field names?

~~C&p inertia. Thx, updated it.~~
Because of the spoteless formatting, which looks pretty bad. But I've applied it now anyways...

matriv · 2021-02-08T08:27:01Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java

+                }
+
+                String typeName = fromJava(o).typeName();
+                headers.add(new ColumnInfo("index", typeName + "_column", typeName));


I don't remember our take here, shouldn't the type name reveal that it's an array? something like byte_array or _array (postgres format) ?

You mean the 3rd argument should be typeName + "_array"? I've updated it anyways, to be clearer, but it's not used in these tests (the formatters don't look at the type, only content).

Thx, yep, I get, the columnInfo is not actually asserted, but it's clearer with the _array suffix. Thx!

- extend `toJson` use in one test; - add + `"_array"` in column def in one test.

- which is pretty ugly in this case.

matriv

LGTM. Thx!

astefan · 2021-02-09T07:57:37Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java

+
+    public void testCsvMultiValueIntegers() {
+        String text = CSV.format(req(), withData(singletonList(asList(
+            asList((byte)1, (byte)2), asList((short)3, (short)4), asList(5, 6), asList(7L, 8L)


Nitty picky: whitespace between the casting and the casted value.

astefan

LGTM

elasticmachine · 2021-02-09T10:19:22Z

Pinging @elastic/es-ql (Team:QL)

costin

LGTM

costin · 2021-02-09T10:23:58Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java

+            sb.append('[');
+            Collection<?> values = (Collection<?>) value;
+            values.forEach(x -> {
+                // quote strings and `\`-escape the `"` character inside them
+                if (x instanceof String) { // TODO: IPs should ideally not be quoted.
+                    sb.append('"');
+                    sb.append(((String) x).replace("\"", "\\\""));
+                    sb.append('"');
+                } else if (x == null) {
+                    sb.append("NULL");
+                } else {
+                    sb.append(toString(x, sqlVersion));
+                }
+                sb.append(',');
+            });
+            if (sb.length() > 1) { // strip trailing comma
+                sb.deleteCharAt(sb.length() - 1);
+            }
+            sb.append(']');
+            return sb.toString();
+        }


Better to use StringJoiner since it's part of the JDK and handles the exact case of prefix/suffix plus delimiter.

Nice, thanks, applied.

- white-space style; - s/StringBuilder/StringJoiner/

matriv · 2021-02-09T16:36:18Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java

@@ -169,27 +170,19 @@ public static String toString(Object value, SqlVersion sqlVersion) {

        // multivalue
        if (value instanceof Collection) {
-            final StringBuilder sb = new StringBuilder();
-            sb.append('[');
+            final StringJoiner sj = new StringJoiner(",", "[", "]");


palesz

LGTM

palesz · 2021-02-09T20:23:03Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java

+                if (x instanceof String) { // TODO: IPs should ideally not be quoted.
+                    sj.add('"' + ((String) x).replace("\"", "\\\"") + '"');
+                } else if (x == null) {
+                    sj.add("NULL");


Nitpicky: we return uppercase NULL, but here we return lowercase null for simple null objects.

Sorry, GH only updated the page with your comment after I hit merge.
The link in the comment doesn't seem to resolve correctly, but I assume you refer to the first if in the function?
I followed Posgresql's capitalised formatting for lists. Generally (and debatable) nulls are capitalised in textual formats, so I'd be in favour for updating the listing of simple null objects.

As extra context:
The NULL would appear "alone" in CSVs and TSVs -- a simple null object wouldn't be output at all in these formats (they'd be represented by two consecutive commas or tabs).
In CLI, the NULL would also appear to the letter in arrays output and the CLI chooses how to represent JSON nulls by itself (which is currently small lettered).
In txt formatting output the difference would be visible and "uncontrollable".

* Add text formatting support for multivalue This adds the text formatting for multivalue doc fields, that is: - CSV and TSV exports - TXT formatting

Add text formatting support for multivalue

f118767

This adds the text formatting for multivalue doc fields, that is: - CSV and TSV exports - TXT formatting

bpintea marked this pull request as ready for review February 8, 2021 08:13

bpintea requested review from matriv, costin, astefan and palesz February 8, 2021 08:13

matriv reviewed Feb 8, 2021

View reviewed changes

bpintea mentioned this pull request Feb 8, 2021

SQL: add multivalue field support (arrays) #68066

Closed

6 tasks

bpintea added 3 commits February 8, 2021 12:43

Merge branch 'feat/sql-multivalue' into feat/multivalue_text

1ac7972

Adress review comments

fc3fe72

- extend `toJson` use in one test; - add + `"_array"` in column def in one test.

Apply spotless formatting

37c3c19

- which is pretty ugly in this case.

matriv approved these changes Feb 8, 2021

View reviewed changes

astefan reviewed Feb 9, 2021

View reviewed changes

astefan approved these changes Feb 9, 2021

View reviewed changes

bpintea added the :Analytics/SQL SQL querying label Feb 9, 2021

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Feb 9, 2021

bpintea added the v8.0.0 label Feb 9, 2021

costin approved these changes Feb 9, 2021

View reviewed changes

Address review comments

4f16e1b

- white-space style; - s/StringBuilder/StringJoiner/

matriv reviewed Feb 9, 2021

View reviewed changes

bpintea added 2 commits February 9, 2021 18:18

Merge branch 'feat/sql-multivalue' into feat/multivalue_text

5e5ffb9

Merge branch 'feat/sql-multivalue' into feat/multivalue_text

78a39df

palesz approved these changes Feb 9, 2021

View reviewed changes

bpintea merged commit 0d40d65 into elastic:feat/sql-multivalue Feb 9, 2021

bpintea deleted the feat/multivalue_text branch February 9, 2021 20:55

bpintea added a commit that referenced this pull request Feb 11, 2021

SQL: Add text formatting support for multivalue (#68606)

859e8e0

* Add text formatting support for multivalue This adds the text formatting for multivalue doc fields, that is: - CSV and TSV exports - TXT formatting

bpintea added a commit that referenced this pull request Mar 16, 2021

SQL: Add text formatting support for multivalue (#68606)

89547e7

* Add text formatting support for multivalue This adds the text formatting for multivalue doc fields, that is: - CSV and TSV exports - TXT formatting

bpintea added a commit that referenced this pull request Mar 25, 2021

SQL: Add text formatting support for multivalue (#68606)

57ee8bc

* Add text formatting support for multivalue This adds the text formatting for multivalue doc fields, that is: - CSV and TSV exports - TXT formatting

pugnascotia added the >enhancement label Mar 31, 2021

bpintea added a commit that referenced this pull request May 20, 2021

SQL: Add text formatting support for multivalue (#68606)

760c5f0

* Add text formatting support for multivalue This adds the text formatting for multivalue doc fields, that is: - CSV and TSV exports - TXT formatting

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

SQL: Add text formatting support for multivalue #68606

SQL: Add text formatting support for multivalue #68606

Uh oh!

Conversation

bpintea commented Feb 5, 2021

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 9, 2021

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palesz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bpintea Feb 8, 2021 •

edited

Loading