Skip to content

[QueryGenerator] Fix subtle bug that can unintentionally cause giant ON clauses (too many OR/AND/NOT conditions)

Narayanan Iyer requested to merge nars1/YDBOcto:querygenerator into master
  • One of the pipeline jobs failed in the test_jdbc_connection/TJC007 subtest due to a timeout. One generated query (pasted at #573 (comment 393431719)) had taken forever to execute.

  • This query had 4 joins and in some of the ON clauses, there were 20 to 30 OR/AND/NOT conditions. And because of a lot of OR conditions, the DNF expansion caused a combinatorial explosion resulting in the query taking forever to execute. #573 tracks the fact that Octo should avoid such DNF explosions.

  • But the query generator (tests/fixtures/QueryGenerator.m) has code to limit the number of OR/AND/NOT conditions in an ON clause to a max of 3. So it is not clear why 30 conditions were generated in one ON clause.

  • Turns out the way the limiting was done in the query generator had a bug.

    The for loop for i=1:1:loopCount (where loopCount was a variable with a max value of 3) is how the limiting was done in the M program.

    But turns out the loop control variable i was reused as a loop control variable for a nested for loop in multiple places. The nested for loops were of the form for i=1:1:15 do quit:(leftType=rightType) in 2 places.

    This meant it was possible that when the outer for loop control variable i had a value of say 2, the inner for loop could reset the variable i to a smaller value (say 1) which meant that once the inner for loop was done, the outer for loop will resume the next iteration with the next value of i which would still be 2 (and not 3). Depending on how small the nested for loop could reset the value of i we could end up executing the outer for loop for a lot more than the desired 3 times.

  • This is now fixed by using a different variable j in the inner for loop.

  • In addition, there was a line of the form set:(i=15) onClause="",result=" at the end of the outer for loop which checked if the inner for loop gave up and if so it reset the ON clause. This is now fixed in a better fashion by a new variable resetOnClause which is set to 1 whenever any of the inner for loops give up (it is possible if compatible columns for ON conditions cannot be found given the current query circumstances). And this variable then controls whether the entire ON clause is reset to "" or not.

  • There was a 3rd inner for loop where I believe we will always terminate without giving up so instead of treating it like the other 2 inner for loops, I added a do assert(20>j) in that for loop to assert that we never run out of iterations. And if we do, the test will fail the assert and alert us for further debugging.

  • Finally, a commented line that had a comment (that it can be removed if not needed) is now removed as we have been running with that line commented out for a long time and no issues.

Merge request reports