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.

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

SQL: Fix issue with field names containing "." #37364

Merged
merged 11 commits into from
Jan 15, 2019
Merged

SQL: Fix issue with field names containing "." #37364

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
35213d6
SQL: Fix issue with field names containing "."
matriv Jan 11, 2019
e75a4e8
Added test
matriv Jan 11, 2019
266cd2b
Added test with randomness
matriv Jan 11, 2019
b38aa31
Address comments
matriv Jan 11, 2019
7c17a51
Added some comments to the algorithm
matriv Jan 11, 2019
eec5897
Adjust algorithm for common prefixes
matriv Jan 13, 2019
07fa7a8
Add test for same path and different hierarchy
matriv Jan 13, 2019
00e5c09
Handle multiple values in different path hiearchy
matriv Jan 14, 2019
c39978f
Merge remote-tracking branch 'upstream/master' into mt/fix-37128
matriv Jan 14, 2019
d8fdb93
Address comment
matriv Jan 14, 2019
818e48d
Address comment
matriv Jan 14, 2019
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
36 changes: 31 additions & 5 deletions ...c/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.sql.execution.search.extractor;

import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.document.DocumentField;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
Expand Down Expand Up @@ -39,7 +40,7 @@ public class FieldHitExtractor implements HitExtractor {
*/
private static String[] sourcePath(String name, boolean useDocValue, String hitName) {
return useDocValue ? Strings.EMPTY_ARRAY : Strings
.tokenizeToStringArray(hitName == null ? name : name.substring(hitName.length() + 1), ".");
.tokenizeToStringArray(hitName == null ? name : name.substring(hitName.length() + 1), ".");
Copy link
Member

Choose a reason for hiding this comment

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

There's definitely some formatting differences here.
Anyway, make sure you set your IDE to format only the lines that were modified as that prevents unnecessary changes like the above.

}

private final String fieldName, hitName;
Expand Down Expand Up @@ -144,19 +145,44 @@ Object extractFromSource(Map<String, Object> map) {
Object value = map;
boolean first = true;
// each node is a key inside the map
for (String node : path) {
for (int i = 0; i < path.length; i++) {
String node = path[i];
if (value == null) {
return null;
} else if (first || value instanceof Map) {
first = false;
value = ((Map<String, Object>) value).get(node);
map = ((Map<String, Object>) value);
value = map.get(node);
if (value == null) { // Try to extract field with dots (e.g.: "b.c")
Tuple<Object, Integer> tuple = extractAsDottedField(map, i, node);
value = tuple.v1();
if (value != null) {
if (value instanceof Map) {
i = tuple.v2();
} else {
return unwrapMultiValue(value);
}
}
}
} else {
throw new SqlIllegalArgumentException("Cannot extract value [{}] from source", fieldName);
}
}
return unwrapMultiValue(value);
}

private Tuple<Object, Integer> extractAsDottedField(Map<String, Object> map, int idx, String node) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking to remove this extracted code back to the body of extractFromSource() as now we create an extra Tuple and Integer object which happens for every document returned. What do you think?

Also the code might be a bit ugly but I wanted to avoid calling recursively a method (with every combination of a.b, a.b.c, etc.) Happy to hear better suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

+1. It looks like a small method and while it might be inlined the extra Tuple/Integer are boiler-plate.
If the method gets unrolled, both the index and the found value will be available without wrapping/boxing.

Object value = null;
StringBuilder sb = new StringBuilder(node);
int i = idx + 1;
while (value == null && i < path.length) {
sb.append(".").append(path[i]);
value = map.get(sb.toString());
i++;
}
return new Tuple<>(value, i - 1);
}

@Override
public String hitName() {
return hitName;
Expand All @@ -178,8 +204,8 @@ public boolean equals(Object obj) {
}
FieldHitExtractor other = (FieldHitExtractor) obj;
return fieldName.equals(other.fieldName)
&& hitName.equals(other.hitName)
&& useDocValue == other.useDocValue;
&& hitName.equals(other.hitName)
Copy link
Member

Choose a reason for hiding this comment

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

Again with the formatting...

&& useDocValue == other.useDocValue;
}

@Override
Expand Down
73 changes: 70 additions & 3 deletions ...t/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.StringJoiner;
import java.util.function.Supplier;

import static java.util.Arrays.asList;
Expand All @@ -47,7 +48,7 @@ protected Reader<FieldHitExtractor> instanceReader() {
}

@Override
protected FieldHitExtractor mutateInstance(FieldHitExtractor instance) throws IOException {
protected FieldHitExtractor mutateInstance(FieldHitExtractor instance) {
return new FieldHitExtractor(instance.fieldName() + "mutated", null, true, instance.hitName());
}

Expand Down Expand Up @@ -237,7 +238,73 @@ public void testMultiValuedSource() {
assertThat(ex.getMessage(), is("Arrays (returned by [a]) are not supported"));
}

public Object randomValue() {
public void testFieldWithDots() {
FieldHitExtractor fe = new FieldHitExtractor("a.b", null, false);
Object value = randomValue();
Map<String, Object> map = singletonMap("a.b", value);
assertEquals(value, fe.extractFromSource(map));
}

public void testNestedFieldWithDots() {
FieldHitExtractor fe = new FieldHitExtractor("a.b.c", null, false);
Object value = randomValue();
Map<String, Object> map = singletonMap("a", singletonMap("b.c", value));
assertEquals(value, fe.extractFromSource(map));
}

public void testNestedFieldWithDotsWithNestedField() {
FieldHitExtractor fe = new FieldHitExtractor("a.b.c.d", null, false);
Object value = randomValue();
Map<String, Object> map = singletonMap("a", singletonMap("b.c", singletonMap("d", value)));
assertEquals(value, fe.extractFromSource(map));
}

public void testNestedFieldWithDotsWithNestedFieldWithDots() {
FieldHitExtractor fe = new FieldHitExtractor("a.b.c.d.e", null, false);
Object value = randomValue();
Map<String, Object> map = singletonMap("a", singletonMap("b.c", singletonMap("d.e", value)));
assertEquals(value, fe.extractFromSource(map));
}

public void testNestedFieldsWithDotsAndRandomHiearachy() {
String[] path = new String[100];
StringJoiner sj = new StringJoiner(".");
for (int i = 0; i < 100; i++) {
path[i] = randomAlphaOfLength(randomIntBetween(1, 10));
sj.add(path[i]);
}
FieldHitExtractor fe = new FieldHitExtractor(sj.toString(), null, false);


List<String> paths = new ArrayList<>(path.length);
int start = 0;
while (start < path.length) {
int end = randomIntBetween(start + 1, path.length);
sj = new StringJoiner(".");
for (int j = start; j < end; j++) {
sj.add(path[j]);
}
paths.add(sj.toString());
start = end;
}

Object value = randomValue();
Map<String, Object> map = singletonMap(paths.get(paths.size() - 1), value);
for (int i = paths.size() - 2; i >= 0; i--) {
map = singletonMap(paths.get(i), map);
}
assertEquals(value, fe.extractFromSource(map));
}

public void testExtractSourceIncorrectPathWithFieldWithDots() {
FieldHitExtractor fe = new FieldHitExtractor("a.b.c.d.e", null, false);
Object value = randomNonNullValue();
Map<String, Object> map = singletonMap("a", singletonMap("b.c", singletonMap("d", value)));
SqlException ex = expectThrows(SqlException.class, () -> fe.extractFromSource(map));
assertThat(ex.getMessage(), is("Cannot extract value [a.b.c.d.e] from source"));
}

private Object randomValue() {
Supplier<Object> value = randomFrom(Arrays.asList(
() -> randomAlphaOfLength(10),
ESTestCase::randomLong,
Expand All @@ -246,7 +313,7 @@ public Object randomValue() {
return value.get();
}

public Object randomNonNullValue() {
private Object randomNonNullValue() {
Supplier<Object> value = randomFrom(Arrays.asList(
() -> randomAlphaOfLength(10),
ESTestCase::randomLong,
Expand Down