mihaibudiu commented on code in PR #4591:
URL: https://github.com/apache/calcite/pull/4591#discussion_r2445565882
##########
core/src/main/java/org/apache/calcite/util/Bug.java:
##########
@@ -226,6 +226,12 @@ public abstract class Bug {
* is fixed. */
public static final boolean CALCITE_6611_FIXED = false;
+ /** Whether
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7201";>[CALCITE-7201]
+ * ClassCastException in RexInterpreter#search with different NUMERIC
values</a>
+ * is fixed. */
+ public static final boolean CALCITE_7201_FIXED = false;
Review Comment:
Let's merge the other one first; I've never seen any of these flags get
deleted.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2040,6 +2040,22 @@ private void checkExponentialCnf(int n) {
checkSimplify(expr, simplified);
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7194";>[CALCITE-7194]
+ * Simplify comparisons between function calls and literals to SEARCH</a>. */
+ @Test void testSimplifyComparisonsOfCallsWithLiteralsToSearch() {
Review Comment:
I would expect a test case for every example in the JIRA.
But writing test cases at this low level is tedious, is it difficult to
isolate the effect of simplify if you use a SQL input and then check the plan?
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -3206,29 +3206,11 @@ private boolean accept_(RexNode e, List<RexNode>
newTerms) {
private boolean accept2(RexNode left, RexNode right, SqlKind kind,
List<RexNode> newTerms) {
- switch (left.getKind()) {
- case INPUT_REF:
- case FIELD_ACCESS:
- case CAST:
- switch (right.getKind()) {
- case LITERAL:
- return accept2b(left, kind, (RexLiteral) right, newTerms);
- default:
- break;
- }
- return false;
- case LITERAL:
- switch (right.getKind()) {
- case INPUT_REF:
- case FIELD_ACCESS:
- case CAST:
- return accept2b(right, kind.reverse(), (RexLiteral) left, newTerms);
- default:
- break;
- }
- return false;
- default:
- break;
+ if (right.isA(SqlKind.LITERAL) && RexUtil.isDeterministic(left)) {
Review Comment:
None of these accept functions has any JavaDoc. I know they are private, but
why not?
Not having a spec makes it harder to review code that uses or modifies them.
Maybe you can at least document this one and accept2b since you're touching
them?
##########
druid/src/test/java/org/apache/calcite/test/DruidAdapterIT.java:
##########
@@ -3219,7 +3220,7 @@ private void testCountWithApproxDistinct(boolean approx,
String sql, String expe
+ "and '1997-03-01 00:00:00 UTC' order by t limit 2";
final String druidQuery =
"{'queryType':'scan','dataSource':'foodmart','intervals':"
- +
"['1997-01-01T00:00:00.000Z/1997-04-01T00:00:00.000Z'],'virtualColumns':"
+ +
"['1997-01-01T00:00:00.000Z/1997-03-01T00:00:00.001Z'],'virtualColumns':"
Review Comment:
so the previous result was wrong?
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -3206,29 +3206,11 @@ private boolean accept_(RexNode e, List<RexNode>
newTerms) {
private boolean accept2(RexNode left, RexNode right, SqlKind kind,
List<RexNode> newTerms) {
- switch (left.getKind()) {
- case INPUT_REF:
- case FIELD_ACCESS:
- case CAST:
- switch (right.getKind()) {
- case LITERAL:
- return accept2b(left, kind, (RexLiteral) right, newTerms);
- default:
- break;
- }
- return false;
- case LITERAL:
- switch (right.getKind()) {
- case INPUT_REF:
- case FIELD_ACCESS:
- case CAST:
- return accept2b(right, kind.reverse(), (RexLiteral) left, newTerms);
- default:
- break;
- }
- return false;
- default:
- break;
+ if (right.isA(SqlKind.LITERAL) && RexUtil.isDeterministic(left)) {
Review Comment:
so you are generalizing this from a handfull of cases to all deterministic
functions?
--
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]
