SQL: add support for index aliases for SYS COLUMNS command #53525

astefan · 2020-03-13T00:03:32Z

Fixes #31609.

…o 31609_fix

elasticmachine · 2020-03-13T00:03:35Z

Pinging @elastic/es-search (:Search/SQL)

costin

Left some comments

costin · 2020-03-13T09:28:04Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

+                                    .local(true)
+                                    .aliases("*")
+                                    .indices(indicesList)
+                                    .indicesOptions(IndicesOptions.lenientExpandOpen());


The IndicesOption depends on whether includeFrozen is true or not.

costin · 2020-03-13T09:29:27Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

@@ -460,15 +463,31 @@ public void resolveAsSeparateMappings(String indexWildcard, String javaRegex, bo
        FieldCapabilitiesRequest fieldRequest = createFieldCapsRequest(indexWildcard, includeFrozen);
        client.fieldCaps(fieldRequest,
                ActionListener.wrap(


Maybe use static imports on ActionListener to make the code a bit more readable.

costin · 2020-03-13T09:32:03Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

@@ -460,15 +463,31 @@ public void resolveAsSeparateMappings(String indexWildcard, String javaRegex, bo
        FieldCapabilitiesRequest fieldRequest = createFieldCapsRequest(indexWildcard, includeFrozen);
        client.fieldCaps(fieldRequest,
                ActionListener.wrap(
-                        response -> listener.onResponse(
-                                separateMappings(typeRegistry, indexWildcard, javaRegex, response.getIndices(), response.get())),
+                        response -> {


Small nit - maybe it makes sense to extract this method into a separate one - it's not reusable but it might make the call easier to read:

client.fieldCaps(fieldRequest, wrap(response -> getIndexAliases(response, wrap(aliases -> ...))

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

costin · 2020-03-13T09:35:15Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

+        }
+
+        List<String> resolvedIndices = new ArrayList<>(asList(indexNames));
+        Map<String, Fields> indices = new LinkedHashMap<>(resolvedIndices.size() + resolvedAliases.size());


The size is incorrect - use CollectionUtils.mapSize

costin · 2020-03-13T09:36:08Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

            return emptyList();
        }

-        final List<String> resolvedIndices = asList(indexNames);
-        Map<String, Fields> indices = new LinkedHashMap<>(resolvedIndices.size());
+        Set<String> resolvedAliases = new LinkedHashSet<>();


Is it okay to put all aliases in the same map?

With this I wanted to count them and create a list of indices and aliases appropriately sized.

costin · 2020-03-13T09:39:58Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

+            ObjectObjectCursor<String, List<AliasMetaData>> index = iter.next();
+            for (AliasMetaData aliasMetaData : index.value) {
+                String aliasName = aliasMetaData.alias();
+                typesErrors.putIfAbsent(aliasName, new HashSet<>());


this creates a HashSet regardless of the key presence - better to do the check

Actually, this was intended like this. See this line where the field type for this field belonging to this alias is added to the list of unique field types. And here the size of the set is checked to see if there is more than one field type across all indices belonging to this alias. If there is more than one field type, then the field is considered "invalid" for this alias.

I don't follow - putIfAbsent does if (key == null) put(key, value) - I'm pointing out that a new HashSet is created every time.
What I'm suggesting:

if (key == null) { put(key, new HashSet()) }

I am aware of the putIfAbsent functionality. I think I can simplify the logic a bit.

…o 31609_fix

bpintea · 2020-03-16T19:23:55Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

+            return emptyMap();
+        }
+        Map<String, InvalidMappedField> invalidFields = new HashMap<>();
+        Map<String, Set<String>> typesErrors = new HashMap<>(); // map holding aliases and a list of unique field types accross its indices


nit: s/accross/across.

bpintea

LGTM

costin

LGTM. A second pass could improve the code further by moving the merging code into one place so that separate and mergeMappings have the same code flow since aliases, even without a pattern, force a mapping merge.

costin · 2020-03-16T21:49:42Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

            Function<String, String> indexNameProcessor,
            BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> validityVerifier) {

-        if (indexNames == null || indexNames.length == 0) {
+        if ((indexNames == null || indexNames.length == 0) && (aliases == null || aliases.size() == 0)) {


Use org.elasticsearch.common.util.CollectionUtils.isEmpty instead.

That's not possible unfortunately, because aliases is an ImmutableOpenMap, which is not a Collection.

…o 31609_fix

matriv

LGTM. Nicely done! Left 2 minor comments

matriv · 2020-03-17T00:13:22Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

+        if (aliases != null) {
+            Iterator<ObjectObjectCursor<String, List<AliasMetaData>>> iterator = aliases.iterator();
+            while (iterator.hasNext()) {
+                for(AliasMetaData alias : iterator.next().value) {


Suggested change

for(AliasMetaData alias : iterator.next().value) {

for (AliasMetaData alias : iterator.next().value) {

matriv · 2020-03-17T00:25:46Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

+            ObjectObjectCursor<String, List<AliasMetaData>> index = iter.next();
+            for (AliasMetaData aliasMetaData : index.value) {
+                String aliasName = aliasMetaData.alias();
+                aliasToIndices.putIfAbsent(aliasName, new HashSet<String>());


Can it be simply <>?

Suggested change

aliasToIndices.putIfAbsent(aliasName, new HashSet<String>());

aliasToIndices.putIfAbsent(aliasName, new HashSet<>());

…3525) (cherry picked from commit f65e4d6)

…53653) (cherry picked from commit f65e4d6)

astefan added 3 commits March 13, 2020 01:36

Add support for index aliases for SYS COLUMNS command

947e501

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

dabcccf

…o 31609_fix

Update to test after branch update

3aa01f3

astefan added >bug :Analytics/SQL SQL querying v8.0.0 v7.7.0 labels Mar 13, 2020

astefan requested review from costin, bpintea and matriv March 13, 2020 00:03

costin reviewed Mar 13, 2020

View reviewed changes

astefan added 3 commits March 16, 2020 15:41

Address reviews

26bf119

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

93d8162

…o 31609_fix

Fix one integration test after update branch

80f0b63

astefan requested a review from costin March 16, 2020 15:02

astefan added 2 commits March 16, 2020 18:40

Address further review

d34271a

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

bf8df94

…o 31609_fix

bpintea reviewed Mar 16, 2020

View reviewed changes

bpintea approved these changes Mar 16, 2020

View reviewed changes

costin approved these changes Mar 16, 2020

View reviewed changes

astefan added 2 commits March 17, 2020 07:45

Address more reviews

9ed8fa2

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

cd6c420

…o 31609_fix

matriv approved these changes Mar 17, 2020

View reviewed changes

Small updates

079be6b

astefan merged commit f65e4d6 into elastic:master Mar 17, 2020

astefan deleted the 31609_fix branch March 17, 2020 09:46

astefan added a commit to astefan/elasticsearch that referenced this pull request Mar 17, 2020

SQL: add support for index aliases for SYS COLUMNS command (elastic#5…

24a195a

…3525) (cherry picked from commit f65e4d6)

astefan added the backport pending label Mar 17, 2020

astefan mentioned this pull request Mar 17, 2020

SQL: add support for index aliases for SYS COLUMNS command (#53525) #53653

Merged

astefan added a commit that referenced this pull request Mar 17, 2020

SQL: add support for index aliases for SYS COLUMNS command (#53525) (#…

79600eb

…53653) (cherry picked from commit f65e4d6)

astefan added backport pending v7.6.2 and removed backport pending labels Mar 17, 2020

astefan mentioned this pull request Mar 17, 2020

SQL: add support for index aliases for SYS COLUMNS command #53672

Merged

astefan removed the backport pending label Mar 17, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: add support for index aliases for SYS COLUMNS command #53525

SQL: add support for index aliases for SYS COLUMNS command #53525

astefan commented Mar 13, 2020

elasticmachine commented Mar 13, 2020

costin left a comment

costin Mar 13, 2020

costin Mar 13, 2020

costin Mar 13, 2020

costin Mar 13, 2020

costin Mar 13, 2020

astefan Mar 13, 2020

costin Mar 13, 2020

astefan Mar 16, 2020

costin Mar 16, 2020

astefan Mar 16, 2020

bpintea Mar 16, 2020

bpintea left a comment

costin left a comment

costin Mar 16, 2020

astefan Mar 17, 2020

matriv left a comment

matriv Mar 17, 2020

matriv Mar 17, 2020

	for(AliasMetaData alias : iterator.next().value) {
	for (AliasMetaData alias : iterator.next().value) {

	aliasToIndices.putIfAbsent(aliasName, new HashSet<String>());
	aliasToIndices.putIfAbsent(aliasName, new HashSet<>());

SQL: add support for index aliases for SYS COLUMNS command #53525

SQL: add support for index aliases for SYS COLUMNS command #53525

Conversation

astefan commented Mar 13, 2020

elasticmachine commented Mar 13, 2020

costin 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

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

bpintea left a comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment