asolimando commented on code in PR #3316:
URL: https://github.com/apache/calcite/pull/3316#discussion_r1279256471
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -801,6 +804,17 @@ private void checkExponentialCnf(int n) {
and(eRef,
or(fRef,
and(gRef, or(trueLiteral, falseLiteral)))))))));
+
Review Comment:
Sorry I picked the wrong test name, the correct one is
`RexProgramTest#testPullFactors` as you mention, running it twice (after
replacing `LinkedHashMap` to `HashMap`) gave a difference, so the test is
effective:
```
RexUtil.pullFactors(rexBuilder, OR(AND(=(?0.a, ?0.b), =(?0.j, 'Brand#12'),
>=(?0.h, 1), <=(?0.h, 11), ?0.c, SEARCH(?0.i, Sarg['AIR':CHAR(7), 'AIR
REG']:CHAR(7)), SEARCH(?0.k, Sarg['SM BOX':CHAR(7), 'SM CASE', 'SM PACK', 'SM
PKG':CHAR(7)]:CHAR(7))), AND(=(?0.a, ?0.b), =(?0.j, 'Brand#13'), >=(?0.h, 10),
<=(?0.h, 20), ?0.c, SEARCH(?0.i, Sarg['AIR':CHAR(7), 'AIR REG']:CHAR(7)),
SEARCH(?0.k, Sarg['MED BOX':CHAR(8), 'MED CASE', 'MED PACK', 'MED
PKG':CHAR(8)]:CHAR(8))), AND(=(?0.a, ?0.b), =(?0.j, 'Brand#14'), >=(?0.h, 20),
<=(?0.h, 30), ?0.c, SEARCH(?0.i, Sarg['AIR':CHAR(7), 'AIR REG']:CHAR(7)),
SEARCH(?0.k, Sarg['LG BOX':CHAR(7), 'LG CASE', 'LG PACK', 'LG
PKG':CHAR(7)]:CHAR(7)))))
Expected: with toString() "AND(=(?0.a, ?0.b), ?0.c, SEARCH(?0.i,
Sarg['AIR':CHAR(7), 'AIR REG']:CHAR(7)), OR(AND(=(?0.j, 'Brand#12'), >=(?0.h,
1), <=(?0.h, 11), SEARCH(?0.k, Sarg['SM BOX':CHAR(7), 'SM CASE', 'SM PACK', 'SM
PKG':CHAR(7)]:CHAR(7))), AND(=(?0.j, 'Brand#13'), >=(?0.h, 10), <=(?0.h, 20),
SEARCH(?0.k, Sarg['MED BOX':CHAR(8), 'MED CASE', 'MED PACK', 'MED
PKG':CHAR(8)]:CHAR(8))), AND(=(?0.j, 'Brand#14'), >=(?0.h, 20), <=(?0.h, 30),
SEARCH(?0.k, Sarg['LG BOX':CHAR(7), 'LG CASE', 'LG PACK', 'LG
PKG':CHAR(7)]:CHAR(7)))))"
but: toString() was "AND(SEARCH(?0.i, Sarg['AIR':CHAR(7), 'AIR
REG']:CHAR(7)), =(?0.a, ?0.b), ?0.c, OR(AND(=(?0.j, 'Brand#12'), >=(?0.h, 1),
<=(?0.h, 11), SEARCH(?0.k, Sarg['SM BOX':CHAR(7), 'SM CASE', 'SM PACK', 'SM
PKG':CHAR(7)]:CHAR(7))), AND(=(?0.j, 'Brand#13'), >=(?0.h, 10), <=(?0.h, 20),
SEARCH(?0.k, Sarg['MED BOX':CHAR(8), 'MED CASE', 'MED PACK', 'MED
PKG':CHAR(8)]:CHAR(8))), AND(=(?0.j, 'Brand#14'), >=(?0.h, 20), <=(?0.h, 30),
SEARCH(?0.k, Sarg['LG BOX':CHAR(7), 'LG CASE', 'LG PACK', 'LG
PKG':CHAR(7)]:CHAR(7)))))"
java.lang.AssertionError: RexUtil.pullFactors(rexBuilder, OR(AND(=(?0.a,
?0.b), =(?0.j, 'Brand#12'), >=(?0.h, 1), <=(?0.h, 11), ?0.c, SEARCH(?0.i,
Sarg['AIR':CHAR(7), 'AIR REG']:CHAR(7)), SEARCH(?0.k, Sarg['SM BOX':CHAR(7),
'SM CASE', 'SM PACK', 'SM PKG':CHAR(7)]:CHAR(7))), AND(=(?0.a, ?0.b), =(?0.j,
'Brand#13'), >=(?0.h, 10), <=(?0.h, 20), ?0.c, SEARCH(?0.i, Sarg['AIR':CHAR(7),
'AIR REG']:CHAR(7)), SEARCH(?0.k, Sarg['MED BOX':CHAR(8), 'MED CASE', 'MED
PACK', 'MED PKG':CHAR(8)]:CHAR(8))), AND(=(?0.a, ?0.b), =(?0.j, 'Brand#14'),
>=(?0.h, 20), <=(?0.h, 30), ?0.c, SEARCH(?0.i, Sarg['AIR':CHAR(7), 'AIR
REG']:CHAR(7)), SEARCH(?0.k, Sarg['LG BOX':CHAR(7), 'LG CASE', 'LG PACK', 'LG
PKG':CHAR(7)]:CHAR(7)))))
Expected: with toString() "AND(=(?0.a, ?0.b), ?0.c, SEARCH(?0.i,
Sarg['AIR':CHAR(7), 'AIR REG']:CHAR(7)), OR(AND(=(?0.j, 'Brand#12'), >=(?0.h,
1), <=(?0.h, 11), SEARCH(?0.k, Sarg['SM BOX':CHAR(7), 'SM CASE', 'SM PACK', 'SM
PKG':CHAR(7)]:CHAR(7))), AND(=(?0.j, 'Brand#13'), >=(?0.h, 10), <=(?0.h, 20),
SEARCH(?0.k, Sarg['MED BOX':CHAR(8), 'MED CASE', 'MED PACK', 'MED
PKG':CHAR(8)]:CHAR(8))), AND(=(?0.j, 'Brand#14'), >=(?0.h, 20), <=(?0.h, 30),
SEARCH(?0.k, Sarg['LG BOX':CHAR(7), 'LG CASE', 'LG PACK', 'LG
PKG':CHAR(7)]:CHAR(7)))))"
but: toString() was "AND(SEARCH(?0.i, Sarg['AIR':CHAR(7), 'AIR
REG']:CHAR(7)), =(?0.a, ?0.b), ?0.c, OR(AND(=(?0.j, 'Brand#12'), >=(?0.h, 1),
<=(?0.h, 11), SEARCH(?0.k, Sarg['SM BOX':CHAR(7), 'SM CASE', 'SM PACK', 'SM
PKG':CHAR(7)]:CHAR(7))), AND(=(?0.j, 'Brand#13'), >=(?0.h, 10), <=(?0.h, 20),
SEARCH(?0.k, Sarg['MED BOX':CHAR(8), 'MED CASE', 'MED PACK', 'MED
PKG':CHAR(8)]:CHAR(8))), AND(=(?0.j, 'Brand#14'), >=(?0.h, 20), <=(?0.h, 30),
SEARCH(?0.k, Sarg['LG BOX':CHAR(7), 'LG CASE', 'LG PACK', 'LG
PKG':CHAR(7)]:CHAR(7)))))"
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
at
org.apache.calcite.rex.RexProgramTestBase.checkPullFactors(RexProgramTestBase.java:63)
at
org.apache.calcite.rex.RexProgramTest.testPullFactors(RexProgramTest.java:813)
[...]
```
As I was saying, it seems difficult to make it fail more consistently,
because that's exactly the point of your PR, so I think this test, as-is, is
good enough, because:
- it fails at times, so if we break the determinism at least we will have a
flaky test, which will force people to look into what happened
- it documents explicitly that we want a deterministic output for that
method, if the test becomes flaky people will easily figure out why from your
comments (both in the unit-test and in the `commonFactors` method)
Another possible addition is to make `commonFactors` return
`LinkedHashMap<RexNode, RexNode>` instead of `Map<RexNode, RexNode>`, to stress
that we really want a deterministic order (and therefore, `Map<>` would not be
good). This is one of the few cases where it makes sense to use a concrete
implementation in place of an interface.
The `commonFactors` method is `private static` and is called in a single
place, it wouldn't cause much troubles changing its return type, we can
consider this but I don't feel strongly about it, it's just a stronger way to
document our expectations on the method other than a comment.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
