Skip to content

CALCITE-1790 Simplify boolean CASE into AND/OR#450

Closed
rusanu wants to merge 1 commit intoapache:masterfrom
rusanu:CALCITE-1790
Closed

CALCITE-1790 Simplify boolean CASE into AND/OR#450
rusanu wants to merge 1 commit intoapache:masterfrom
rusanu:CALCITE-1790

Conversation

@rusanu
Copy link
Copy Markdown
Contributor

@rusanu rusanu commented May 16, 2017

This PR converts the boolean CASE into AND/OR expressions. I did not add any limit atm, will add one but I'm not sure what would be the favorite way (configuration, hard coded) and also what to measure (number of CASE children, complexity of resulted 'simplification?')

Copy link
Copy Markdown
Contributor

@jcamachor jcamachor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerning your question about limiting the number of predicates, I guess we could specify it in terms of number of operands in the CASE statement? This way it would be rather straightforward to bail out quick.

if (pair.e.getKey().getType().isNullable()) {
break trueFalse;
boolean anyNullable = false;
trueFalse:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can kill the trueFalse tag and make the code linear (or maybe I am not reading it properly with the indentation)? It would be more readable, I think after your changes in the method, it does not make sense to have it anymore. 1) Check nullability. 2) Try 2. 3) Try 3.

@@ -0,0 +1,525 @@
/*
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, it would be cool to integrate with existing tests in RexProgramTest instead of adding new infra again, as they already contain multiple tests for CASE (and other preds) simplification.

https://github.com/apache/calcite/blob/master/core/src/test/java/org/apache/calcite/test/RexProgramTest.java#L1101

pair.e.getKey(),
pair.e.getValue()),
false);
terms.add(simplify(toAdd));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might miss some simplification if we call simplify at every iteration vs over the returned disjunction (L514), right? (e.g., folding of OR true)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the simplify to the final disjunction

// WHEN p3 THEN TRUE
// ELSE FALSE
// END
// if p1...pn cannot be nullable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing transformation in the comment -> // (p1 or (p3 and not(p2)))

@rusanu rusanu force-pushed the CALCITE-1790 branch 5 times, most recently from 0d7f311 to 3700f54 Compare May 18, 2017 09:33
@rusanu
Copy link
Copy Markdown
Contributor Author

rusanu commented May 22, 2017

This seems to risky

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants