Skip to content

Commit

Permalink
Upgrade to latest FE (#184)
Browse files Browse the repository at this point in the history 
- Upgrade to frontend `9.0.2018091`
- Since 9.0.8 front-end removes `normalizeReturnClauses` which created intermediate projection for ORDER, LIMIT e.t.c
- Intermediate projection is now created in CfoG translation
- Finalization moved to separate projection
- Rewriter that removes multiple projections if not necessary

Signed-off-by: Dwitry dwitry@users.noreply.github.com
  • Loading branch information
@dwitry
dwitry authored Oct 1, 2018
1 parent 1885a1e commit d870ab8
Show file tree
Hide file tree
Showing 7 changed files with 239 additions and 58 deletions.
2 changes: 1 addition & 1 deletion build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ buildscript {
guavaVersion = '22.0'
junitVersion = '4.12'
cypherVersion = '9.0'
cypherFrontendVersion = '9.0.7'
cypherFrontendVersion = '9.0.20180925'
scalaVersion = '2.11'
scalaPatchVersion = '12'
springBootVersion = '1.5.6.RELEASE'
Expand Down
78 changes: 78 additions & 0 deletions ...in/scala/org/opencypher/gremlin/translation/ir/rewrite/RemoveIntermediateProjection.scala
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright (c) 2018 "Neo4j, Inc." [https://neo4j.com]
*
* Licensed 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.
*/
package org.opencypher.gremlin.translation.ir.rewrite

import org.opencypher.gremlin.translation.ir.TraversalHelper._
import org.opencypher.gremlin.translation.ir.model._

/**
* This rule removes intermediate projection in case it does not have any logic, and followed by final projection
*/
object RemoveIntermediateProjection extends GremlinRewriter {

override def apply(steps: Seq[GremlinStep]): Seq[GremlinStep] = {
val traversals = split(BeforeStep, {
case Project(_*) => true
case _ => false
})(steps)

val size = traversals.size
if (size < 3) {
return steps
}

val intermediateProjection = traversals(size - 2)
val finalizationProjection = traversals.last

if (finalizationProjection.size == intermediateProjection.size
&& finalizationProjection.head == intermediateProjection.head
&& onlyContainsIdentityOrSelect(intermediateProjection)) {

val replacements = getReplacements(intermediateProjection)
val singleProjection = replaceInBy(finalizationProjection, replacements)

(traversals.take(size - 2) :+ singleProjection).flatten
} else {
steps
}
}

def onlyContainsIdentityOrSelect(steps: Seq[GremlinStep]): Boolean = !steps.exists {
case Project(_*) => false
case By(Identity :: Nil, None) => false
case By(SelectK(_) :: Nil, None) => false
case _ => true
}

def getReplacements(intermediateProjection: Seq[GremlinStep]): Map[GremlinStep, GremlinStep] = {
val Project(keys @ _*) = intermediateProjection.head
val valueSteps = intermediateProjection.tail.map {
case By(Identity :: Nil, None) => Identity
case By(SelectK(key) :: Nil, None) => SelectK(key)
}

val keySteps = keys.map(key => SelectK(key))

(keySteps zip valueSteps).toMap
}

def replaceInBy(projection: Seq[GremlinStep], replace: Map[GremlinStep, GremlinStep]): Seq[GremlinStep] =
projection.map {
case By(steps, order) => By(replace(steps.head) +: steps.tail, order)
case s => s
}

}
1 change: 1 addition & 0 deletions ...ation/src/main/scala/org/opencypher/gremlin/translation/translator/TranslatorFlavor.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ object TranslatorFlavor {
SimplifyRenamedAliases,
RemoveMultipleAliases,
GroupStepFilters,
RemoveIntermediateProjection,
SimplifySingleProjections,
RemoveUselessNullChecks,
RemoveIdentityReselect,
Expand Down
94 changes: 55 additions & 39 deletions translation/src/main/scala/org/opencypher/gremlin/translation/walker/ProjectionWalker.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ object ProjectionWalker {
def walk[T, P](context: WalkerContext[T, P], g: GremlinSteps[T, P], node: ProjectionClause): Unit = {
node match {
case Return(distinct, ReturnItems(_, items), orderBy, skip, limit, _) =>
new ProjectionWalker(context, g).walk(distinct, items, orderBy, skip, limit, finalize = true)
new ProjectionWalker(context, g).walkFinal(distinct, items, orderBy, skip, limit)
case With(distinct, ReturnItems(_, items), orderBy, skip, limit, where) =>
new ProjectionWalker(context, g).walkIntermediate(distinct, items, orderBy, skip, limit, where)
case _ => context.unsupported("projection", node)
Expand All @@ -64,11 +64,10 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
items: Seq[ReturnItem],
orderBy: Option[OrderBy],
skip: Option[Skip],
limit: Option[Limit],
finalize: Boolean): Unit = {
limit: Option[Limit]): Unit = {
ensureFirstStatement(g, context)

val subTraversals = returnSubTraversals(items, finalize)
val subTraversals = returnSubTraversals(items)

applyProjection(subTraversals)
applyLimits(distinct, orderBy, skip, limit)
Expand All @@ -82,16 +81,26 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
limit: Option[Limit],
where: Option[Where]): Unit = {
applyWhereFromReturnItems(items)
walk(distinct, items, orderBy, skip, limit, finalize = false)
walk(distinct, items, orderBy, skip, limit)
applyWhere(where)
reselectProjection(items)
}

def walkFinal(
distinct: Boolean,
items: Seq[ReturnItem],
orderBy: Option[OrderBy],
skip: Option[Skip],
limit: Option[Limit]): Unit = {
walk(distinct, items, orderBy, skip, limit)
finalizeProjection(items)
}

private def __ = {
g.start()
}

private def returnSubTraversals(items: Seq[ReturnItem], finalize: Boolean = false): SubTraversals = {
private def returnSubTraversals(items: Seq[ReturnItem]): SubTraversals = {
val select = getVariableNames(items)

val pivotCollector = mutable.LinkedHashMap.empty[String, GremlinSteps[T, P]]
Expand All @@ -101,7 +110,7 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
for (item <- items) {
val AliasedReturnItem(expression, Variable(alias)) = item

val (returnType, traversal) = subTraversal(alias, expression, finalize)
val (returnType, traversal) = subTraversal(alias, expression)

allCollector.put(alias, traversal)

Expand Down Expand Up @@ -229,6 +238,31 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
g.flatMap(NodeUtils.reselectProjection(variables, context))
}

private def finalizeProjection(items: Seq[ReturnItem]): Unit = {
val aliasToExpression = items.flatMap {
case AliasedReturnItem(expression, Variable(alias)) => Some((alias, expression))
case _ => None
}

val needsFinalization = aliasToExpression.exists(n =>
qualifiedType(n._2) match {
case (_: NodeType, _) => true
case (_: ListType, _: NodeType) => true
case (_: RelationshipType, _) => true
case (_: ListType, _: RelationshipType) => true
case (_: PathType, _) => true
case (_: ListType, _: PathType) => true
case _ => false
})

if (needsFinalization) {
g.project(aliasToExpression.map(_._1): _*)
for ((alias, expression) <- aliasToExpression) {
g.by(finalizeValue(__.select(alias), alias, expression))
}
}
}

private def getVariableNames(items: Seq[ReturnItem]): Seq[String] = {
val dependencyNames = for (AliasedReturnItem(expression, _) <- items;
Variable(n) <- expression.dependencies) yield n
Expand All @@ -240,25 +274,11 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
g.choose(p.neq(NULL), trueChoice, g.start().constant(NULL))
}

private def subTraversal(
alias: String,
expression: Expression,
finalize: Boolean): (ReturnFunctionType, GremlinSteps[T, P]) = {

val variable = expression match {
case Variable(a) => a
case _ => alias
}

private def subTraversal(alias: String, expression: Expression): (ReturnFunctionType, GremlinSteps[T, P]) = {
if (expression.containsAggregate) {
aggregation(alias, expression, finalize)
aggregation(alias, expression)
} else {
val localTraversal = walkLocal(expression, Some(alias))
if (finalize) {
(Pivot, finalizeValue(localTraversal, variable, expression))
} else {
(Pivot, localTraversal)
}
(Pivot, walkLocal(expression, Some(alias)))
}
}

Expand All @@ -268,12 +288,6 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
expression: Expression): GremlinSteps[T, P] = {
val p = context.dsl.predicates()

val qualifiedType = context.expressionTypes.get(expression) match {
case Some(typ: ListType) => (typ, typ.innerType)
case Some(typ) => (typ, AnyType.instance)
case _ => (AnyType.instance, AnyType.instance)
}

lazy val finalizeNode =
__.valueMap(true)

Expand Down Expand Up @@ -301,7 +315,7 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
.valueMap(true)
.fold())

qualifiedType match {
qualifiedType(expression) match {
case (_: NodeType, _) =>
nullIfNull(
subTraversal,
Expand Down Expand Up @@ -341,10 +355,14 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
}
}

private def aggregation(
alias: String,
expression: Expression,
finalize: Boolean): (ReturnFunctionType, GremlinSteps[T, P]) = {
private def qualifiedType(expression: Expression): (CypherType, CypherType) =
context.expressionTypes.get(expression) match {
case Some(typ: ListType) => (typ, typ.innerType)
case Some(typ) => (typ, AnyType.instance)
case _ => (AnyType.instance, AnyType.instance)
}

private def aggregation(alias: String, expression: Expression): (ReturnFunctionType, GremlinSteps[T, P]) = {
val p = context.dsl.predicates()

expression match {
Expand All @@ -357,7 +375,7 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
if (args.exists(_.containsAggregate))
throw new SyntaxException("Can't use aggregate functions inside of aggregate functions")

val (_, traversal) = subTraversal(alias, args.head, finalize = false)
val (_, traversal) = subTraversal(alias, args.head)

if (distinct) {
traversal.dedup()
Expand All @@ -368,8 +386,6 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
fnName.toLowerCase match {
case "avg" =>
(Aggregation, traversal.mean())
case "collect" if finalize =>
(Aggregation, finalizeValue(traversal.fold(), alias, expression))
case "collect" =>
(Aggregation, traversal.fold())
case "count" =>
Expand All @@ -389,7 +405,7 @@ private class ProjectionWalker[T, P](context: WalkerContext[T, P], g: GremlinSte
}
case CountStar() =>
(Aggregation, __.count())
case _: Expression if !finalize && isWherePrecondition(expression) =>
case _: Expression if isWherePrecondition(expression) =>
(Expression, __.identity())
case _ =>
context.unsupported("expression", expression)
Expand Down
32 changes: 14 additions & 18 deletions translation/src/test/java/org/opencypher/gremlin/translation/AstRewriterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,62 +104,58 @@ public void matchWhere() throws Exception {
}

@Test
public void matchWhereOrderBy() throws Exception {
public void dontRewriteMatchWhereOrderBy() throws Exception {
assertThat(parse(
"MATCH (a:artist) " +
"WITH a WHERE a.age > 18 " +
"RETURN a.name " +
"ORDER BY a.name"
)).normalizedTo(
"MATCH (a:artist) " +
"WITH a AS a " +
"WHERE a.age > 18 " +
"WITH a.name AS ` FRESHID50` " +
"ORDER BY ` FRESHID50` " +
"RETURN ` FRESHID50` AS `a.name`"
"WITH a WHERE a.age > 18 " +
"RETURN a.name " +
"ORDER BY a.name"
);
}

@Test
public void matchOrderBySingleName() throws Exception {
public void dontRewriteMatchOrderBySingleName() throws Exception {
assertThat(parse(
"MATCH (a:artist) " +
"RETURN a.name " +
"ORDER BY a.name"
)).normalizedTo(
"MATCH (a:artist) " +
"WITH a.name AS ` FRESHID26` " +
"ORDER BY ` FRESHID26` " +
"RETURN ` FRESHID26` AS `a.name`"
"RETURN a.name " +
"ORDER BY a.name"
);
}

@Test
public void matchOrderByMultipleNames() throws Exception {
public void dontRewriteMatchOrderByMultipleNames() throws Exception {
assertThat(parse(
"MATCH (a:artist)<-[:writtenBy]-(s:song) " +
"RETURN a.name, s.name " +
"ORDER BY a.name, s.name"
)).normalizedTo(
"MATCH (a:artist)<-[:writtenBy]-(s:song) " +
"WITH a.name AS ` FRESHID49`, s.name AS ` FRESHID57` " +
"ORDER BY ` FRESHID49`, ` FRESHID57` " +
"RETURN ` FRESHID49` AS `a.name`, ` FRESHID57` AS `s.name`"
"RETURN a.name, s.name " +
"ORDER BY a.name, s.name"
);
}

@Test
public void matchOrderBySkipLimit() throws Exception {
public void dontRewriteMatchOrderBySkipLimit() throws Exception {
assertThat(parse(
"MATCH (a:artist) " +
"RETURN a.name " +
"ORDER BY a.name " +
"SKIP 1 LIMIT 2"
)).normalizedTo(
"MATCH (a:artist) " +
"WITH a.name AS ` FRESHID26` " +
"ORDER BY ` FRESHID26` SKIP 1 LIMIT 2 " +
"RETURN ` FRESHID26` AS `a.name`"
"RETURN a.name " +
"ORDER BY a.name " +
"SKIP 1 LIMIT 2"
);
}

Expand Down
Loading

0 comments on commit d870ab8

Please sign in to comment.