Skip to content

Commit dd028a7

Browse files
deusaquilusdeusaquilus
authored
Reverse null-check branch order in Optional operations (#2970)
1 parent 2e8a6d5 commit dd028a7

File tree

23 files changed

+108
-209
lines changed

23 files changed

+108
-209
lines changed

‎.scalafmt.conf‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,26 @@
11
version = "3.7.17"
2-
maxColumn = 120
2+
maxColumn = 240
33
align.preset = most
44
align.multiline = false
55
continuationIndent.defnSite = 2
66
assumeStandardLibraryStripMargin = true
77
docstrings.style = Asterisk
88
docstrings.wrapMaxColumn = 80
99
lineEndings = preserve
10-
includeCurlyBraceInSelectChains = false
1110
danglingParentheses.preset = true
1211
optIn.annotationNewlines = true
1312
newlines.alwaysBeforeMultilineDef = false
1413
runner.dialect = scala213
1514
rewrite.rules = [RedundantBraces]
1615

16+
# If I've inserted extra newlines I know what I'm doing, don't wrap them back.
17+
newlines.source = keep
18+
19+
# Don't change braces in one-liners to parens e.g. don't change this: `test("foo") { assertEquals(x,y) }`
20+
# to this `test("foo")(assertEquals(x,y))`. The `rewrite.rules = [RedundantBraces]` will introduce this behavior
21+
# unless you add the below option.
22+
rewrite.redundantBraces.parensForOneLineApply = false
23+
1724
project.excludePaths = ["glob:**/scalafix/input/**", "glob:**/scalafix/output/**"]
1825

1926
rewrite.redundantBraces.generalExpressions = false

‎build/setup_db_scripts.sh‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ function setup_sqlite() {
3434
DB_FILE=quill-jdbc-monix/quill_test.db
3535
rm -f $DB_FILE
3636
sqlite3 $DB_FILE < $SQLITE_SCRIPT
37+
chmod a+rw $DB_FILE
38+
39+
# DB File in quill-jdbc-monix
40+
DB_FILE=quill-jdbc-test-sqlite/quill_test.db
41+
rm -f $DB_FILE
42+
sqlite3 $DB_FILE < $SQLITE_SCRIPT
3743
chmod a+rw $DB_FILE
3844

3945
echo "Sqlite ready!"

‎quill-cassandra-pekko/src/test/scala/io/getquill/context/cassandra/pekko/CassandraPekkoSpec.scala‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,7 @@ package io.getquill.context.cassandra.pekko
33
import org.apache.pekko.actor.ActorSystem
44
import org.apache.pekko.stream.Materializer
55
import org.apache.pekko.stream.connectors.cassandra.CassandraSessionSettings
6-
import org.apache.pekko.stream.connectors.cassandra.scaladsl.{
7-
CassandraSessionRegistry,
8-
CassandraSession => CassandraPekkoSession
9-
}
6+
import org.apache.pekko.stream.connectors.cassandra.scaladsl.{CassandraSessionRegistry, CassandraSession => CassandraPekkoSession}
107
import org.apache.pekko.testkit.TestKit
118
import io.getquill.base.Spec
129
import io.getquill.context.cassandra.CassandraTestEntities

‎quill-cassandra/src/main/scala/io/getquill/context/cassandra/encoding/CassandraTypes.scala‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,6 @@
11
package io.getquill.context.cassandra.encoding
22

3-
import java.lang.{
4-
Boolean => JBoolean,
5-
Byte => JByte,
6-
Double => JDouble,
7-
Float => JFloat,
8-
Integer => JInt,
9-
Long => JLong,
10-
Short => JShort
11-
}
3+
import java.lang.{Boolean => JBoolean, Byte => JByte, Double => JDouble, Float => JFloat, Integer => JInt, Long => JLong, Short => JShort}
124
import java.math.{BigDecimal => JBigDecimal}
135
import java.nio.ByteBuffer
146
import java.util.{Date, UUID}

‎quill-core/src/test/scala/io/getquill/norm/FlattenOptionOperationSpec.scala‎

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
package io.getquill.norm
22

3-
import io.getquill.ast._
3+
import io.getquill.ast.{+&&+, _}
44
import io.getquill.MirrorContexts.testContext._
55
import io.getquill.ast.Implicits._
66
import io.getquill.norm.ConcatBehavior.{AnsiConcat, NonAnsiConcat}
77
import io.getquill.MoreAstOps._
88
import io.getquill.base.Spec
99
import io.getquill.util.TraceConfig
1010

11-
class FlattenOptionOperationSpec extends Spec { // hello
11+
class FlattenOptionOperationSpec extends Spec {
1212

1313
def o = Ident("o")
1414
def c1 = Constant.auto(1)
@@ -156,51 +156,51 @@ class FlattenOptionOperationSpec extends Spec { // hello
156156
}
157157
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(
158158
q.ast.body: Ast
159-
).toString mustEqual "((o != null) && (o < 1)) || ((o == null) && true)"
159+
).toString mustEqual "((o < 1) && (o != null)) || (true && (o == null))"
160160
}
161161
"map + getOrElse(false)" in {
162162
val q = quote { (o: Option[Int]) =>
163163
o.map(_ < 1).getOrElse(false)
164164
}
165165
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(
166166
q.ast.body: Ast
167-
).toString mustEqual "((o != null) && (o < 1)) || ((o == null) && false)"
167+
).toString mustEqual "((o < 1) && (o != null)) || (false && (o == null))"
168168
}
169169
"forall" - {
170170
"regular operation" in {
171171
val q = quote { (o: Option[Int]) =>
172172
o.forall(i => i != 1)
173173
}
174174
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
175-
(IsNullCheck(o) +||+ (o +!=+ c1))
175+
((o +!=+ c1) +||+ IsNullCheck(o))
176176
new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
177-
(IsNullCheck(o) +||+ (o +!=+ c1))
177+
((o +!=+ c1) +||+ IsNullCheck(o))
178178
}
179179
"possible-fallthrough operation" in {
180180
val q = quote { (o: Option[String]) =>
181181
o.forall(s => s + "foo" == "bar")
182182
}
183183
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
184-
(IsNullCheck(o) +||+ ((o +++ cFoo) +==+ cBar))
184+
(((o +++ cFoo) +==+ cBar) +||+ IsNullCheck(o))
185185
new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
186-
(IsNullCheck(o) +||+ (IsNotNullCheck(o) +&&+ ((o +++ cFoo) +==+ cBar)))
186+
((((o +++ cFoo) +==+ cBar) +&&+ IsNotNullCheck(o)) +||+ IsNullCheck(o))
187187
}
188188
"never-fallthrough operation" in {
189189
val q = quote { (o: Option[String]) =>
190190
o.forall(s => if (s + "foo" == "bar") true else false)
191191
}
192192
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
193-
(IsNullCheck(o) +||+ (IsNotNullCheck(o) +&&+ If(
193+
((If(
194194
(o +++ cFoo) +==+ cBar,
195195
Constant.auto(true),
196196
Constant.auto(false)
197-
)))
197+
) +&&+ IsNotNullCheck(o)) +||+ IsNullCheck(o))
198198
new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
199-
(IsNullCheck(o) +||+ (IsNotNullCheck(o) +&&+ If(
199+
((If(
200200
(o +++ cFoo) +==+ cBar,
201201
Constant.auto(true),
202202
Constant.auto(false)
203-
)))
203+
) +&&+ IsNotNullCheck(o)) +||+ IsNullCheck(o))
204204
}
205205
}
206206
"map + forall + binop" - {
@@ -209,19 +209,16 @@ class FlattenOptionOperationSpec extends Spec { // hello
209209
o.map(_.i).forall(i => i != 1) && true
210210
}
211211
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
212-
((IsNullCheck(Property(o, "i")) +||+ ((Property(o, "i") +!=+ c1))) +&&+ Constant.auto(true))
212+
((((Property(o, "i") +!=+ c1)) +||+ IsNullCheck(Property(o, "i"))) +&&+ Constant.auto(true))
213213
}
214214
"possible-fallthrough operation" in {
215215
val q = quote { (o: Option[TestEntity2]) =>
216216
o.map(_.s).forall(s => s + "foo" == "bar") && true
217217
}
218218
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
219-
((IsNullCheck(Property(o, "s")) +||+ ((Property(o, "s") +++ cFoo) +==+ cBar) +&&+ Constant.auto(true)))
219+
((((Property(o, "s") +++ cFoo) +==+ cBar) +||+ IsNullCheck(Property(o, "s")) +&&+ Constant.auto(true)))
220220
new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
221-
((IsNullCheck(Property(o, "s")) +||+ (IsNotNullCheck(Property(o, "s")) +&&+ ((Property(
222-
o,
223-
"s"
224-
) +++ cFoo) +==+ cBar))) +&&+ Constant.auto(true))
221+
(((((Property(o, "s") +++ cFoo) +==+ cBar) +&&+ IsNotNullCheck(Property(o, "s"))) +||+ IsNullCheck(Property(o, "s"))) +&&+ Constant.auto(true))
225222
}
226223
}
227224
"exists" - {
@@ -241,16 +238,16 @@ class FlattenOptionOperationSpec extends Spec { // hello
241238
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
242239
((o +++ cFoo) +==+ cBar)
243240
new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
244-
(IsNotNullCheck(o) +&&+ ((o +++ cFoo) +==+ cBar))
241+
(((o +++ cFoo) +==+ cBar) +&&+ IsNotNullCheck(o))
245242
}
246243
"never-fallthrough operation" in {
247244
val q = quote { (o: Option[String]) =>
248245
o.exists(s => if (s + "foo" == "bar") true else false)
249246
}
250247
new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
251-
(IsNotNullCheck(o) +&&+ If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)))
248+
(If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)) +&&+ IsNotNullCheck(o))
252249
new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
253-
(IsNotNullCheck(o) +&&+ If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)))
250+
(If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)) +&&+ IsNotNullCheck(o))
254251
}
255252
}
256253
"exists row" in {

‎quill-engine/src/main/scala/io/getquill/norm/FlattenOptionOperation.scala‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
3434
validateBody(body),
3535
() => {
3636
val reduction = BetaReduction(body, alias -> ast)
37-
apply((IsNullCheck(ast) +||+ (IsNotNullCheck(ast) +&&+ reduction)): Ast)
37+
apply(((reduction +&&+ IsNotNullCheck(ast)) +||+ IsNullCheck(ast)): Ast)
3838
},
3939
() => {
4040
val reduced = BetaReduction(body, alias -> ast)
41-
apply((IsNullCheck(ast) +||+ reduced): Ast)
41+
apply((reduced +||+ IsNullCheck(ast)): Ast)
4242
}
4343
)
4444

@@ -81,7 +81,7 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
8181

8282
case OptionGetOrElse(HasBooleanQuat(OptionMap(ast, alias, body)), HasBooleanQuat(alternative)) =>
8383
val expr = BetaReduction(body, alias -> ast)
84-
val output: Ast = (IsNotNullCheck(ast) +&&+ expr) +||+ (IsNullCheck(ast) +&&+ alternative)
84+
val output: Ast = (expr +&&+ IsNotNullCheck(ast)) +||+ (alternative +&&+ IsNullCheck(ast))
8585
apply(output)
8686

8787
case OptionGetOrElse(ast, body) =>
@@ -96,6 +96,14 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
9696
case OptionMap(ast, alias, body) =>
9797
uncheckedReduction(ast, alias, body, containsNonFallthroughElement)
9898

99+
// a.orElse(b).forAll(alias => body) becomes:
100+
// body(->a) || a==null && body(->b) || a==null && b==null
101+
//
102+
// Note that since all of the clauses are boolean this a.orElse(...) can be replaced
103+
// by a||(b && a==null). If the clause was actually returning value (e.g. if(a) foo else bar)
104+
// then these kinds of reductions would not be possible.
105+
// Leaving the ||a==null clause without reversing for now despite the fact that ==null
106+
// clauses shuold generally be the 2nd in the order because of the <> issue.
99107
case OptionForall(OptionOrElse(a, b), alias, body) =>
100108
val reducedA = BetaReduction(body, alias -> a)
101109
val reducedB = BetaReduction(body, alias -> b)
@@ -117,7 +125,7 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
117125
containsNonFallthroughElement(body),
118126
() => {
119127
val reduction = BetaReduction(body, alias -> ast)
120-
apply(IsNotNullCheck(ast) +&&+ reduction: Ast)
128+
apply(reduction +&&+ IsNotNullCheck(ast): Ast)
121129
},
122130
() => apply(BetaReduction(body, alias -> ast))
123131
)

‎quill-engine/src/main/scala/io/getquill/norm/SheathLeafClauses.scala‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,6 @@
11
package io.getquill.norm
22

3-
import io.getquill.ast.{
4-
Aggregation,
5-
Ast,
6-
CaseClass,
7-
ConcatMap,
8-
Distinct,
9-
Filter,
10-
FlatMap,
11-
GroupBy,
12-
GroupByMap,
13-
Ident,
14-
Join,
15-
Map,
16-
Property,
17-
Query,
18-
StatefulTransformerWithStack,
19-
Union,
20-
UnionAll
21-
}
3+
import io.getquill.ast.{Aggregation, Ast, CaseClass, ConcatMap, Distinct, Filter, FlatMap, GroupBy, GroupByMap, Ident, Join, Map, Property, Query, StatefulTransformerWithStack, Union, UnionAll}
224
import io.getquill.ast.Ast.LeafQuat
235
import io.getquill.ast.StatefulTransformerWithStack.History
246
import io.getquill.util.{Interpolator, TraceConfig}

‎quill-engine/src/main/scala/io/getquill/norm/capture/AvoidAliasConflict.scala‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,6 @@
11
package io.getquill.norm.capture
22

3-
import io.getquill.ast.{
4-
Entity,
5-
Filter,
6-
FlatJoin,
7-
FlatMap,
8-
GroupBy,
9-
Ident,
10-
Join,
11-
Map,
12-
Query,
13-
SortBy,
14-
StatefulTransformer,
15-
_
16-
}
3+
import io.getquill.ast.{Entity, Filter, FlatJoin, FlatMap, GroupBy, Ident, Join, Map, Query, SortBy, StatefulTransformer, _}
174
import io.getquill.ast.Implicits._
185
import io.getquill.norm.{BetaReduction, Normalize}
196
import io.getquill.util.{Interpolator, TraceConfig}

‎quill-engine/src/main/scala/io/getquill/sql/idiom/SqlIdiom.scala‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,7 @@ import io.getquill.norm.ConcatBehavior.AnsiConcat
1717
import io.getquill.norm.EqualityBehavior.AnsiEquality
1818
import io.getquill.norm.{ConcatBehavior, EqualityBehavior, ExpandReturning, NormalizeCaching, ProductAggregationToken}
1919
import io.getquill.quat.Quat
20-
import io.getquill.sql.norm.{
21-
HideTopLevelFilterAlias,
22-
NormalizeFilteredActionAliases,
23-
RemoveExtraAlias,
24-
RemoveUnusedSelects
25-
}
20+
import io.getquill.sql.norm.{HideTopLevelFilterAlias, NormalizeFilteredActionAliases, RemoveExtraAlias, RemoveUnusedSelects}
2621
import io.getquill.util.{Interleave, Interpolator, Messages, TraceConfig}
2722
import io.getquill.util.Messages.{TraceType, fail, trace}
2823

‎quill-engine/src/main/scala/io/getquill/sql/norm/RemoveUnusedSelects.scala‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,7 @@
11
package io.getquill.sql.norm
22

33
import io.getquill.ast.{Ast, CollectAst, Ident, Property, StatefulTransformer}
4-
import io.getquill.context.sql.{
5-
DistinctKind,
6-
FlatJoinContext,
7-
FlattenSqlQuery,
8-
FromContext,
9-
InfixContext,
10-
JoinContext,
11-
QueryContext,
12-
SelectValue,
13-
SetOperationSqlQuery,
14-
SqlQuery,
15-
TableContext,
16-
UnaryOperationSqlQuery
17-
}
4+
import io.getquill.context.sql.{DistinctKind, FlatJoinContext, FlattenSqlQuery, FromContext, InfixContext, JoinContext, QueryContext, SelectValue, SetOperationSqlQuery, SqlQuery, TableContext, UnaryOperationSqlQuery}
185
import io.getquill.norm.PropertyMatryoshka
196
import io.getquill.quat.Quat
207

0 commit comments

Comments
 (0)