Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix the reverse scan null predicate missing issue for JOIN case #2606

Merged
merged 1 commit into from Jul 27, 2015
Merged

Fix the reverse scan null predicate missing issue for JOIN case #2606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/frontend/org/voltdb/planner/SelectSubPlanAssembler.java
View file
Open in desktop
Expand Up @@ -580,8 +580,11 @@ private AbstractPlanNode getSelectSubPlanForJoin(BranchNode joinNode,
// InnerPlan is an IndexScan. In this case the inner and inner-outer
// non-index join expressions (if any) are in the otherExpr. The former should stay as
// an IndexScanPlan predicate and the latter stay at the NLJ node as a join predicate
List<AbstractExpression> innerExpr = filterSingleTVEExpressions(innerAccessPath.otherExprs);
joinClauses.addAll(innerAccessPath.otherExprs);
ArrayList<AbstractExpression> otherExprs = new ArrayList<AbstractExpression>();
// PLEASE do not update the "innerAccessPath.otherExprs", it may be reused
// for other path evaluation on the other outer side join.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment. Can you identify the method where the innerAccessPath.otherExprs element needs to be reused? Can you identify a specific case where we want to extract a single-TVE expression into innerExpr and also keep it available in innerAccessPath.otherExprs?

Is the idea that we extract all single-TVE expressions as just a first pass to get conditions that MIGHT be indexable on the one specific side, and we don't care yet about the TVEs' tables because we will check that we are on the correct side (for that table) later? I could believe that, but, for the sake of query performance, I would want some confirmation that the final plan does not leave behind a completely redundant join condition that was already enforced by the child scan (either as an index-covered predicate or as a scan post-predicate).

I am suspicious that this SEEMs to specifically solve a join-of-reverse-scan-with-null case.
Does this mean that a reverse scan could still have a null-handling bug in the non-join case?
Or can you explain how this join mis-handling was specifically causing a null handling bug --
was it really a more general bug, and this reverse scan null case just happened to be the one reproducer we found -- one of many possible symptoms of a missing filter?

List<AbstractExpression> innerExpr = filterSingleTVEExpressions(innerAccessPath.otherExprs, otherExprs);
joinClauses.addAll(otherExprs);
AbstractExpression indexScanPredicate = ExpressionUtil.combine(innerExpr);
((IndexScanPlanNode)innerPlan).setPredicate(indexScanPredicate);
}
Expand Down Expand Up @@ -644,19 +647,22 @@ else if (canHaveNLIJ) {
/**
* A method to filter out single TVE expressions.
*
* @param expr List of expressions.
* @param expr List of single TVE expressions.
* @param otherExprs List of expressions other than TVE.
Copy link
Contributor

Choose a reason for hiding this comment

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

The opposite of "single-TVE expressions" is NOT "expressions other than TVE" it is "multi-TVE expressions".

I am hoping that the other possible case -- zero-TVE expressions (constant conditions) -- are not getting mixed in here!

Also, change all comments to hyphenate "single-TVE". A "single TVE expression" can be mistaken to mean "one that IS just a TVE". A "single-TVE expression" more clearly expresses "one that only HAS one TVE".

* @return List of single TVE expressions from the input collection.
* They are also removed from the input.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is now a lie.

*/
private static List<AbstractExpression> filterSingleTVEExpressions(List<AbstractExpression> exprs) {
private static List<AbstractExpression> filterSingleTVEExpressions(List<AbstractExpression> exprs,
List<AbstractExpression> otherExprs) {
List<AbstractExpression> singleTVEExprs = new ArrayList<AbstractExpression>();
for (AbstractExpression expr : exprs) {
List<TupleValueExpression> tves = ExpressionUtil.getTupleValueExpressions(expr);
if (tves.size() == 1) {
singleTVEExprs.add(expr);
} else {
otherExprs.add(expr);
}
}
exprs.removeAll(singleTVEExprs);
return singleTVEExprs;
}

Expand Down
9 changes: 8 additions & 1 deletion tests/frontend/org/voltdb/regressionsuites/TestJoinsSuite.java
View file
Open in desktop
Expand Up @@ -836,23 +836,30 @@ private void subtestOuterJoinMultiTable(Client client)
private void subtestOuterJoinENG8692(Client client)
throws NoConnectionsException, IOException, ProcCallException
{

client.callProcedure("@AdHoc", "INSERT INTO t1 VALUES(1);");
client.callProcedure("@AdHoc", "INSERT INTO t2 VALUES(1);");
client.callProcedure("@AdHoc", "INSERT INTO t3 VALUES(1);");
client.callProcedure("@AdHoc", "INSERT INTO t4 VALUES(1);");
client.callProcedure("@AdHoc", "INSERT INTO t4 VALUES(null);");

String sql;
long MINVAL = Long.MIN_VALUE;

// case 1: missing join expression
sql = "SELECT * FROM t1 INNER JOIN t2 ON t1.i1 = t2.i2 RIGHT OUTER JOIN t3 ON t1.i1 = 1000;";
validateTableOfLongs(client, sql, new long[][]{{MINVAL, MINVAL, 1}});

// case 2: more than 5 table joins
sql = "SELECT * FROM t1 INNER JOIN t2 AS t2_copy1 ON t1.i1 = t2_copy1.i2 "
+ "INNER JOIN t2 AS t2_copy2 ON t1.i1 = t2_copy2.i2 "
+ "INNER JOIN t2 AS t2_copy3 ON t1.i1 = t2_copy3.i2 "
+ "INNER JOIN t2 AS t2_copy4 ON t1.i1 = t2_copy4.i2 "
+ "RIGHT OUTER JOIN t3 ON t1.i1 = t3.i3 AND t3.i3 < -1000;";
validateTableOfLongs(client, sql, new long[][]{{MINVAL, MINVAL, MINVAL, MINVAL, MINVAL, 1}});

// case 3: reverse scan with null data
sql = "SELECT * FROM t1 INNER JOIN t2 ON t1.i1= t2.i2 INNER JOIN t4 ON t4.i4 < 45;";
validateTableOfLongs(client, sql, new long[][]{{1, 1, 1}});
}


Expand Down
4 changes: 3 additions & 1 deletion tests/frontend/org/voltdb/regressionsuites/testjoins-ddl.sql
View file
Open in desktop
Expand Up @@ -40,4 +40,6 @@ PARTITION TABLE P3 ON COLUMN A;
-- ENG-8692
CREATE TABLE t1(i1 INTEGER);
CREATE TABLE t2(i2 INTEGER);
CREATE TABLE t3(i3 INTEGER);
CREATE TABLE t3(i3 INTEGER);
CREATE TABLE t4(i4 INTEGER);
CREATE INDEX t4_idx ON T4 (i4);
6 changes: 3 additions & 3 deletions tests/scripts/examples/sql_coverage/join-template.sql
View file
Open in desktop
Expand Up @@ -46,6 +46,6 @@ SELECT * FROM @fromtables LHS39 @jointype JOIN @fromtables RHS ON LHS39.@idcol
SELECT * FROM @fromtables LHS40 @jointype JOIN @fromtables MHS ON LHS40.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON LHS40.@numcol = RHS.@numcol
SELECT @idcol, @numcol FROM @fromtables LHS41 @jointype JOIN @fromtables MHS ON LHS41.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON LHS41.@numcol = RHS.@numcol

SELECT * FROM @fromtables LHS42 @jointype JOIN @fromtables MHS ON LHS42.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON LHS42.@numcol < 45;
SELECT * FROM @fromtables LHS43 @jointype JOIN @fromtables MHS ON LHS43.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON MHS.@numcol < 45;
SELECT * FROM @fromtables LHS44 @jointype JOIN @fromtables MHS ON LHS44.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON RHS.@numcol < 45;
SELECT * FROM @fromtables LHS42 @jointype JOIN @fromtables MHS ON LHS42.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON LHS42.@numcol _cmp 45;
SELECT * FROM @fromtables LHS43 @jointype JOIN @fromtables MHS ON LHS43.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON MHS.@numcol _cmp 45;
SELECT * FROM @fromtables LHS44 @jointype JOIN @fromtables MHS ON LHS44.@idcol = MHS.@idcol @jointype JOIN @fromtables RHS ON RHS.@numcol _cmp 45;