Skip to content

Commit

Permalink
Removed runtime state from RelationAnalyzer since this is a singleton.
Browse files Browse the repository at this point in the history 
This lead to race conditions since the nested contexts have been shared.
  • Loading branch information
@dobe
dobe committed Feb 18, 2015
1 parent 0da45f0 commit cd877e0
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 51 deletions.
3 changes: 3 additions & 0 deletions CHANGES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ Changes for Crate
Unreleased
==========

- Fixed a race condition where some statements would break because of
using a shared state internally.

- Made zero length string routing consistent and route by the actual
hash of an empty string instead of handling it as null. CAUTION: If
you are using emtpy strings as routing values, affected records
Expand Down
3 changes: 2 additions & 1 deletion sql/src/main/java/io/crate/analyze/DeleteStatementAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ private static class InnerAnalysisContext {
public AnalyzedStatement visitDelete(Delete node, Analysis context) {
int numNested = 1;

RelationAnalysisContext relationAnalysisContext = new RelationAnalysisContext(context.parameterContext());
RelationAnalysisContext relationAnalysisContext = new RelationAnalysisContext(
context.parameterContext(), analysisMetaData);

AnalyzedRelation analyzedRelation = relationAnalyzer.process(node.getRelation(), relationAnalysisContext);
if (Relations.isReadOnly(analyzedRelation)) {
Expand Down
4 changes: 1 addition & 3 deletions sql/src/main/java/io/crate/analyze/SelectStatementAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
package io.crate.analyze;

import io.crate.analyze.relations.QueriedRelation;
import io.crate.analyze.relations.RelationAnalysisContext;
import io.crate.analyze.relations.RelationAnalyzer;
import io.crate.sql.tree.DefaultTraversalVisitor;
import io.crate.sql.tree.Query;
Expand All @@ -42,8 +41,7 @@ public SelectStatementAnalyzer(RelationAnalyzer relationAnalyzer) {
@Override
protected SelectAnalyzedStatement visitQuery(Query node, Analysis analysis) {
// TODO: make RelationAnalyzer a singleton
RelationAnalysisContext relationAnalysisContext = new RelationAnalysisContext(analysis.parameterContext());
QueriedRelation relation = (QueriedRelation) relationAnalyzer.process(node.getQueryBody(), relationAnalysisContext);
QueriedRelation relation = (QueriedRelation) relationAnalyzer.analyze(node.getQueryBody(), analysis);
analysis.rootRelation(relation);
return new SelectAnalyzedStatement(relation);
}
Expand Down
3 changes: 2 additions & 1 deletion sql/src/main/java/io/crate/analyze/UpdateStatementAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ public AnalyzedStatement analyze(Node node, Analysis analysis) {

@Override
public AnalyzedStatement visitUpdate(Update node, Analysis analysis) {
RelationAnalysisContext relationAnalysisContext = new RelationAnalysisContext(analysis.parameterContext());
RelationAnalysisContext relationAnalysisContext = new RelationAnalysisContext(
analysis.parameterContext(), analysisMetaData);
AnalyzedRelation analyzedRelation = relationAnalyzer.analyze(node.relation(), relationAnalysisContext);
if (Relations.isReadOnly(analyzedRelation)) {
throw new UnsupportedOperationException(String.format(
Expand Down
30 changes: 28 additions & 2 deletions sql/src/main/java/io/crate/analyze/relations/RelationAnalysisContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@

package io.crate.analyze.relations;

import io.crate.analyze.AnalysisMetaData;
import io.crate.analyze.ParameterContext;
import io.crate.analyze.expressions.ExpressionAnalysisContext;
import io.crate.analyze.expressions.ExpressionAnalyzer;
import io.crate.sql.tree.QualifiedName;

import java.util.Arrays;
Expand All @@ -30,12 +33,17 @@

public class RelationAnalysisContext {

private final ExpressionAnalysisContext expressionAnalysisContext;
private ExpressionAnalyzer expressionAnalyzer;
private Map<QualifiedName, AnalyzedRelation> sources = new HashMap<>();
private ParameterContext parameterContext;
private AnalysisMetaData analysisMetaData;
private FullQualifedNameFieldProvider fieldProvider;

public RelationAnalysisContext(ParameterContext parameterContext) {

public RelationAnalysisContext(ParameterContext parameterContext, AnalysisMetaData analysisMetaData) {
this.parameterContext = parameterContext;
this.analysisMetaData = analysisMetaData;
this.expressionAnalysisContext = new ExpressionAnalysisContext();
}

public ParameterContext parameterContext() {
Expand All @@ -54,4 +62,22 @@ public Map<QualifiedName, AnalyzedRelation> sources() {
return sources;
}

public ExpressionAnalyzer expressionAnalyzer(){
if (expressionAnalyzer == null){
expressionAnalyzer = new ExpressionAnalyzer(analysisMetaData, parameterContext(),
fieldProvider());
}
return expressionAnalyzer;
}

FieldProvider fieldProvider(){
if (fieldProvider == null){
fieldProvider = new FullQualifedNameFieldProvider(sources());
}
return fieldProvider;
}

public ExpressionAnalysisContext expressionAnalysisContext() {
return expressionAnalysisContext;
}
}
62 changes: 28 additions & 34 deletions sql/src/main/java/io/crate/analyze/relations/RelationAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.common.collect.Multimap;
import io.crate.analyze.*;
import io.crate.analyze.expressions.ExpressionAnalysisContext;
import io.crate.analyze.expressions.ExpressionAnalyzer;
import io.crate.analyze.relations.select.SelectAnalyzer;
import io.crate.analyze.validator.GroupBySymbolValidator;
import io.crate.analyze.validator.HavingSymbolValidator;
Expand Down Expand Up @@ -56,9 +55,6 @@ public class RelationAnalyzer extends DefaultTraversalVisitor<AnalyzedRelation,

private final AnalysisMetaData analysisMetaData;

private ExpressionAnalyzer expressionAnalyzer;
private ExpressionAnalysisContext expressionAnalysisContext;

@Inject
public RelationAnalyzer(AnalysisMetaData analysisMetaData) {
this.analysisMetaData = analysisMetaData;
Expand All @@ -70,7 +66,7 @@ public AnalyzedRelation analyze(Node node, RelationAnalysisContext relationAnaly
}

public AnalyzedRelation analyze(Node node, Analysis analysis){
return analyze(node, new RelationAnalysisContext(analysis.parameterContext()));
return analyze(node, new RelationAnalysisContext(analysis.parameterContext(), analysisMetaData));
}

@Override
Expand All @@ -90,23 +86,15 @@ protected AnalyzedRelation visitQuerySpecification(QuerySpecification node, Rela
throw new UnsupportedOperationException
("Only exactly one table is allowed in the FROM clause, got: " + context.sources().size());
}
FieldProvider fieldProvider = new FullQualifedNameFieldProvider(context.sources());
expressionAnalyzer = new ExpressionAnalyzer(analysisMetaData, context.parameterContext(), fieldProvider);
expressionAnalysisContext = new ExpressionAnalysisContext();
ExpressionAnalysisContext expressionAnalysisContext = context.expressionAnalysisContext();

WhereClause whereClause = analyzeWhere(node.getWhere());
WhereClause whereClause = analyzeWhere(node.getWhere(), context);
if (whereClause.hasQuery()) {
WhereClauseValidator whereClauseValidator = new WhereClauseValidator();
whereClauseValidator.validate(whereClause);
}
SelectAnalyzer.SelectAnalysis selectAnalysis = SelectAnalyzer.analyzeSelect(
node.getSelect(),
context.sources(),
expressionAnalyzer,
expressionAnalysisContext
);

List<Symbol> groupBy = analyzeGroupBy(selectAnalysis, node.getGroupBy());
SelectAnalyzer.SelectAnalysis selectAnalysis = SelectAnalyzer.analyzeSelect(node.getSelect(), context);
List<Symbol> groupBy = analyzeGroupBy(selectAnalysis, node.getGroupBy(), context);

if (!node.getGroupBy().isEmpty() || expressionAnalysisContext.hasAggregates) {
ensureNonAggregatesInGroupBy(selectAnalysis.outputSymbols(), groupBy);
Expand All @@ -116,10 +104,10 @@ protected AnalyzedRelation visitQuerySpecification(QuerySpecification node, Rela
}

QuerySpec querySpec = new QuerySpec()
.orderBy(analyzeOrderBy(selectAnalysis, node.getOrderBy()))
.having(analyzeHaving(node.getHaving(), groupBy, expressionAnalysisContext.hasAggregates))
.limit(expressionAnalyzer.integerFromExpression(node.getLimit()))
.offset(expressionAnalyzer.integerFromExpression(node.getOffset()))
.orderBy(analyzeOrderBy(selectAnalysis, node.getOrderBy(), context))
.having(analyzeHaving(node.getHaving(), groupBy, context))
.limit(context.expressionAnalyzer().integerFromExpression(node.getLimit()))
.offset(context.expressionAnalyzer().integerFromExpression(node.getOffset()))
.outputs(selectAnalysis.outputSymbols())
.where(whereClause)
.groupBy(groupBy)
Expand Down Expand Up @@ -200,7 +188,8 @@ public Boolean visitAggregation(Aggregation symbol, Void context) {
}

@Nullable
private OrderBy analyzeOrderBy(SelectAnalyzer.SelectAnalysis selectAnalysis, List<SortItem> orderBy) {
private OrderBy analyzeOrderBy(SelectAnalyzer.SelectAnalysis selectAnalysis, List<SortItem> orderBy,
RelationAnalysisContext context) {
int size = orderBy.size();
if (size==0){
return null;
Expand All @@ -212,7 +201,7 @@ private OrderBy analyzeOrderBy(SelectAnalyzer.SelectAnalysis selectAnalysis, Lis
for (int i = 0; i < size; i++) {
SortItem sortItem = orderBy.get(i);
Expression sortKey = sortItem.getSortKey();
Symbol symbol = symbolFromSelectOutputReferenceOrExpression(sortKey, selectAnalysis, "ORDER BY");
Symbol symbol = symbolFromSelectOutputReferenceOrExpression(sortKey, selectAnalysis, "ORDER BY", context);
SemanticSortValidator.validate(symbol);

symbols.add(symbol);
Expand All @@ -232,33 +221,36 @@ private OrderBy analyzeOrderBy(SelectAnalyzer.SelectAnalysis selectAnalysis, Lis
return new OrderBy(symbols, reverseFlags, nullsFirst);
}

private List<Symbol> analyzeGroupBy(SelectAnalyzer.SelectAnalysis selectAnalysis, List<Expression> groupBy) {
private List<Symbol> analyzeGroupBy(SelectAnalyzer.SelectAnalysis selectAnalysis, List<Expression> groupBy,
RelationAnalysisContext context) {
List<Symbol> groupBySymbols = new ArrayList<>(groupBy.size());
for (Expression expression : groupBy) {
Symbol symbol = symbolFromSelectOutputReferenceOrExpression(expression, selectAnalysis, "GROUP BY");
Symbol symbol = symbolFromSelectOutputReferenceOrExpression(
expression, selectAnalysis, "GROUP BY", context);
GroupBySymbolValidator.validate(symbol);
groupBySymbols.add(symbol);
}
return groupBySymbols;
}

private HavingClause analyzeHaving(Optional<Expression> having, List<Symbol> groupBy, boolean hasAggregates) {
private HavingClause analyzeHaving(Optional<Expression> having, List<Symbol> groupBy, RelationAnalysisContext context) {
if (having.isPresent()) {
if (!hasAggregates && groupBy.isEmpty()) {
if (!context.expressionAnalysisContext().hasAggregates && groupBy.isEmpty()) {
throw new IllegalArgumentException("HAVING clause can only be used in GROUP BY or global aggregate queries");
}
Symbol symbol = expressionAnalyzer.convert(having.get(), expressionAnalysisContext);
Symbol symbol = context.expressionAnalyzer().convert(having.get(), context.expressionAnalysisContext());
HavingSymbolValidator.validate(symbol, groupBy);
return new HavingClause(expressionAnalyzer.normalize(symbol));
return new HavingClause(context.expressionAnalyzer().normalize(symbol));
}
return null;
}

private WhereClause analyzeWhere(Optional<Expression> where) {
private WhereClause analyzeWhere(Optional<Expression> where, RelationAnalysisContext context) {
if (!where.isPresent()) {
return WhereClause.MATCH_ALL;
}
return new WhereClause(expressionAnalyzer.normalize(expressionAnalyzer.convert(where.get(), expressionAnalysisContext)));
return new WhereClause(context.expressionAnalyzer().normalize(
context.expressionAnalyzer().convert(where.get(), context.expressionAnalysisContext())));
}


Expand All @@ -279,7 +271,8 @@ private WhereClause analyzeWhere(Optional<Expression> where) {
*/
private Symbol symbolFromSelectOutputReferenceOrExpression(Expression expression,
SelectAnalyzer.SelectAnalysis selectAnalysis,
String clause) {
String clause,
RelationAnalysisContext context) {
Symbol symbol;
if (expression instanceof QualifiedNameReference) {
List<String> parts = ((QualifiedNameReference) expression).getName().getParts();
Expand All @@ -290,7 +283,7 @@ private Symbol symbolFromSelectOutputReferenceOrExpression(Expression expression
}
}
}
symbol = expressionAnalyzer.convert(expression, expressionAnalysisContext);
symbol = context.expressionAnalyzer().convert(expression, context.expressionAnalysisContext());
if (symbol.symbolType().isValueSymbol()) {
Literal longLiteral;
try {
Expand Down Expand Up @@ -333,7 +326,8 @@ private Symbol getOneOrAmbiguous(Multimap<String, Symbol> selectList, String key

@Override
protected AnalyzedRelation visitAliasedRelation(AliasedRelation node, RelationAnalysisContext context) {
AnalyzedRelation childRelation = process(node.getRelation(), new RelationAnalysisContext(context.parameterContext()));
AnalyzedRelation childRelation = process(node.getRelation(),
new RelationAnalysisContext(context.parameterContext(), analysisMetaData));
context.addSourceRelation(node.getAlias(), childRelation);
return childRelation;
}
Expand Down
17 changes: 7 additions & 10 deletions sql/src/main/java/io/crate/analyze/relations/select/SelectAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.crate.analyze.expressions.ExpressionAnalysisContext;
import io.crate.analyze.expressions.ExpressionAnalyzer;
import io.crate.analyze.relations.AnalyzedRelation;
import io.crate.analyze.relations.RelationAnalysisContext;
import io.crate.analyze.validator.SelectSymbolValidator;
import io.crate.metadata.OutputName;
import io.crate.planner.symbol.Field;
Expand All @@ -42,10 +43,8 @@ public class SelectAnalyzer {
private static final InnerVisitor INSTANCE = new InnerVisitor();

public static SelectAnalysis analyzeSelect(Select select,
Map<QualifiedName, AnalyzedRelation> sources,
ExpressionAnalyzer expressionAnalyzer,
ExpressionAnalysisContext expressionAnalysisContext) {
SelectAnalysis selectAnalysis = new SelectAnalysis(sources, expressionAnalyzer, expressionAnalysisContext);
RelationAnalysisContext context){
SelectAnalysis selectAnalysis = new SelectAnalysis(context);
INSTANCE.process(select, selectAnalysis);
SelectSymbolValidator.validate(selectAnalysis.outputSymbols);
return selectAnalysis;
Expand All @@ -60,12 +59,10 @@ public static class SelectAnalysis {
private List<Symbol> outputSymbols = new ArrayList<>();
private Multimap<String, Symbol> outputMultiMap = HashMultimap.create();

private SelectAnalysis(Map<QualifiedName, AnalyzedRelation> sources,
ExpressionAnalyzer expressionAnalyzer,
ExpressionAnalysisContext expressionAnalysisContext) {
this.sources = sources;
this.expressionAnalyzer = expressionAnalyzer;
this.expressionAnalysisContext = expressionAnalysisContext;
private SelectAnalysis(RelationAnalysisContext context) {
this.sources = context.sources();
this.expressionAnalyzer = context.expressionAnalyzer();
this.expressionAnalysisContext = context.expressionAnalysisContext();
}

public List<OutputName> outputNames() {
Expand Down

0 comments on commit cd877e0

Please sign in to comment.