> On April 7, 2016, 7:55 p.m., Jeff Hagelberg wrote: > > repository/src/main/scala/org/apache/atlas/query/GremlinEvaluator.scala, > > line 99 > > <https://reviews.apache.org/r/45499/diff/4/?file=1330263#file1330263line99> > > > > I would suggest adding an intermediate variable here for the output of > > the if expression to improve readability. Generally, the longer an > > expression is, the harder it is to understand. > > Neeru Gupta wrote: > May be I am missing something, but there is already a output variable: > val selExpr. This variable gets initialized based on the parent expression.
This is a very minor comment. The thing that stood out was the period at the end of line 100. The expression is effectively val selExpr = [result of three part if statement].asInstanceOf[Expressions.SelectExpression] I was thinking this could be split into two or more statements, something like this: val selObj = [three part if statement] val selExpr = selObj.asInstanceOf[Expressions.SelectExpression] - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45499/#review127643 ----------------------------------------------------------- On April 14, 2016, 2:15 a.m., Neeru Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45499/ > ----------------------------------------------------------- > > (Updated April 14, 2016, 2:15 a.m.) > > > Review request for atlas, David Kantor and Jeff Hagelberg. > > > Bugs: ATLAS-435 and ATLAS-436 > https://issues.apache.org/jira/browse/ATLAS-435 > https://issues.apache.org/jira/browse/ATLAS-436 > > > Repository: atlas > > > Description > ------- > > ATLAS-435, 436 order by and limit clause in DSL > > > Diffs > ----- > > repository/src/main/scala/org/apache/atlas/query/Expressions.scala > a5dfa9f409c0d810be7449413f5c2d53d5103dce > repository/src/main/scala/org/apache/atlas/query/GremlinEvaluator.scala > edb190db599d43eb98eca13c7907c73e7d77ae34 > repository/src/main/scala/org/apache/atlas/query/GremlinQuery.scala > f1590a884eda007182dad7541a2bc8eb97d68fbc > repository/src/main/scala/org/apache/atlas/query/QueryParser.scala > b6bbbd31a91dd2d0ff3e9082abafff3539765a01 > repository/src/main/scala/org/apache/atlas/query/Resolver.scala > c7e1e81a555d57be1d7f63cb84a1c8f550ac37ab > > repository/src/test/java/org/apache/atlas/discovery/GraphBackedDiscoveryServiceTest.java > ea93cbf0e6071a961aa5d030f0340358e79e446b > > Diff: https://reviews.apache.org/r/45499/diff/ > > > Testing > ------- > > Manual and unit testing done for various DSL queries. Have added test cases > in GraphBackedDiscoveryServiceTest for orderby and limit clause for various > queries. > > Order by clause is limited to specifying one criterian only. Comparison is > case insensitive. > > > Thanks, > > Neeru Gupta > >
