Skip to content

Commit

Permalink
use source lookup for analyzed columns in whereClause if inside funct…
Browse files Browse the repository at this point in the history
…ions

-> where substr(analyzed_text, 1,1) = 'x'

threw GroupByOnArrayUnsupported exception, now it works
correctly using source lookup
  • Loading branch information
mfussenegger committed Oct 30, 2014
1 parent bbf7fa5 commit a4db868
Show file tree
Hide file tree
Showing 24 changed files with 214 additions and 124 deletions.
6 changes: 3 additions & 3 deletions sql/src/main/java/io/crate/analyze/DataStatementAnalyzer.java
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ protected Symbol visitIsNotNullPredicate(IsNotNullPredicate node, T context) {

return context.allocateFunction(
NotPredicate.INFO,
ImmutableList.<Symbol>of(context.allocateFunction(isNullInfo, ImmutableList.of(argument))));
Arrays.<Symbol>asList(context.allocateFunction(isNullInfo, Arrays.asList(argument))));
}

@Override
Expand Down Expand Up @@ -458,7 +458,7 @@ protected Symbol visitIsNullPredicate(IsNullPredicate node, T context) {
new FunctionIdent(io.crate.operation.predicate.IsNullPredicate.NAME,
ImmutableList.of(DataTypeVisitor.fromSymbol((value))));
FunctionInfo functionInfo = context.getFunctionInfo(functionIdent);
return context.allocateFunction(functionInfo, ImmutableList.of(value));
return context.allocateFunction(functionInfo, Arrays.asList(value));
}

@Override
Expand Down Expand Up @@ -885,7 +885,7 @@ private void castTypes() {
left = DataTypeSymbol.toDataTypeSymbol(left, leftType);
if (rightType.isConvertableTo(leftType)) {
FunctionInfo functionInfo = CastFunctionResolver.functionInfo(rightType, leftType);
ImmutableList<Symbol> arguments = ImmutableList.of(right);
List<Symbol> arguments = Arrays.asList(right);
right = new Function(functionInfo, arguments);
} else {
throw new IllegalArgumentException(SymbolFormatter.format(
Expand Down
11 changes: 11 additions & 0 deletions sql/src/main/java/io/crate/lucene/LuceneQueryBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

package io.crate.lucene;

import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableMap;
import com.spatial4j.core.context.jts.JtsSpatialContext;
import com.spatial4j.core.shape.Rectangle;
Expand All @@ -30,7 +31,9 @@
import io.crate.analyze.WhereClause;
import io.crate.lucene.match.MatchQueryBuilder;
import io.crate.lucene.match.MultiMatchQueryBuilder;
import io.crate.metadata.DocReferenceConverter;
import io.crate.metadata.Functions;
import io.crate.metadata.ReferenceInfo;
import io.crate.operation.Input;
import io.crate.operation.collect.CollectInputSymbolVisitor;
import io.crate.operation.operator.*;
Expand Down Expand Up @@ -737,6 +740,14 @@ private Query genericFunctionQuery(Function function) {
raiseUnsupported(function);
}

DocReferenceConverter.convertIf(function, new Predicate<Reference>() {
@Override
public boolean apply(@javax.annotation.Nullable Reference input) {
assert input != null;
return input.info().indexType() == ReferenceInfo.IndexType.ANALYZED ||
input.info().indexType() == ReferenceInfo.IndexType.NO;
}
});
final CollectInputSymbolVisitor.Context ctx = inputSymbolVisitor.process(function);
assert ctx.topLevelInputs().size() == 1;
@SuppressWarnings("unchecked")
Expand Down

This file was deleted.

129 changes: 129 additions & 0 deletions sql/src/main/java/io/crate/metadata/DocReferenceConverter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/*
* Licensed to CRATE Technology GmbH ("Crate") under one or more contributor
* license agreements. See the NOTICE file distributed with this work for
* additional information regarding copyright ownership. Crate licenses
* this file to you under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. You may
* obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*
* However, if you have executed another commercial license agreement
* with Crate these terms will supersede the license and you may use the
* software solely pursuant to the terms of the relevant commercial agreement.
*/

package io.crate.metadata;

import com.google.common.base.Predicate;
import io.crate.metadata.doc.DocSchemaInfo;
import io.crate.metadata.doc.DocSysColumns;
import io.crate.metadata.table.TableInfo;
import io.crate.planner.symbol.*;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

/**
* Visitor to change regular column references into references using the DOC sys column.
*
* e.g. s.t.colname -> s.t._DOC['colname']
*/
public class DocReferenceConverter {

private final static SafeVisitor SAFE_VISITOR = new SafeVisitor();
private final static PredicateVisitor PREDICATE_VISITOR = new PredicateVisitor();

/**
* re-writes any references to source lookup ( n -> _doc['n'] )
*
* won't be converted: partition columns or non-doc-schema columns
*/
public static Symbol convertIfPossible(Symbol symbol, TableInfo tableInfo) {
return SAFE_VISITOR.process(symbol, tableInfo);
}

/**
* will convert any references that are analyzed or not indexed to doc-references
*/
public static Symbol convertIf(Symbol symbol, Predicate<Reference> predicate) {
return PREDICATE_VISITOR.process(symbol, predicate);
}

private static class Visitor<T> extends SymbolVisitor<T, Symbol> {

@Override
public Symbol visitFunction(Function symbol, T context) {
int idx = 0;
for (Symbol argument : symbol.arguments()) {
symbol.setArgument(idx, process(argument, context));
idx++;
}
return symbol;
}

@Override
public Symbol visitDynamicReference(DynamicReference symbol, T context) {
return visitReference(symbol, context);
}

@Override
protected Symbol visitSymbol(Symbol symbol, T context) {
return symbol;
}
}

private static Reference toSourceLookup(Reference reference) {
ReferenceIdent ident = reference.info().ident();
List<String> path = new ArrayList<>(ident.columnIdent().path());
if (path.isEmpty()) { // if it's empty it might be an empty immutableList
path = Arrays.asList(ident.columnIdent().name());
} else {
path.add(0, ident.columnIdent().name());
}
return new Reference(
new ReferenceInfo(
new ReferenceIdent(ident.tableIdent(), DocSysColumns.DOC.name(), path),
reference.info().granularity(),
reference.valueType()
)
);
}

private static class PredicateVisitor extends Visitor<Predicate<Reference>> {
@Override
public Symbol visitReference(Reference symbol, Predicate<Reference> predicate) {
if (predicate.apply(symbol)) {
return toSourceLookup(symbol);
}
return symbol;
}
}

private static class SafeVisitor extends Visitor<TableInfo> {

@Override
public Symbol visitReference(Reference symbol, TableInfo tableInfo) {
ReferenceIdent ident = symbol.info().ident();
if (ident.columnIdent().name().startsWith("_")) {
return symbol;
}
String schema = ident.tableIdent().schema();
if ((schema != null && !schema.equals(DocSchemaInfo.NAME)
|| tableInfo.partitionedByColumns().contains(symbol.info()))) {
return symbol;
}

return toSourceLookup(symbol);
}
}

private DocReferenceConverter() {}
}
3 changes: 3 additions & 0 deletions sql/src/main/java/io/crate/metadata/doc/DocIndexMetaData.java
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,9 @@ private ReferenceInfo.IndexType getColumnIndexType(String columnName,
&& analyzerName != null && !analyzerName.equals("keyword")) {
return ReferenceInfo.IndexType.ANALYZED;
}
} // default indexType is analyzed so need to check analyzerName if indexType is null
else if (analyzerName != null && !analyzerName.equals("keyword")) {
return ReferenceInfo.IndexType.ANALYZED;
}
return ReferenceInfo.IndexType.NOT_ANALYZED;
}
Expand Down
4 changes: 2 additions & 2 deletions sql/src/main/java/io/crate/planner/Planner.java
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ private void copyToPlan(CopyAnalysis analysis, Plan plan) {
if (analysis.outputSymbols() != null && !analysis.outputSymbols().isEmpty()) {
List<Symbol> columns = new ArrayList<>(analysis.outputSymbols().size());
for (Symbol symbol : analysis.outputSymbols()) {
columns.add(DocReferenceBuildingVisitor.convert(symbol, analysis.table()));
columns.add(DocReferenceConverter.convertIfPossible(symbol, analysis.table()));
}
contextBuilder = contextBuilder.output(columns);
projection.inputs(contextBuilder.outputs());
Expand Down Expand Up @@ -538,7 +538,7 @@ private void normalSelect(SelectAnalysis analysis, Plan plan, Context context) {
} else {
toCollect = new ArrayList<>();
for (Symbol symbol : contextBuilder.toCollect()) {
toCollect.add(DocReferenceBuildingVisitor.convert(symbol, analysis.table()));
toCollect.add(DocReferenceConverter.convertIfPossible(symbol, analysis.table()));
}
}

Expand Down
4 changes: 4 additions & 0 deletions sql/src/main/java/io/crate/planner/symbol/Function.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import io.crate.metadata.FunctionInfo;
import io.crate.types.DataType;
import org.elasticsearch.common.io.stream.StreamInput;
Expand All @@ -27,6 +28,9 @@ public Function(FunctionInfo info, List<Symbol> arguments) {
Preconditions.checkNotNull(info, "function info is null");
Preconditions.checkArgument(arguments.size() == info.ident().argumentTypes().size());
this.info = info;

assert arguments.isEmpty() || !(arguments instanceof ImmutableList) :
"must not be an immutable list - would break setArgument";
this.arguments = arguments;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void testSelectStarEmptyMapping() throws Exception {
@Test
public void testGroupByOnAnalyzedColumn() throws Exception {
expectedException.expect(SQLActionException.class);
expectedException.expectMessage("Column \"col1\" has a value that is an array. Group by doesn't work on Arrays");
expectedException.expectMessage("Cannot select analyzed column 'test1.col1' within grouping or aggregations");

execute("create table test1 (col1 string index using fulltext)");
ensureGreen();
Expand Down Expand Up @@ -2923,6 +2923,30 @@ public void testWhereColumnEqColumnAndFunctionEqFunction() throws Exception {
execute("select name from locations where substr(name, 1, 1) = substr(name, 1, 1)");
assertThat(response.rowCount(), is(12L));
}

@Test
public void testWhereFunctionWithAnalazyedColumnArgument() throws Exception {
execute("create table t (text string index using fulltext) " +
"clustered into 1 shards with (number_of_replicas = 0)");
ensureGreen();
execute("insert into t (text) values ('hello world')");
refresh();

execute("select text from t where substr(text, 1, 1) = 'h'");
assertThat(response.rowCount(), is(1L));
}

@Test
public void testWhereFunctionWithIndexOffColumn() throws Exception {
execute("create table t (text string index off) " +
"clustered into 1 shards with (number_of_replicas = 0)");
ensureGreen();
execute("insert into t (text) values ('hello world')");
refresh();

execute("select text from t where substr(text, 1, 1) = 'h'");
assertThat(response.rowCount(), is(1L));
}
}


Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public void testInformationSchemaTables() throws Exception {
FunctionImplementation eqImpl = functions.get(new FunctionIdent(EqOperator.NAME,
ImmutableList.<DataType>of(DataTypes.STRING, DataTypes.STRING)));
Function whereClause = new Function(eqImpl.info(),
ImmutableList.of(tableNameRef, Literal.newLiteral("shards")));
Arrays.asList(tableNameRef, Literal.newLiteral("shards")));

collectNode.whereClause(new WhereClause(whereClause));
collectNode.toCollect(toCollect);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@
*/
package io.crate.operation.operator;

import com.google.common.collect.ImmutableList;
import io.crate.planner.symbol.Function;
import io.crate.planner.symbol.Literal;
import io.crate.planner.symbol.Symbol;
import io.crate.types.DataTypes;
import org.apache.lucene.util.BytesRef;
import org.junit.Test;

import java.util.Arrays;

import static io.crate.operation.operator.LikeOperator.DEFAULT_ESCAPE;
import static org.junit.Assert.*;

Expand All @@ -39,7 +40,7 @@ private static Symbol normalizeSymbol(String expression, String pattern) {
);
Function function = new Function(
op.info(),
ImmutableList.<Symbol>of(Literal.newLiteral(expression), Literal.newLiteral(pattern))
Arrays.<Symbol>asList(Literal.newLiteral(expression), Literal.newLiteral(pattern))
);
return op.normalizeSymbol(function);
}
Expand Down
Loading

0 comments on commit a4db868

Please sign in to comment.