[WIP] Add support for incremental rewrite of nested queries #400
Conversation
...cremental/src/test/java/com/linkedin/coral/incremental/RelToIncrementalSqlConverterTest.java
Outdated
Show resolved
Hide resolved
...-incremental/src/main/java/com/linkedin/coral/incremental/IncrementalTransformerResults.java
Outdated
Show resolved
Hide resolved
| private RelNode incrementalRelNode; | ||
| private RelNode refreshRelNode; | ||
| private Map<String, RelNode> intermediateQueryRelNodes; | ||
| private List<String> intermediateOrderings; |
There was a problem hiding this comment.
As we discussed does it make sense to get rid of incrementalRelNode? Is it a special case of Map<String, RelNode> intermediateQueryRelNodes?
...-incremental/src/main/java/com/linkedin/coral/incremental/IncrementalTransformerResults.java
Outdated
Show resolved
Hide resolved
| private RelNode incrementalRelNode; | ||
| private RelNode refreshRelNode; | ||
| private Map<String, RelNode> intermediateQueryRelNodes; | ||
| private List<String> intermediateOrderings; |
There was a problem hiding this comment.
intermediateOrderings --> materializationOrder?
There was a problem hiding this comment.
I'm now getting indices from the map so will get rid of this entirely.
|
|
||
| public IncrementalTransformerResults() { | ||
| incrementalRelNode = null; | ||
| refreshRelNode = null; |
There was a problem hiding this comment.
Should it be a list of refresh relnodes? We can remove from current PR till we converge on the best representation.
There was a problem hiding this comment.
Yes, this should be a list of refresh RelNodes—one for each aggregate query. Will remove for now.
...-incremental/src/main/java/com/linkedin/coral/incremental/IncrementalTransformerResults.java
Outdated
Show resolved
Hide resolved
...-incremental/src/main/java/com/linkedin/coral/incremental/RelNodeIncrementalTransformer.java
Show resolved
Hide resolved
...-incremental/src/main/java/com/linkedin/coral/incremental/RelNodeIncrementalTransformer.java
Outdated
Show resolved
Hide resolved
...cremental/src/test/java/com/linkedin/coral/incremental/RelToIncrementalSqlConverterTest.java
Outdated
Show resolved
Hide resolved
...-incremental/src/main/java/com/linkedin/coral/incremental/RelNodeIncrementalTransformer.java
Outdated
Show resolved
Hide resolved
| RelShuttle converter = new RelShuttleImpl() { | ||
| @Override | ||
| public RelNode visit(TableScan scan) { | ||
| RelOptTable originalTable = scan.getTable(); | ||
|
|
||
| // Set relOptSchema | ||
| if (relOptSchema == null) { |
There was a problem hiding this comment.
what is the use case for this check? when will relOptSchema not be present in the table?
There was a problem hiding this comment.
This check is to set relOptSchema only when it has not previously already been set. I don't think it's absolutely necessary but I just added a comment to explain the conditional's purpose.
...-incremental/src/main/java/com/linkedin/coral/incremental/RelNodeIncrementalTransformer.java
Outdated
Show resolved
Hide resolved
...-incremental/src/main/java/com/linkedin/coral/incremental/RelNodeIncrementalTransformer.java
Outdated
Show resolved
Hide resolved
...-incremental/src/main/java/com/linkedin/coral/incremental/IncrementalTransformerResults.java
Outdated
Show resolved
Hide resolved
| IncrementalTransformerResults incrementalTransformerResultsRight = convertRelIncremental(right); | ||
| RelNode incrementalLeft = incrementalTransformerResultsLeft.getIncrementalRelNode(); | ||
| RelNode incrementalRight = incrementalTransformerResultsRight.getIncrementalRelNode(); | ||
| incrementalTransformerResults |
There was a problem hiding this comment.
will it simplify the logic if we make intermediateQueryRelNodes global?
There was a problem hiding this comment.
Yes, we are actually getting rid of IncrementalTransformerResults and keeping all logic inside of the transformer itself (larger discussion in the Slack channel).
What changes are proposed in this pull request, and why are they necessary?
How was this patch tested?